[RFC PATCH] fs: Add vfs_masks_device_ioctl*() helpers

[Mickaël Salaün]

On Fri, Mar 08, 2024 at 02:22:58PM -0500, Paul Moore wrote:
> On Fri, Mar 8, 2024 at 4:29 AM Mickaël Salaün <mic at digikod.net> wrote:
> > On Fri, Mar 08, 2024 at 08:02:13AM +0100, Arnd Bergmann wrote:
> > > On Fri, Mar 8, 2024, at 00:09, Dave Chinner wrote:
> > > > On Thu, Mar 07, 2024 at 03:40:44PM -0500, Paul Moore wrote:
> > > >> On Thu, Mar 7, 2024 at 7:57 AM Günther Noack <gnoack at google.com> wrote:
> > > >> I need some more convincing as to why we need to introduce these new
> > > >> hooks, or even the vfs_masked_device_ioctl() classifier as originally
> > > >> proposed at the top of this thread.  I believe I understand why
> > > >> Landlock wants this, but I worry that we all might have different
> > > >> definitions of a "safe" ioctl list, and encoding a definition into the
> > > >> LSM hooks seems like a bad idea to me.
> > > >
> > > > I have no idea what a "safe" ioctl means here. Subsystems already
> > > > restrict ioctls that can do damage if misused to CAP_SYS_ADMIN, so
> > > > "safe" clearly means something different here.
> > >
> > > That was my problem with the first version as well, but I think
> > > drawing the line between "implemented in fs/ioctl.c" and
> > > "implemented in a random device driver fops->unlock_ioctl()"
> > > seems like a more helpful definition.
> > >
> > > This won't just protect from calling into drivers that are lacking
> > > a CAP_SYS_ADMIN check, but also from those that end up being
> > > harmful regardless of the ioctl command code passed into them
> > > because of stupid driver bugs.
> >
> > Indeed.
> >
> > "safe" is definitely not the right word, it is too broad, relative to
> > use cases and threat models.  There is no "safe" IOCTL.
> >
> > Let's replace "safe IOCTL" with "IOCTL always allowed in a Landlock
> > sandbox".
> 
> Which is a problem from a LSM perspective as we want to avoid hooks
> which are tightly bound to a single LSM or security model.  It's okay
> if a new hook only has a single LSM implementation, but the hook's
> definition should be such that it is reasonably generalized to support
> multiple LSM/models.

As any new hook, there is a first user.  Obviously this new hook would
not be restricted to Landlock, it is a generic approach.  I'm pretty
sure a few hooks are only used by one LSM though. ;)

> 
> > Our assumptions are (in the context of Landlock):
> >
> > 1. There are IOCTLs tied to file types (e.g. block device with
> >    major/minor) that can easily be identified from user space (e.g. with
> >    the path name and file's metadata).  /dev/* files make sense for user
> >    space and they scope to a specific use case (with relative
> >    privileges).  This category of IOCTLs is implemented in standalone
> >    device drivers (for most of them).
> >
> > 2. Most user space processes should not be denied access to IOCTLs that
> >    are managed by the VFS layer or the underlying filesystem
> >    implementations.  For instance, the do_vfs_ioctl()'s ones (e.g.
> >    FIOCLEX, FIONREAD) should always be allowed because they may be
> >    required to legitimately use files, and for performance and security
> >    reasons (e.g. fs-crypt, fsverity implemented at the filesystem layer).
> >    Moreover, these IOCTLs should already check the read/write permission
> >    (on the related FD), which is not the case for most block/char device
> >    IOCTL.
> >
> > 3. IOCTLs to pipes and sockets are out of scope.  They should always be
> >    allowed for now because they don't directly expose files' data but
> >    IPCs instead, and we are focusing on FS access rights for now.
> >
> > We want to add a new LANDLOCK_ACCESS_FS_IOCTL_DEV right that could match
> > on char/block device's specific IOCTLs, but it would not have any impact
> > on other IOCTLs which would then always be allowed (if the sandboxed
> > process is allowed to open the file).
> >
> > Because IOCTLs are implemented in layers and all IOCTLs commands live in
> > the same 32-bit namespace, we need a way to identify the layer
> > implemented by block and character devices.  The new LSM hook proposal
> > enables us to cleanly and efficiently identify the char/block device
> > IOCTL layer with an additional check on the file type.
> 
> I guess I should wait until there is an actual patch, but as of right
> now a VFS ioctl specific LSM hook looks far too limited to me and
> isn't something I can support at this point in time.  It's obviously
> limited to only a subset of the ioctls, meaning that in order to have
> comprehensive coverage we would either need to implement a full range
> of subsystem ioctl hooks (ugh), or just use the existing
> security_file_ioctl().

I think there is a misunderstanding.  The subset of IOCTL commands the
new hook will see would be 99% of them (i.e. all except those
implemented in fs/ioctl.c).  Being able to only handle this (big) subset
would empower LSMs to control IOCTL commands without collision (e.g. the
same command/value may have different meanings according to the
implementation/layer), which is not currently possible (without manual
tweaking).

This proposal is to add a new hook for the layer just beneath the VFS
catch-all IOCTL implementation.  This layer can then differentiate
between the underlying implementation according to the file properties.
There is no need for additional hooks for other layers/subsystems.

The existing security_file_ioctl() hook is useful to catch all IOCTL
commands, but it doesn't enable to identify the underlying target and
then the semantic of the command.  Furthermore, as Günther said, an
IOCTL call can already do kernel operations without looking at the
command, but we would then be able to identify that by looking at the
char/block device file for instance.

> I understand that this makes things a bit more
> complicated for Landlock's initial ioctl implementation, but
> considering my thoughts above and the fact that Landlock's ioctl
> protections are still evolving I'd rather not add a lot of extra hooks
> right now.

Without this hook, we'll need to rely on a list of allowed IOCTLs, which
will be out-of-sync eventually.  It would be a maintenance burden and an
hacky approach.

We're definitely open to new proposals, but until now this is the best
approach we found from a maintenance, performance, and security point of
view.