Re: [PATCH v12 1/9] security: Introduce ENOFILEOPS return value for IOCTL hooks

From: "Mickaël Salaün" <mic@digikod.net>
To: "Günther Noack" <gnoack@google.com>
Cc: Arnd Bergmann <arnd@arndb.de>,
	linux-security-module@vger.kernel.org,
	 Jeff Xu <jeffxu@google.com>,
	Jorge Lucangeli Obes <jorgelo@chromium.org>,
	 Allen Webb <allenwebb@google.com>,
	Dmitry Torokhov <dtor@google.com>,
	 Paul Moore <paul@paul-moore.com>,
	Konstantin Meskhidze <konstantin.meskhidze@huawei.com>,
	 Matt Bobrowski <repnop@google.com>,
	linux-fsdevel@vger.kernel.org,
	 Christian Brauner <brauner@kernel.org>
Subject: Re: [PATCH v12 1/9] security: Introduce ENOFILEOPS return value for IOCTL hooks
Date: Tue, 26 Mar 2024 15:28:08 +0100	[thread overview]
Message-ID: <20240326.ooCheem1biV2@digikod.net> (raw)
In-Reply-To: <ZgLJG0aN0psur5Z7@google.com>

On Tue, Mar 26, 2024 at 02:09:47PM +0100, Günther Noack wrote:
> On Tue, Mar 26, 2024 at 12:58:42PM +0100, Arnd Bergmann wrote:
> > On Tue, Mar 26, 2024, at 11:10, Mickaël Salaün wrote:
> > > On Tue, Mar 26, 2024 at 10:33:23AM +0100, Arnd Bergmann wrote:
> > >> On Tue, Mar 26, 2024, at 09:32, Mickaël Salaün wrote:
> > >> >
> > >> > This is indeed a simpler solution but unfortunately this doesn't fit
> > >> > well with the requirements for an access control, especially when we
> > >> > need to log denied accesses.  Indeed, with this approach, the LSM (or
> > >> > any other security mechanism) that returns ENOFILEOPS cannot know for
> > >> > sure if the related request will allowed or not, and then it cannot
> > >> > create reliable logs (unlike with EACCES or EPERM).
> > >> 
> > >> Where does the requirement come from specifically, i.e.
> > >> who is the consumer of that log?
> > >
> > > The audit framework may be used by LSMs to log denials.
> > >
> > >> 
> > >> Even if the log doesn't tell you directly whether the ioctl
> > >> was ultimately denied, I would think logging the ENOFILEOPS
> > >> along with the command number is enough to reconstruct what
> > >> actually happened from reading the log later.
> > >
> > > We could indeed log ENOFILEOPS but that could include a lot of allowed
> > > requests and we usually only want unlegitimate access requests to be
> > > logged.  Recording all ENOFILEOPS would mean 1/ that logs would be
> > > flooded by legitimate requests and 2/ that user space log parsers would
> > > need to deduce if a request was allowed or not, which require to know
> > > the list of IOCTL commands implemented by fs/ioctl.c, which would defeat
> > > the goal of this specific patch.
> > 
> > Right, makes sense. Unfortunately that means I don't see any
> > option that I think is actually better than what we have today,
> > but that forces the use of a custom whitelist or extra logic in
> > landlock.
> > 
> > I didn't really mind having an extra hook for the callbacks
> > in addition to the top-level one, but that was already nacked.
> 
> Thank you both for the review!
> 
> I agree, this approach would break logging.
> 
> As you both also said, I also think this leads us back to the approach
> where we hardcode the allow-list of permitted IOCTL commands in the
> file_ioctl hook.
> 
> I think this approach has the following upsides:
> 
>   1. Landlock's (future) audit logging will be able to log exactly
>      which IOCTL commands were denied.
>   2. The allow-list of permitted IOCTL commands can be reasoned about
>      locally and does not accidentally change as a side-effect of a
>      change to the implementation of fs/ioctl.c.
> 
> A risk that we have is:
> 
>   3. We might miss changes to fs/ioctl.c which we might want to
>      reflect in Landlock.
> 
> 
> To think about 2 and 3 in more concrete terms, I categorized the
> scenarios in which IOCTL cmd implementations can get added to or
> removed from the do_vfs_ioctl() layer:
> 
>   Case A: New cmd added to do_vfs_ioctl layer
> 
>     We want to double check whether this cmd should be included in
>     Landlock's allow list.  (Because the command is new, there are no
>     existing users of the IOCTL command, so we can't break anyone and
>     we it probably does not require to be made explicit in Landlock's
>     ABI versioning scheme.)
> 
>     ==> We need to catch these changes early,
>         and should do Landlock-side changes in sync.
> 
>   Case B: Existing cmd removed from do_vfs_ioctl layer
> 
>     (This case is unlikely, as it would be a backwards-incompatible
>     change in the ioctl interface.)
> 
>   Case C: Existing cmd is moved from f_ops->..._ioctl() to do_vfs_ioctl()
> 
>     (For regular files, I think this has happened for the XFS
>     "reflink" ioctls before, which became features in other COW file
>     systems as well.)
> 
>     If such a change happens for device files (which are in scope for
>     Landlock's IOCTL feature), we should catch these changes.  We
>     should consider whether the affected IOCTL command should be
>     allow-listed.  Strictly speaking, if we allow-list the cmd, which
>     was previously not allow-listed, this would mean that Landlock's
>     DEV_IOCTL feature would have different semantics than before
>     (permitting an additional cmd), and it would potentially be a
>     backwards-incompatible change that need to be made explicit in
>     Landlock's ABI versioning.
> 
>   Case D: Existing cmd is moved from do_vfs_ioctl() to f_ops->..._ioctl()
> 
>     (This case seems also very unlikely to me because it decentralizes
>     the reasoning about these IOCTL APIs which are currently centrally
>     controlled and must stay backwards compatible.)
> 

Excellent summary! You should put a link to this email in the commit
message and quickly explain why we ended up here.

> 
> 
> So -- a proposal:
> 
> * Keep the original implementation of fs/ioctl.c
> * Implement Landlock's file_ioctl hook with a switch(cmd) where we list
>   the permitted IOCTL commands from do_vfs_ioctl.
> * Make sure Landlock maintainers learn about changes to do_vfs_ioctl():
>   * Put a warning on top of do_vfs_ioctl() to loop in Landlock
>     maintainers

Yes please.

>   * Set up automation to catch such changes?
> 
> 
> Open questions are:
> 
> * Mickaël, do you think we should use a more device-file-specific
>   subset of IOCTL commands again, or would you prefer to stick to the
>   full list of all IOCTL commands implemented in do_vfs_ioctl()?

We should stick to the device-file-specific IOCTL commands [1] but we
should probably complete the switch-case with commented cases (in the
same order as in do_vfs_ioctl) to know which one were reviewed (as in
[1]).  These helpers should now be static and in security/landlock/fs.c

[1] https://lore.kernel.org/r/20240219183539.2926165-1-mic@digikod.net

BTW, there are now two new IOCTL commands (FS_IOC_GETUUID and
FS_IOC_GETFSSYSFSPATH) masking device-specific IOCTL ones.

> 
> * Regarding automation:
> 
>   We could additionally set up some automation to watch changes to
>   do_vfs_ioctl().  Given the low rate of change in that corner, we
>   might get away with extracting the relevant portion of the C file
>   (awk '/^static int do_vfs_ioctl/, /^\}/' fs/ioctl.c) and watching
>   for any changes.  It is brittle, but the rate of changes is low, and
>   reasoning about source code is difficult and error prone as well.
> 
>   In an ideal world this could maybe be part of the kernel test
>   suites, but I still have not found the right place where this could
>   be hooked in.  Do you have any thoughts on this?

I think this change should be enough for now:

diff --git a/MAINTAINERS b/MAINTAINERS
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12222,6 +12222,7 @@ W:	https://landlock.io
 T:	git https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git
 F:	Documentation/security/landlock.rst
 F:	Documentation/userspace-api/landlock.rst
+F:	fs/ioctl.c
 F:	include/uapi/linux/landlock.h
 F:	samples/landlock/
 F:	security/landlock/

It should be OK considered the number of patches matching this file: a
subset of https://lore.kernel.org/all/?q=dfn%3Afs%2Fioctl.c
