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

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

On Fri, Mar 08, 2024 at 05:25:21PM -0500, Paul Moore wrote:
> On Fri, Mar 8, 2024 at 3:12 PM Mickaël Salaün <mic@digikod.net> wrote:
> > 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@digikod.net> wrote:
> > > > 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. ;)
> 
> Sure, as I said above, it's okay for there to only be a single LSM
> implementation, but the basic idea behind the hook needs to have some
> hope of being generic.  Your "let's redefine a safe ioctl as 'IOCTL
> always allowed in a Landlock sandbox'" doesn't fill me with confidence
> about the hook being generic; who knows, maybe it will be, but in the
> absence of a patch, I'm left with descriptions like those.

FWIW, the existing IOCTL hook is used in the following places:

* TOMOYO: seemingly configurable per IOCTL command?  (I did not dig deeper)
* SELinux: has a hardcoded switch of IOCTL commands, some with special checks.
  These are also a subset of the do_vfs_ioctl() commands,
  plus KDSKBENT, KDSKBSENT (from ioctl_console(2)).
* Smack: Decomposes the IOCTL command number to look at the _IOC_WRITE and
  _IOC_READ bits. (This is a known problematic approach, because (1) these bits
  describe whether the argument is getting read or written, not whether the
  operation is a mutating one, and (2) some IOCTL commands do not adhere to the
  convention and don't use these macros)

AppArmor does not use the LSM IOCTL hook.


> > > 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.
> 
> Welcome to the painful world of a LSM developer, ioctls are not the
> only place where this is a problem, and it should be easy enough to
> watch for changes in the ioctl list and update your favorite LSM
> accordingly.  Honestly, I think that is kinda the right thing anyway,
> I'm skeptical that one could have a generic solution that would
> automatically allow or disallow a new ioctl without potentially
> breaking your favorite LSM's security model.  If a new ioctl is
> introduced it seems like having someone manually review it's impact on
> your LSM would be a good idea.

We are concerned that we will miss a change in do_vfs_ioctl(), which we would
like to reflect in the matching Landlock code.  Do other LSMs have any
approaches for that which go beyond just watching the do_vfs_ioctl()
implementation for changes?


> > 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.
> 
> At this point it's probably a good idea to post another RFC patch with
> your revised idea, if nothing else it will help rule out any
> confusion.  While I remain skeptical, perhaps I am misunderstanding
> the design and you'll get my apology and an ACK, but be warned that as
> of right now I'm not convinced.

Thanks you for your feedback!

Here is V10 with the approach where we use a new LSM hook:
https://lore.kernel.org/all/20240309075320.160128-1-gnoack@google.com/

I hope this helps to clarify the approach a bit.  I'm explaining it in more
detail again in the commit which adds the LSM hook, including a call graph, and
avoiding the word "safe" this time ;-)

Let me know what you think!

Thanks!
—Günther