Re: [PATCH v6 4/9] landlock: Add IOCTL access right

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

On Thu, Nov 30, 2023 at 10:28:44AM +0100, Mickaël Salaün wrote:
> On Fri, Nov 24, 2023 at 06:30:21PM +0100, Günther Noack wrote:
> > --- a/security/landlock/fs.c
> > +++ b/security/landlock/fs.c
> > @@ -83,6 +86,141 @@ static const struct landlock_object_underops landlock_fs_underops = {
> >  	.release = release_inode
> >  };
> >  
> > +/* IOCTL helpers */
> > +
> > +/*
> > + * These are synthetic access rights, which are only used within the kernel, but
> > + * not exposed to callers in userspace.  The mapping between these access rights
> > + * and IOCTL commands is defined in the required_ioctl_access() helper function.
> > + */
> > +#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)
> > +
> > +/* ioctl_groups - all synthetic access rights for IOCTL command groups */
> > +static const access_mask_t ioctl_groups =
> 
> I find it easier to read and maintain with an ORed right per line, which
> requires clang-format on/off marks.

Done.

I turned this into a #define as well, so that the static_assert() works even in
the GCC9-based PowerPC configuration where the compile previouly failed.  (I
have not reproduced this, but it seems obvious that this is the problem; the old
compiler does not realize yet that a "const int" is constant enough.)


> > +/**
> > + * landlock_expand_access_fs() - Returns @access with the synthetic IOCTL group
> > + * flags enabled if necessary.
> > + *
> > + * @handled: Handled FS access rights.
> > + * @access: FS access rights to expand.
> > + *
> > + * Returns: @access expanded by the necessary flags for the synthetic IOCTL
> > + * access rights.
> > + */
> > +static access_mask_t landlock_expand_access_fs(const access_mask_t handled,
> > +					       const access_mask_t access)
> > +{
> > +	static_assert((ioctl_groups & LANDLOCK_MASK_ACCESS_FS) == ioctl_groups);
> 
> You can move the static_assert() call just after the ioctl_groups
> declaration (contrary to BUILD_BUG_ON() calls which must be in a
> function).

Done.

> > + * Returns: @handled, with the bits for the synthetic IOCTL access rights set,
> > + * if %LANDLOCK_ACCESS_FS_IOCTL is handled
> 
> Missing final dot.

Thanks, added here and also in a few other places where I missed it.

—Günther