Re: [PATCH v5 2/7] landlock: Add IOCTL access right

["Günther Noack" <gnoack@google.com>]

On Mon, Nov 20, 2023 at 08:43:30PM +0100, Mickaël Salaün wrote:
> On Fri, Nov 17, 2023 at 04:49:15PM +0100, Günther Noack wrote:
> > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP1	(LANDLOCK_LAST_PUBLIC_ACCESS_FS << 1)
> > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP2	(LANDLOCK_LAST_PUBLIC_ACCESS_FS << 2)
> > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP3	(LANDLOCK_LAST_PUBLIC_ACCESS_FS << 3)
> > +#define LANDLOCK_ACCESS_FS_IOCTL_GROUP4	(LANDLOCK_LAST_PUBLIC_ACCESS_FS << 4)
> 
> Please move this LANDLOCK_ACCESS_FS_IOCTL_* block to fs.h
> 
> We can still create the public and private masks in limits.h but add a
> static_assert() to make sure there is no overlap.

Done.


> >  	/* Checks content (and 32-bits cast). */
> > -	if ((ruleset_attr.handled_access_fs | LANDLOCK_MASK_ACCESS_FS) !=
> > -	    LANDLOCK_MASK_ACCESS_FS)
> > +	if ((ruleset_attr.handled_access_fs | LANDLOCK_MASK_PUBLIC_ACCESS_FS) !=
> > +	    LANDLOCK_MASK_PUBLIC_ACCESS_FS)
> 
> It would now be possible to add LANDLOCK_ACCESS_FS_IOCTL_GROUP* to a
> rule, which is not part of the API/ABI. I've sent a patch with new tests
> to make sure this is covered:
> https://lore.kernel.org/r/20231120193914.441117-2-mic@digikod.net
> 
> I'll push it in my -next branch if everything is OK before pushing your
> next series. Please review it.

Thanks, good catch!

Looking at add_rule_path_beneath(), it indeed does not look like I have covered
that case in my patch.  I'll put an explicit check for it, like this:

  /*
   * Checks that allowed_access matches the @ruleset constraints and only
   * consists of publicly visible access rights (as opposed to synthetic
   * ones).
   */
  mask = landlock_get_raw_fs_access_mask(ruleset, 0) &
         LANDLOCK_MASK_PUBLIC_ACCESS_FS;
  if ((path_beneath_attr.allowed_access | mask) != mask)
          return -EINVAL;

I assume that the tests that you added were failing?  Or was there an obscure
code path that caught it anyway?

Thanks,
—Günther