Re: [PATCH] landlock: Use bit-fields for storing handled layer access masks

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

Great!  Looking at the generated data structures with pahole, it doesn't
increase the whole size, and it should be fine with other (small) fields
too.

With this new struct, we don't need the landlock_get_* helpers anymore.
We might want to keep the landlock_add_*() helpers as safeguards
(because of the WARN_ON_ONCE) though.

On Mon, Jun 10, 2024 at 08:21:15AM +0000, Günther Noack wrote:
> When defined using bit-fields, the compiler takes care of packing the
> bits in a memory-efficient way and frees us from defining
> LANDLOCK_SHIFT_ACCESS_* by hand.  The exact memory layout does not
> matter in our use case.
> 
> The manual definition of LANDLOCK_SHIFT_ACCESS_* has resulted in bugs
> in at least two recent patch sets where new kinds of handled access
> rights were introduced.
> 
> Cc: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> Cc: Tahera Fahimi <fahimitahera@gmail.com>
> Link: https://lore.kernel.org/all/ebd680cc-25d6-ee14-4856-310f5e5e28e4@huawei-partners.com/
> Link: https://lore.kernel.org/all/ZmLEoBfHyUR3nKAV@google.com/

Please add [1] and [2] at the end of each link to reference them in the
commit message.

> Signed-off-by: Günther Noack <gnoack@google.com>
> ---
>  security/landlock/limits.h  |  2 --
>  security/landlock/ruleset.c |  4 ----
>  security/landlock/ruleset.h | 24 +++++++++---------------
>  3 files changed, 9 insertions(+), 21 deletions(-)
> 
> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
> index 20fdb5ff3514..4eb643077a2a 100644
> --- a/security/landlock/limits.h
> +++ b/security/landlock/limits.h
> @@ -21,12 +21,10 @@
>  #define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_IOCTL_DEV
>  #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
>  
>  #define LANDLOCK_LAST_ACCESS_NET	LANDLOCK_ACCESS_NET_CONNECT_TCP
>  #define LANDLOCK_MASK_ACCESS_NET	((LANDLOCK_LAST_ACCESS_NET << 1) - 1)
>  #define LANDLOCK_NUM_ACCESS_NET		__const_hweight64(LANDLOCK_MASK_ACCESS_NET)
> -#define LANDLOCK_SHIFT_ACCESS_NET	LANDLOCK_NUM_ACCESS_FS
>  
>  /* clang-format on */
>  
> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
> index e0a5fbf9201a..6ff232f58618 100644
> --- a/security/landlock/ruleset.c
> +++ b/security/landlock/ruleset.c
> @@ -169,13 +169,9 @@ static void build_check_ruleset(void)
>  		.num_rules = ~0,
>  		.num_layers = ~0,
>  	};
> -	typeof(ruleset.access_masks[0]) access_masks = ~0;
>  
>  	BUILD_BUG_ON(ruleset.num_rules < LANDLOCK_MAX_NUM_RULES);
>  	BUILD_BUG_ON(ruleset.num_layers < LANDLOCK_MAX_NUM_LAYERS);
> -	BUILD_BUG_ON(access_masks <
> -		     ((LANDLOCK_MASK_ACCESS_FS << LANDLOCK_SHIFT_ACCESS_FS) |
> -		      (LANDLOCK_MASK_ACCESS_NET << LANDLOCK_SHIFT_ACCESS_NET)));
>  }
>  
>  /**
> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
> index c7f1526784fd..0f1b5b4c8f6b 100644
> --- a/security/landlock/ruleset.h
> +++ b/security/landlock/ruleset.h
> @@ -39,10 +39,10 @@ static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_NET);
>  static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
>  
>  /* Ruleset access masks. */
> -typedef u32 access_masks_t;
> -/* Makes sure all ruleset access rights can be stored. */
> -static_assert(BITS_PER_TYPE(access_masks_t) >=
> -	      LANDLOCK_NUM_ACCESS_FS + LANDLOCK_NUM_ACCESS_NET);
> +struct access_masks {
> +	access_mask_t fs : LANDLOCK_NUM_ACCESS_FS;
> +	access_mask_t net : LANDLOCK_NUM_ACCESS_NET;
> +};
>  
>  typedef u16 layer_mask_t;
>  /* Makes sure all layers can be checked. */
> @@ -226,7 +226,7 @@ struct landlock_ruleset {
>  			 * layers are set once and never changed for the
>  			 * lifetime of the ruleset.
>  			 */
> -			access_masks_t access_masks[];
> +			struct access_masks access_masks[];
>  		};
>  	};
>  };
> @@ -265,8 +265,7 @@ landlock_add_fs_access_mask(struct landlock_ruleset *const ruleset,
>  
>  	/* Should already be checked in sys_landlock_create_ruleset(). */
>  	WARN_ON_ONCE(fs_access_mask != fs_mask);
> -	ruleset->access_masks[layer_level] |=
> -		(fs_mask << LANDLOCK_SHIFT_ACCESS_FS);
> +	ruleset->access_masks[layer_level].fs |= fs_mask;
>  }
>  
>  static inline void
> @@ -278,17 +277,14 @@ landlock_add_net_access_mask(struct landlock_ruleset *const ruleset,
>  
>  	/* Should already be checked in sys_landlock_create_ruleset(). */
>  	WARN_ON_ONCE(net_access_mask != net_mask);
> -	ruleset->access_masks[layer_level] |=
> -		(net_mask << LANDLOCK_SHIFT_ACCESS_NET);
> +	ruleset->access_masks[layer_level].net |= net_mask;
>  }
>  
>  static inline access_mask_t
>  landlock_get_raw_fs_access_mask(const struct landlock_ruleset *const ruleset,
>  				const u16 layer_level)
>  {
> -	return (ruleset->access_masks[layer_level] >>
> -		LANDLOCK_SHIFT_ACCESS_FS) &
> -	       LANDLOCK_MASK_ACCESS_FS;
> +	return ruleset->access_masks[layer_level].fs;
>  }
>  
>  static inline access_mask_t
> @@ -304,9 +300,7 @@ static inline access_mask_t
>  landlock_get_net_access_mask(const struct landlock_ruleset *const ruleset,
>  			     const u16 layer_level)
>  {
> -	return (ruleset->access_masks[layer_level] >>
> -		LANDLOCK_SHIFT_ACCESS_NET) &
> -	       LANDLOCK_MASK_ACCESS_NET;
> +	return ruleset->access_masks[layer_level].net;
>  }
>  
>  bool landlock_unmask_layers(const struct landlock_rule *const rule,
> -- 
> 2.45.2.505.gda0bf45e8d-goog
> 
>