[PATCH v3 0/5] Landlock: IOCTL support

[Mickaël Salaün]

On Fri, Nov 03, 2023 at 02:06:53PM +0100, Günther Noack wrote:
> 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.

I guess we can put the expand functions in fs.c .

But at that point we need an actual patch to discuss such details.

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

If users set the GFX right, the library should fallback to the IOCTL
right if GFX is not supported.

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

Actually, I think these groups should be static const variables defined
in the function that uses them, so the naming would not change much.
Maybe something like ioctl_groupN?

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

Definitely

> 
> —Günther