Message-ID: <ZxpglWaGWN-BUVHB@google.com>
Date: Thu, 24 Oct 2024 16:58:29 +0200
From: "Günther Noack" <gnoack@...gle.com>
To: "Mickaël Salaün" <mic@...ikod.net>
Cc: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>, 
	Konstantin Meskhidze <konstantin.meskhidze@...wei.com>, Paul Moore <paul@...l-moore.com>, 
	Tahera Fahimi <fahimitahera@...il.com>, linux-kernel@...r.kernel.org, 
	linux-security-module@...r.kernel.org
Subject: Re: [PATCH v3 1/3] landlock: Refactor filesystem access mask management

The approach of only using the union locally to the merging functions is much
nicer. 👍  Still some documentation/wording remarks, but overall looks good.

On Tue, Oct 22, 2024 at 05:11:42PM +0200, Mickaël Salaün wrote:
> Replace get_raw_handled_fs_accesses() with a generic
> landlock_merge_access_masks(), and replace get_fs_domain() with a
> generic landlock_match_ruleset().  These helpers will also be useful for
> other types of access.
> 
> Cc: Günther Noack <gnoack@...gle.com>
> Cc: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
> Signed-off-by: Mickaël Salaün <mic@...ikod.net>
> Link: https://lore.kernel.org/r/20241022151144.872797-2-mic@digikod.net
> ---
> 
> Changes since v2:
> * Create a new type union access_masks_all instead of changing struct
>   acces_masks.
> * Replace get_fs_domain() with an explicit call to
>   landlock_match_ruleset().
> 
> Changes since v1:
> * Rename landlock_filter_access_masks() to landlock_match_ruleset().
> * Rename local variables from domain to ruleset to mach helpers'
>   semantic.  We'll rename and move these helpers when we'll have a
>   dedicated domain struct type.
> * Rename the all_fs mask to any_fs.
> * Add documentation, as suggested by Günther.
> ---
>  security/landlock/fs.c       | 31 ++++------------
>  security/landlock/ruleset.h  | 70 +++++++++++++++++++++++++++++++-----
>  security/landlock/syscalls.c |  2 +-
>  3 files changed, 70 insertions(+), 33 deletions(-)
> 
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index 7d79fc8abe21..dd9a7297ea4e 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -388,38 +388,21 @@ static bool is_nouser_or_private(const struct dentry *dentry)
>  		unlikely(IS_PRIVATE(d_backing_inode(dentry))));
>  }
>  
> -static access_mask_t
> -get_raw_handled_fs_accesses(const struct landlock_ruleset *const domain)
> -{
> -	access_mask_t access_dom = 0;
> -	size_t layer_level;
> -
> -	for (layer_level = 0; layer_level < domain->num_layers; layer_level++)
> -		access_dom |=
> -			landlock_get_raw_fs_access_mask(domain, layer_level);
> -	return access_dom;
> -}
> -
>  static access_mask_t
>  get_handled_fs_accesses(const struct landlock_ruleset *const domain)
>  {
>  	/* Handles all initially denied by default access rights. */
> -	return get_raw_handled_fs_accesses(domain) |
> +	return landlock_merge_access_masks(domain).fs |
>  	       LANDLOCK_ACCESS_FS_INITIALLY_DENIED;
>  }
>  
> -static const struct landlock_ruleset *
> -get_fs_domain(const struct landlock_ruleset *const domain)
> -{
> -	if (!domain || !get_raw_handled_fs_accesses(domain))
> -		return NULL;
> -
> -	return domain;
> -}
> +static const struct access_masks any_fs = {
> +	.fs = ~0,
> +};
>  
>  static const struct landlock_ruleset *get_current_fs_domain(void)
>  {
> -	return get_fs_domain(landlock_get_current_domain());
> +	return landlock_match_ruleset(landlock_get_current_domain(), any_fs);
>  }
>  
>  /*
> @@ -1516,8 +1499,8 @@ 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,
>  		optional_access;
> -	const struct landlock_ruleset *const dom =
> -		get_fs_domain(landlock_cred(file->f_cred)->domain);
> +	const struct landlock_ruleset *const dom = landlock_match_ruleset(
> +		landlock_cred(file->f_cred)->domain, any_fs);
>  
>  	if (!dom)
>  		return 0;
> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
> index 61bdbc550172..e00edcb38c5b 100644
> --- a/security/landlock/ruleset.h
> +++ b/security/landlock/ruleset.h
> @@ -47,6 +47,15 @@ struct access_masks {
>  	access_mask_t scope : LANDLOCK_NUM_SCOPE;
>  };
>  
> +union access_masks_all {
> +	struct access_masks masks;
> +	u32 all;
> +};
> +
> +/* Makes sure all fields are covered. */
> +static_assert(sizeof(((union access_masks_all *)NULL)->masks) ==
> +	      sizeof(((union access_masks_all *)NULL)->all));

Nit: Could maybe be written as
sizeof(typeof_member(union access_masks_all, masks))

Nit 2: Should this be <= instead of ==?

