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

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

Hi!

On Thu, Nov 16, 2023 at 04:50:56PM -0500, Mickaël Salaün wrote:
> On Fri, Nov 03, 2023 at 04:57:12PM +0100, Günther Noack wrote:
> > diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> > index 25c8d7677539..6d41c059e910 100644
> > --- a/include/uapi/linux/landlock.h
> > +++ b/include/uapi/linux/landlock.h
> > @@ -198,13 +199,26 @@ struct landlock_net_port_attr {
> >   *   If multiple requirements are not met, the ``EACCES`` error code takes
> >   *   precedence over ``EXDEV``.
> >   *
> > + * The following access right applies both to files and directories:
> > + *
> > + * - %LANDLOCK_ACCESS_FS_IOCTL: Invoke :manpage:`ioctl(2)` commands on an opened
> > + *   file or directory.
> > + *
> > + *   This access right applies to all :manpage:`ioctl(2)` commands, except of
> > + *   ``FIOCLEX``, ``FIONCLEX``, ``FIONBIO``, ``FIOASYNC`` and ``FIONREAD``.
> > + *   These commands continue to be invokable independent of the
> > + *   %LANDLOCK_ACCESS_FS_IOCTL access right.
> > + *
> > + *   This access right is available since the fourth version of the Landlock
> 
> It is now the fifth version. Same for the documentation.

Thanks!

I've accidentally put the last update to this docstring into the "Documentation"
commit.  I'll rebase that so that it becomes part of the commit that touches the
implementation and the header.  The version is fixed there.


> > diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> > index bc7c126deea2..aa54970c235f 100644
> > --- a/security/landlock/fs.c
> > +++ b/security/landlock/fs.c
> > @@ -1123,7 +1130,9 @@ static int hook_file_open(struct file *const file)
> >  {
> >  	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
> >  	access_mask_t open_access_request, full_access_request, allowed_access;
> > -	const access_mask_t optional_access = LANDLOCK_ACCESS_FS_TRUNCATE;
> > +	const access_mask_t optional_access =
> > +		LANDLOCK_ACCESS_FS_TRUNCATE | LANDLOCK_ACCESS_FS_IOCTL |
> > +		IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G3 | IOCTL_CMD_G4;
> 
> I think it would be more future-proof to declare a new global const and
> use it here for optional_access:
> 
> static const access_mask_t ioctl_groups =
> 	IOCTL_CMD_G1 |
> 	IOCTL_CMD_G2 |
> 	IOCTL_CMD_G3 |
> 	IOCTL_CMD_G4;

Done.


> > +static access_mask_t required_ioctl_access(unsigned int cmd)
> > +{
> > +	switch (cmd) {
> > +	case FIOQSIZE:
> > +		return IOCTL_CMD_G1;
> > +	case FS_IOC_FIEMAP:
> > +	case FIBMAP:
> > +	case FIGETBSZ:
> > +		return IOCTL_CMD_G2;
> > +	case FIONREAD:
> > +	case FIDEDUPERANGE:
> > +		return IOCTL_CMD_G3;
> > +	case FICLONE:
> > +	case FICLONERANGE:
> > +	case FS_IOC_RESVSP:
> > +	case FS_IOC_RESVSP64:
> > +	case FS_IOC_UNRESVSP:
> > +	case FS_IOC_UNRESVSP64:
> > +	case FS_IOC_ZERO_RANGE:
> > +		return IOCTL_CMD_G4;
> > +	case FIOCLEX:
> > +	case FIONCLEX:
> > +	case FIONBIO:
> > +	case FIOASYNC:
> > +		/*
> > +		 * FIOCLEX, FIONCLEX, FIONBIO and FIOASYNC manipulate the FD's
> > +		 * close-on-exec and the file's buffered-IO and async flags.
> > +		 * These operations are also available through fcntl(2),
> > +		 * and are unconditionally permitted in Landlock.
> > +		 */
> > +		return 0;
> 
> You can move this block at the top.

Done.


> > +	default:
> > +		/*
> > +		 * Other commands are guarded by the catch-all access right.
> > +		 */
> > +		return LANDLOCK_ACCESS_FS_IOCTL;
> > +	}
> > +}
> > +
> > +static int hook_file_ioctl(struct file *file, unsigned int cmd,
> > +			   unsigned long arg)
> > +{
> > +	access_mask_t required_access = required_ioctl_access(cmd);
> > +	access_mask_t allowed_access = landlock_file(file)->allowed_access;
> 
> You can use const for these variables.

Done.


> > +
> > +	/*
> > +	 * It is the access rights at the time of opening the file which
> > +	 * determine whether ioctl can be used on the opened file later.
> > +	 *
> > +	 * The access right is attached to the opened file in hook_file_open().
> > +	 */
> > +	if ((allowed_access & required_access) == required_access)
> > +		return 0;
> 
> Please add a new line.

Done.


> > +	return -EACCES;
> > +}
> > +
> >  static struct security_hook_list landlock_hooks[] __ro_after_init = {
> >  	LSM_HOOK_INIT(inode_free_security, hook_inode_free_security),
> >  
> > @@ -1218,6 +1283,7 @@ static struct security_hook_list landlock_hooks[] __ro_after_init = {
> >  	LSM_HOOK_INIT(file_alloc_security, hook_file_alloc_security),
> >  	LSM_HOOK_INIT(file_open, hook_file_open),
> >  	LSM_HOOK_INIT(file_truncate, hook_file_truncate),
> > +	LSM_HOOK_INIT(file_ioctl, hook_file_ioctl),
> >  };
> >  
> >  __init void landlock_add_fs_hooks(void)
> > diff --git a/security/landlock/limits.h b/security/landlock/limits.h
> > index 93c9c6f91556..d0a95169ba3f 100644
> > --- a/security/landlock/limits.h
> > +++ b/security/landlock/limits.h
> > @@ -18,7 +18,15 @@
> >  #define LANDLOCK_MAX_NUM_LAYERS		16
> >  #define LANDLOCK_MAX_NUM_RULES		U32_MAX
> >  
> > -#define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_TRUNCATE
> > +#define LANDLOCK_LAST_PUBLIC_ACCESS_FS	LANDLOCK_ACCESS_FS_IOCTL
> > +#define LANDLOCK_MASK_PUBLIC_ACCESS_FS	((LANDLOCK_LAST_PUBLIC_ACCESS_FS << 1) - 1)
> > +
> 
> Please add a comment to explain that the semantic of these synthetic
> access rights is defined by the required_ioctl_access() helper.

Done.


> > +#define IOCTL_CMD_G1			(LANDLOCK_LAST_PUBLIC_ACCESS_FS << 1)
> > +#define IOCTL_CMD_G2			(LANDLOCK_LAST_PUBLIC_ACCESS_FS << 2)
> > +#define IOCTL_CMD_G3			(LANDLOCK_LAST_PUBLIC_ACCESS_FS << 3)
> > +#define IOCTL_CMD_G4			(LANDLOCK_LAST_PUBLIC_ACCESS_FS << 4)
> > +
> > +#define LANDLOCK_LAST_ACCESS_FS		IOCTL_CMD_G4
> >  #define LANDLOCK_MASK_ACCESS_FS		((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
> >  #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
> >  #define LANDLOCK_SHIFT_ACCESS_FS	0
> > diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
> > index c7f1526784fd..58d96aff3980 100644
> > --- a/security/landlock/ruleset.h
> > +++ b/security/landlock/ruleset.h
> > @@ -30,7 +30,7 @@
> >  	LANDLOCK_ACCESS_FS_REFER)
> >  /* clang-format on */
> >  
> > -typedef u16 access_mask_t;
> > +typedef u32 access_mask_t;
> >  /* Makes sure all filesystem access rights can be stored. */
> >  static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS);
> >  /* Makes sure all network access rights can be stored. */
> > @@ -256,6 +256,54 @@ static inline void landlock_get_ruleset(struct landlock_ruleset *const ruleset)
> >  		refcount_inc(&ruleset->usage);
> >  }
> >  
> > +/**
> > + * expand_ioctl - return the dst flags from either the src flag or the
> 
> Return...

Done.


> > + * LANDLOCK_ACCESS_FS_IOCTL flag, depending on whether the
> > + * LANDLOCK_ACCESS_FS_IOCTL and src access rights are handled or not.
> > + *
> > + * @handled: Handled access rights
> > + * @access:  The access mask to copy values from
> > + * @src:     A single access right to copy from in @access.
> > + * @dst:     One or more access rights to copy to
> 
> Please don't add extra spaces, that would avoid to shift all the lines
> if we get another name which is longer.

Done.


> > + *
> > + * Returns:
> > + * @dst, or 0
> > + */
> > +static inline access_mask_t expand_ioctl(access_mask_t handled,
> > +					 access_mask_t access,
> > +					 access_mask_t src, access_mask_t dst)
> > +{
> > +	if (!(handled & LANDLOCK_ACCESS_FS_IOCTL))
> > +		return 0;
> > +
> > +	access_mask_t copy_from = (handled & src) ? src :
> > +						    LANDLOCK_ACCESS_FS_IOCTL;
> > +	if (access & copy_from)
> > +		return dst;
> 
> Please add a newline after return.

Done.


> > +	return 0;
> > +}
> > +
> > +/**
> > + * Returns @access with the synthetic IOCTL group flags enabled if necessary.
> 
> I think htmldocs will print a warning with this format.

You are absolutely right, I got a warning about that. %-)  Fixed.


—Günther