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