> +
>  typedef u16 layer_mask_t;
>  /* Makes sure all layers can be checked. */
>  static_assert(BITS_PER_TYPE(layer_mask_t) >= LANDLOCK_MAX_NUM_LAYERS);
> @@ -260,6 +269,58 @@ static inline void landlock_get_ruleset(struct landlock_ruleset *const ruleset)
>  		refcount_inc(&ruleset->usage);
>  }
>  
> +/**
> + * landlock_merge_access_masks - Return the merge of a ruleset's access_masks

Documentation uses the same words as in the function name, and it's not
intuitively clear to me what "merge" means.  Would it be fair to describe it
like this:

  landlock_merge_access_masks - Return all access rights handled in the ruleset

?

(To describe mathematical set operations, I'd normally say "a union" instead of
"a merge", but that might be confusing given that we also use the C "union"
feature in the same function.)

> + *
> + * @ruleset: Landlock ruleset (used as a domain)
> + *
> + * Returns: an access_masks result of the OR of all the ruleset's access masks.
> + */
> +static inline struct access_masks
> +landlock_merge_access_masks(const struct landlock_ruleset *const ruleset)
> +{
> +	union access_masks_all matches = {};
> +	size_t layer_level;
> +
> +	for (layer_level = 0; layer_level < ruleset->num_layers;
> +	     layer_level++) {
> +		union access_masks_all layer = {
> +			.masks = ruleset->access_masks[layer_level],
> +		};
> +
> +		matches.all |= layer.all;
> +	}
> +
> +	return matches.masks;
> +}
> +
> +/**
> + * landlock_match_ruleset - Return @ruleset if any @masks right matches

Same here; I think when I see a call for a function called
"landlock_match_ruleset" I might be confused about what is being matched against
what here.  Documentation uses the same wording as well.  Documentation
suggestion:

  landlock_match_ruleset - Return @ruleset iff any access right in @masks
                           is handled in the @ruleset.

This is why in [1], I suggested that this function could return a boolean
and be called differently, like:

  /* True if any access right in @masks is handled in @ruleset. */
  bool landlock_is_any_access_right_handled(
  	const struct landlock_ruleset *const ruleset,
  	struct access_masks masks);

Returning a boolean removes the (slightly unintuitive) semantics where a
function argument is returned under certain conditions, which are not clear from
the function name, and then the function has the more conventional style of
returning a boolean that indicates whether some condition holds.  The function
name would spell out more exactly what is matched against what.

Callers would have to check the boolean and return the ruleset themselves, but
this seems like a reasonable thing to do when the code is clearer to read, IMHO.

  if (landlock_is_any_access_right_handled(ruleset, masks))
  	return ruleset;
  return NULL;

Alternatively, how about wording it like this:

  /*
   * landlock_get_applicable_domain - Returns the @dom ruleset if it
   *                                  applies to (handles) the access
   *                                  rights specified in @masks.
   */
  const struct landlock_ruleset *landlock_get_applicable_domain(
  	const struct landlock_ruleset *const dom,
  	const struct access_masks masks);

[1] https://lore.kernel.org/all/20241005.a69458234f74@gnoack.org/


> + *
> + * @ruleset: Landlock ruleset (used as a domain)
> + * @masks: access masks
> + *
> + * Returns: @ruleset if @masks matches, or NULL otherwise.
> + */
> +static inline const struct landlock_ruleset *
> +landlock_match_ruleset(const struct landlock_ruleset *const ruleset,
> +		       const struct access_masks masks)
> +{
> +	const union access_masks_all masks_all = {
> +		.masks = masks,
> +	};
> +	union access_masks_all merge = {};
> +
> +	if (!ruleset)
> +		return NULL;
> +
> +	merge.masks = landlock_merge_access_masks(ruleset);
> +	if (merge.all & masks_all.all)
> +		return ruleset;
> +
> +	return NULL;
> +}
> +
>  static inline void
>  landlock_add_fs_access_mask(struct landlock_ruleset *const ruleset,
>  			    const access_mask_t fs_access_mask,
> @@ -295,19 +356,12 @@ landlock_add_scope_mask(struct landlock_ruleset *const ruleset,
>  	ruleset->access_masks[layer_level].scope |= 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].fs;
> -}
> -
>  static inline access_mask_t
>  landlock_get_fs_access_mask(const struct landlock_ruleset *const ruleset,
>  			    const u16 layer_level)
>  {
>  	/* Handles all initially denied by default access rights. */
> -	return landlock_get_raw_fs_access_mask(ruleset, layer_level) |
> +	return ruleset->access_masks[layer_level].fs |
>  	       LANDLOCK_ACCESS_FS_INITIALLY_DENIED;
>  }
>  
> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> index f5a0e7182ec0..c097d356fa45 100644
> --- a/security/landlock/syscalls.c
> +++ b/security/landlock/syscalls.c
> @@ -329,7 +329,7 @@ static int add_rule_path_beneath(struct landlock_ruleset *const ruleset,
>  		return -ENOMSG;
>  
>  	/* Checks that allowed_access matches the @ruleset constraints. */
> -	mask = landlock_get_raw_fs_access_mask(ruleset, 0);
> +	mask = ruleset->access_masks[0].fs;
>  	if ((path_beneath_attr.allowed_access | mask) != mask)
>  		return -EINVAL;
>  
> -- 
> 2.47.0
> 

Powered by blists - more mailing lists