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:

quoted

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.

quoted

quoted

    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.

quoted

quoted

    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.

quoted

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