Re: [PATCH v3 0/5] Landlock: IOCTL support

From: "Günther Noack" <gnoack@google.com>
To: "Mickaël Salaün" <mic@digikod.net>
Cc: Christian Brauner <brauner@kernel.org>,
	linux-security-module@vger.kernel.org,
	 Jeff Xu <jeffxu@google.com>,
	Jorge Lucangeli Obes <jorgelo@chromium.org>,
	 Allen Webb <allenwebb@google.com>,
	Dmitry Torokhov <dtor@google.com>,
	Paul Moore <paul@paul-moore.com>,
	 Konstantin Meskhidze <konstantin.meskhidze@huawei.com>,
	Matt Bobrowski <repnop@google.com>,
	 linux-fsdevel@vger.kernel.org, "Theodore Ts'o" <tytso@mit.edu>
Subject: Re: [PATCH v3 0/5] Landlock: IOCTL support
Date: Thu, 26 Oct 2023 00:07:28 +0200	[thread overview]
Message-ID: <ZTmRoESR5eXEA_ky@google.com> (raw)
In-Reply-To: <20231020.moefooYeV9ei@digikod.net>

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

  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.

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.

—Günther