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

["Günther Noack" <gnoack@google.com>]

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

Yes, sorry for the confusion - that is exactly what I meant to say with "safe".:

Those are the IOCTL commands implemented in fs/ioctl.c which do not go through
f_ops->unlocked_ioctl (or the compat equivalent).

We want to give people a way with Landlock so that they can restrict the use of
device-driver implemented IOCTLs, but where they can keep using the bulk of
more harmless IOCTLs in fs/ioctl.c.

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

Exactly -- there is a surprising number of f_ops->unlocked_ioctl implementations
that already run various resource management and locking logic before even
looking at the command number.  That means that even the command numbers that
are not implemented there are executing code in the driver layer, before the
IOCTL returns with an error.

So the f_ops->unlocked_ioctl() invocation is in itself increasing the surface of
exposed functionality, even completely independent of the command number.  Which
makes the invocation of f_ops->unlocked_ioctl() a security boundary that we
would like to restrict.

—Günther