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? Thanks, —Günther
