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@xxxxxxxxxxx > > > > 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 >
