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

[Paul Moore]

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.

> 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 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.

-- 
paul-moore.com