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

[Günther Noack]

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



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
  * 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()?

* 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?

Thanks,
—Günther