Re: [PATCH v3 0/5] Landlock: IOCTL support

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

Hello Mickaël!

Thanks for the review!

On Thu, Oct 26, 2023 at 04:55:30PM +0200, Mickaël Salaün wrote:
> The third column "IOCTL unhandled" is not reflected here. What about
> this patch?
> 
> if (!(handled & LANDLOCK_ACCESS_FS_IOCTL)) {
>   return am | dst;
> }

You are right that this needs special treatment.  The reasoning is the scenario
where a user creates a ruleset where LANDLOCK_ACCESS_FS_READ_FILE is handled,
but LANDLOCK_ACCESS_FS_IOCTL is not.  In that case, when a file is opened for
which we do not have the READ_FILE access right, without your additional check,
the IOCTLs associated with READ_FILE would be forbidden.  But this is also a
Landlock usage that was possible before the introduction of the IOCTL handling,
and so all IOCTLs should work in that case.

> 
> >     if (handled & src) {
> >       /* If "src" access right is handled, populate "dst" from "src". */
> >       return am | ((am & src) ? dst : 0);
> >     } else {
> >       /* Otherwise, populate "dst" flag from "ioctl" flag. */
> >       return am | ((am & LANDLOCK_ACCESS_FS_IOCTL) ? dst : 0);
> >     }
> >   }
> > 
> >   static access_mask_t expand_all_ioctl(access_mask_t handled, access_mask_t am)
> >   {
> 
> Instead of reapeating "am | " in expand_ioctl() and assigning am several
> times in expand_all_ioctl(), you could simply do something like that:
> 
> return am |
> 	expand_ioctl(handled, am, ...) |
> 	expand_ioctl(handled, am, ...) |
> 	expand_ioctl(handled, am, ...);

Agreed, this is more elegant.  Will do.


> >     am = expand_ioctl(handled, am,
> >                       LANDLOCK_ACCESS_FS_WRITE_FILE,
> > 		      IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G4);
> >     am = expand_ioctl(handled, am,
> >                       LANDLOCK_ACCESS_FS_READ_FILE,
> > 		      IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G3);
> >     am = expand_ioctl(handled, am,
> >                       LANDLOCK_ACCESS_FS_READ_DIR,
> > 		      IOCTL_CMD_G1);
> >     return am;
> >   }
> > 
> >   and then during the installing of a ruleset, we'd call
> >   expand_all_ioctl(handled, access) for each specified file access, and
> >   expand_all_ioctl(handled, handled) for the handled access rights,
> >   to populate the synthetic IOCTL_CMD_G* access rights.
> 
> We can do these transformations directly in the new
> landlock_add_fs_access_mask() and landlock_append_fs_rule().

Working on these changes, the location of these transformations is one of the
last outstanding problems that I don't like yet.

I have added the expansion code to landlock_add_fs_access_mask() and
landlock_append_fs_rule() as you suggested.

This works, but as a result, this (somewhat complicated) expansion logic is now
part of the ruleset.o module, where it seems a bit too FS-specific.  I think
that maybe we can pull this out further, but I'll probably send you a patch set
with the current status before doing that, so that we are on the same page.


> Please base the next series on
> https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git/log/?h=next
> This branch might be rebased from time to time, but only minor changes
> will get there.

OK, will do.


In summary, I'll send a patch soon.

FYI, some open questions I still have are:

* Logic
  * How will userspace libraries handle best-effort fallback,
    when expanded IOCTL access rights come into play?
    (Still need to think about this more.)
* Internal code layout
  * Move expansion logic out of ruleset.o module into syscalls.o?
  * Find more appropriate names for IOCTL_CMD_G1,...,IOCTL_CMD_G4

but we can discuss these in the context of the next patch set.

—Günther