Re: [PATCH v10 1/9] security: Create security_file_vfs_ioctl hook

["Mickaël Salaün" <mic@digikod.net>]

On Thu, Mar 14, 2024 at 01:56:25PM -0400, Paul Moore wrote:
> On Sat, Mar 9, 2024 at 2:53 AM Günther Noack <gnoack@google.com> wrote:
> >
> > This LSM hook gets called just before the fs/ioctl.c logic delegates
> > the requested IOCTL command to the file-specific implementation as
> > implemented by f_op->unlocked_ioctl (or f_op->ioctl_compat).
> >
> > It is impractical for LSMs to make security guarantees about these
> > f_op operations without having intimate knowledge of how they are
> > implemented.
> >
> > Therefore, depending on the enabled Landlock policy, Landlock aims to
> > block the calls to filp->f_op->unlocked_ioctl(), but permit the calls
> > to the IOCTL commands which are already implemented in fs/ioctl.c.
> >
> > The current call graph is:
> >
> >   * ioctl syscall
> >     * security_file_ioctl() LSM hook
> >     * do_vfs_ioctl() - standard operations
> >       * file_ioctl() - standard file operations
> >     * vfs_ioctl() - delegate to file (if do_vfs_ioctl() is a no-op)
> >       * filp->f_op->unlocked_ioctl()
> >
> > Why not use the existing security_file_ioctl() hook?
> >
> > With the existing security_file_ioctl() hook, it is technically
> > feasible to prevent the call to filp->f_op->unlocked_ioctl(), but it
> > would be difficult to maintain: security_file_ioctl() gets called
> > further up the call stack, so an implementation of it would need to
> > predict whether the logic further below will decide to call
> > f_op->unlocked_ioctl().  That can only be done by mirroring the logic
> > in do_vfs_ioctl() to some extent, and keeping this in sync.
> 
> Once again, I don't see this as an impossible task, and I would think
> that you would want to inspect each new ioctl command/op added in
> do_vfs_ioctl() anyway to ensure it doesn't introduce an unwanted
> behavior from a Landlock sandbox perspective.

About the LANDLOCK_ACCESS_FS_IOCTL_DEV semantic, we only care about the
IOCTLs that are actually delivered to device drivers, so any new IOCTL
directly handled by fs/ioctl.c should be treated the same way for this
access right.

> Looking at the git
> log/blame, it also doesn't appear that new do_vfs_ioctl() ioctls are
> added very frequently, meaning that keeping Landlock sync'd with
> fs/ioctl.c shouldn't be a terrible task.

do_vfs_ioctl() is indeed not changed often, but this doesn't mean we
should not provide strong guarantees, avoid future security bugs, lower
the maintenance cost, and improve code readability.

> 
> I'm also not excited about the overlap between the existing
> security_file_ioctl() hook and the proposed security_file_vfs_ioctl()
> hook.  There are some cases where we have no choice and we have to
> tolerate the overlap, but this doesn't look like one of those cases to
> me.
> 
> I'm sorry, but I don't agree with this new hook.

OK, I sent a new RFC (in reply to your email) as an alternative
approach.  Instead of adding a new LSM hook, this patch adds the
vfs_get_ioctl_handler() helper and some code refactoring that should be
both interesting for the VFS subsystem and for Landlock.  I guess this
could be interesting for other security mechanisms as well (e.g. BPF
LSM).  What do you think?

Arnd, Christian, would this sound good to you?