[PATCH v3 0/5] Landlock: IOCTL support

[Mickaël Salaün]

On Thu, Oct 26, 2023 at 12:07:28AM +0200, Günther Noack wrote:
> On Fri, Oct 20, 2023 at 04:57:39PM +0200, Mickaël Salaün wrote:
> > On Fri, Oct 20, 2023 at 12:09:36AM +0200, Günther Noack wrote:
> > > * We introduce a flag in struct landlock_ruleset_attr which controls whether the
> > >   graphics-related IOCTLs are controlled through the LANDLOCK_ACCESS_FS_GFX
> > >   access right, rather than through LANDLOCK_ACCESS_FS_IOCTL.
> > > 
> > >   (This could potentially also be put in the "flags" argument to
> > >   landlock_create_ruleset(), but it feels a bit more appropriate in the struct I
> > >   think, as it influences the interpretation of the logic.  But I'm open to
> > >   suggestions.)
> > > 
> > 
> > What would be the difference with creating a
> > LANDLOCK_ACCESS_FS_GFX_IOCTL access right?
> > 
> > The main issue with this approach is that it complexifies the usage of
> > Landlock, and users would need to tweak more knobs to configure a
> > ruleset.
> > 
> > What about keeping my proposal (mainly the IOCTL handling and delegation
> > logic) for the user interface, and translate that for kernel internals
> > to your proposal? See the below example.
> 
> Yes!
> 
> I have pondered this for about a day now, and tried to break the example in
> various ways, but I believe you are right with this -- I think we can actually
> use the "handled" flags to control the IOCTL grouping, and then translate all of
> it quickly to synthetic access rights for the internal logic.  When doing the
> translation only once during ruleset enablement time, we can keep using the
> existing logic for the synthetic rights and it'll obviously work correctly when
> layers are stacked.  (I paraphrase it in more detail at the end, to make sure we
> are on the same page. -- But I think we are.)
> 
> 
> > > Example: Without the flag, the IOCTL groups will be:
> > > 
> > >   These are always permitted:   FIOCLEX, FIONCLEX, FIONBIO, etc.
> > >   LANDLOCK_ACCESS_FS_READ_FILE: controls FIONREAD
> > >   LANDLOCK_ACCESS_FS_IOCTL:     controls all other IOCTL commands
> > > 
> > > but when users set the flag, the IOCTL groups will be:
> > > 
> > >   These are always permitted:   FIOCLEX, FIONCLEX, FIONBIO, etc.
> > >   LANDLOCK_ACCESS_FS_READ_FILE: controls FIONREAD
> > >   LANDLOCK_ACCESS_FS_GFX:       controls (list of gfx-related IOCTLs)
> > >   LANDLOCK_ACCESS_FS_IOCTL:     controls all other IOCTL commands
> > > 
> > 
> > Does this mean that handling LANDLOCK_ACCESS_FS_GFX without the flag
> > would not allow GFX-related IOCTL commands? Thit would be inconsistent
> > with the way LANDLOCK_ACCESS_FS_READ_FILE is handled.
> 
> Yes, that is how I had imagined that.  It's true that it's slightly inconsistent
> in usage, and you are right that it creates some new concepts in the API which
> are maybe avoidable.  Let's try it the way you proposed and control it with the
> "handled" flags.
> 
> 
> > Would this flag works with non-GFX access rights as well? Would there be
> > potentially one new flag per new access right?
> > 
> > > 
> > > Implementation-wise, I think it would actually look very similar to what would
> > > be needed for your proposal of having a new special meaning for "handled".  It
> > > would have the slight advantage that the new flag is actually only needed at the
> > > time when we introduce a new way of grouping the IOCTL commands, so we would
> > > only burden users with the additional complexity when it's actually required.
> > 
> > Indeed, and burdening users with more flags would increase the cost of
> > (properly) using Landlock.
> > 
> > I'm definitely in favor to make the Landlock interface as simple as
> > possible, taking into account the inherent compatibilty complexity, and
> > pushing most of this complexity handling to user space libraries, and if
> > it not possible, pushing the rest of the complexity into the kernel.
> 
> Ack, sounds good.
> 
> 
> > > One implementation approach that I find reasonable to think about is to create
> > > "synthetic" access rights when rulesets are enabled.  That is, we introduce
> > > LANDLOCK_ACCESS_FS_SYNTHETIC_GFX_IOCTL (name TBD), but we keep this constant
> > > private to the kernel.
> > > 
> > > * *At ruleset enablement time*, we populate the bit for this access right either
> > >   from the LANDLOCK_ACCESS_FS_GFX or the LANDLOCK_ACCESS_FS_IOCTL bit from the
> > >   same access_mask_t, depending on the IOCTL grouping which the ruleset is
> > >   configured with.
> > > 
> > > * *In hook_file_open*, we then check for LANDLOCK_ACCESS_FS_SYNTHETIC_GFX_IOCTL
> > >   for the GFX-related IOCTL commands.
> > > 
> > > I'm in favor of using the synthetic access rights, because I find it clearer to
> > > understand that the effective access rights for a file from different layers are
> > > just combined with a bitwise AND, and will give the right results.  We could
> > > probably also make these path walk helpers aware of the special cases and only
> > > have the synthetic right in layer_masks_dom, but I'd prefer not to complicate
> > > these helpers even further.
> > 
> > I like this synthetic access right approach, but what worries me is that
> > it will potentially double the number of access rights. This is not an
> > issue for the handled access right (i.e. per ruleset layer), but we
> > should avoid that for allowed accesses (i.e. rules). Indeed, the
> > layer_masks[] size is proportional to the number of potential allowed
> > access rights, and increasing this array could increase the kernel stack
> > size (see is_access_to_paths_allowed).  It would not be an issue for now
> > though, we have a lot of room, it is just something to keep in mind.
> 
> Yes, acknowledged.
> 
> FWIW, LANDLOCK_ACCESS_FS_IOCTL is already 1 << 15, so adding the synthetic
> rights will indeed make access_mask_t go up to 32 bit.  (This was already done
> in the patch for the metadata access, but that one was not merged yet.)  I also
> feel that the stack usage is the case where this is most likely to be an issue.
> 
> 
> > Because of the way we need to compare file hierarchies (cf. FS_REFER),
> > it seems to be safer to only rely on (synthetic) access rights. So I
> > think it is the right approach.
> > 
> > > 
> > > 
> > > Sorry for the long mail, I hope that the examples clarify it a bit. :)
> > > 
> > > In summary, it seems conceptually cleaner to me to control every IOCTL command
> > > with only one access right, and let users control which one that should be with
> > > a separate flag, so that "handled" keeps its original semantics.  It would also
> > > have the upside that we can delay that implementation until the time where we
> > > actually introduce new IOCTL-aware access rights on top of the current patch st.
> > 
> > I don't see how we'll not get an inconsistent logic: a first one with
> > old/current access rights, and another one for future access rights
> > (e.g. GFX).
> > 
> > > 
> > > I'd be interested to hear your thoughts on it.
> > 
> > Thanks for this detailed explanation, that is useful.
> > 
> > I'm in favor of the synthetic access right, but I'd like to not add
> > optional flags to the user API.  What do you think about the kernel
> > doing the translation to the synthetic access rights?
> > 
> > To make the reasoning easier for the kernel implementation, following
> > the synthetic access rights idea, we can create these groups:
> > 
> > * IOCTL_CMD_G1: FIOQSIZE
> > * IOCTL_CMD_G2: FS_IOCT_FIEMAP | FIBMAP | FIGETBSZ
> > * IOCTL_CMD_G3: FIONREAD | FIDEDUPRANGE
> > * IOCTL_CMD_G4: FICLONE | FICLONERANGE | FS_IOC_RESVSP | FS_IOC_RESVSP64
> >   | FS_IOC_UNRESVSP | FS_IOC_UNRESVSP64 | FS_IOC_ZERO_RANGE
> > 
> > Existing (and future) access rights would automatically get the related
> > IOCTL fine-grained rights *if* LANDLOCK_ACCESS_FS_IOCTL is handled:
> > * LANDLOCK_ACCESS_FS_WRITE_FILE: IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G4
> > * LANDLOCK_ACCESS_FS_READ_FILE: IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G3
> > * LANDLOCK_ACCESS_FS_READ_DIR: IOCTL_CMD_G1
> > 
> > This works with the ruleset handled access rights and the related rules
> > allowed accesses by simply ORing the access rights.
> > 
> > We should also keep in mind that some IOCTL commands may only be related
> > to some specific file types or filesystems, either now or in the future
> > (see the GFX example).
> 
> I am coming around to your approach with using "handled" bits to determine the
> grouping.  Let me paraphrase some key concepts to make sure we are on the same
> page:
> 
> * The IOCTL groups are modeled as synthetic access rights, IOCTL_CMD_G1...G4 in
>   your example.  Each IOCTL command maps to exactly one of these groups.
> 
>   Because the presence of these groups is an implementation detail in the
>   kernel, we can adapt it later and make it more fine-grained if needed.
> 
> * We use "handled" bits like LANDLOCK_ACCESS_FS_WRITE_FILE to determine the
>   synthetic access rights.
> 
>   We can populate the synthetic IOCTL_CMD_G1...G4 groups depending on how the
>   "handled" bits are populated.
> 
>   In my understanding, the logic could roughly be this:
> 
>   static access_mask_t expand_ioctl(access_mask_t handled, access_mask_t am,
>                                     access_mask_t src, access_mask_t dst)
>   {

The third column "IOCTL unhandled" is not reflected here. What about
this patch?

if (!(handled & LANDLOCK_ACCESS_FS_IOCTL)) {
  return am | dst;
}

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

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

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.

> 
>   In expand_ioctl() above, if "src" is *not* handled, we populate the associated
>   synthetic access rights "dst" from the value in LANDLOCK_ACCESS_FS_IOCTL.
>   With that, when enabling a ruleset, we map everything to the most specific
>   grouping which is available, and later on, the LSM hook can just ignore that
>   different grouping configurations are possible.
> 
> * In the ioctl LSM hook, each possible cmd is controlled by exactly one access
>   right.  The ones that you have listed are all controlled by one of the
>   IOCTL_CMD_G1...G4 access rights, and all others by LANDLOCK_ACCESS_FS_IOCTL.
> 
> I was previously concerned that the usage of "handled" to control the grouping
> would be at odds with the layer composition logic, but with this logic, we are
> now mapping these to the synthetic access rights at enablement time, and all the
> ruleset composition logic can stay working as it is (at least until we run out
> of bits in access_mask_t).
> 
> I've also been concerned before that we would break compatibility across
> versions, but this also seems less likely now that we've discussed this in all
> this detail %-)
> 
> I suspect that the normal upgrade path from one Landlock version to the next
> will be for most users to always use the full set of "handled" flags that their
> library knows about.  When we add the hypothetical "GFX" flag to that set, this
> will change the IOCTL grouping a bit, so that files which were previously listed
> as having the LANDLOCK_ACCESS_FS_IOCTL right, might now not be enabled for GFX
> ioctls.  But that is (A) probably correct anyway in most cases, and (B) users
> upgrading from one Landlock ABI version to the next have a chance to read their
> library changelog as part of that upgrade.

Yes, explicitly adding a new flag to a function argument should indeed
lead to read the related documentation, and hopefully test the code in
an up-to-date sandbox environment!

This strategy should help avoid long-term use of the generic
LANDLOCK_ACCESS_FS_IOCTL but converge to new dedicated access rights
instead.

> 
> I think this is a reasonable approach.  If you agree, I'm willing to give it a
> shot and adapt the patch set to implement that.

This looks great!

> 
> —Günther