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
