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

["Mickaël Salaün" <mic@digikod.net>]

On Fri, Nov 24, 2023 at 04:39:02PM +0100, Günther Noack wrote:
> 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?

Yep, it was failing, but it works with the v6.

> 
> Thanks,
> —Günther
>