[PATCH v3 0/3] Refactor Landlock access mask management

[PATCH v3 0/3] Refactor Landlock access mask management

[Mickaël Salaün]

Hi,

To simplify code for new access types [1], add 2 new helpers:
- landlock_merge_access_masks()
- landlock_match_ruleset()

This third version mainly use a new union access_masks_all type instead
of changing struct access_masks.

The last patch uses these helpers to optimize Landlock scope management
like with filesystem and network access checks.

[1] https://lore.kernel.org/r/3433b163-2371-e679-cc8a-e540a0218bca@huawei-partners.com

Previous version:
v2: https://lore.kernel.org/r/20241014124835.1152246-1-mic@digikod.net
v1: https://lore.kernel.org/r/20241001141234.397649-1-mic@digikod.net

Regards,

Mickaël Salaün (3):
  landlock: Refactor filesystem access mask management
  landlock: Refactor network access mask management
  landlock: Optimize scope enforcement

 security/landlock/fs.c       | 31 ++++------------
 security/landlock/net.c      | 27 +++-----------
 security/landlock/ruleset.h  | 70 +++++++++++++++++++++++++++++++-----
 security/landlock/syscalls.c |  2 +-
 security/landlock/task.c     | 20 ++++++++---
 5 files changed, 90 insertions(+), 60 deletions(-)


base-commit: dad2f20715163e80aab284fb092efc8c18bf97c7
-- 
2.47.0

[PATCH v3 1/3] landlock: Refactor filesystem access mask management

[Mickaël Salaün]

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@google.com>
Cc: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
Signed-off-by: Mickaël Salaün <mic@digikod.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));
+
 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
+ *
+ * @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
+ *
+ * @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

[PATCH v3 2/3] landlock: Refactor network access mask management

[Mickaël Salaün]

Replace get_raw_handled_net_accesses() and get_current_net_domain() with
a call to landlock_match_ruleset().

Cc: Günther Noack <gnoack@google.com>
Cc: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
Cc: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20241022151144.872797-3-mic@digikod.net
---

Changes since v2:
* Replace get_current_net_domain() with explicit call to
  landlock_match_ruleset().

Changes since v1:
* Rename the all_net mask to any_net.
---
 security/landlock/net.c | 27 +++++----------------------
 1 file changed, 5 insertions(+), 22 deletions(-)

diff --git a/security/landlock/net.c b/security/landlock/net.c
index c8bcd29bde09..27872d0f7e11 100644
--- a/security/landlock/net.c
+++ b/security/landlock/net.c
@@ -39,27 +39,9 @@ int landlock_append_net_rule(struct landlock_ruleset *const ruleset,
 	return err;
 }
 
-static access_mask_t
-get_raw_handled_net_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_net_access_mask(domain, layer_level);
-	return access_dom;
-}
-
-static const struct landlock_ruleset *get_current_net_domain(void)
-{
-	const struct landlock_ruleset *const dom =
-		landlock_get_current_domain();
-
-	if (!dom || !get_raw_handled_net_accesses(dom))
-		return NULL;
-
-	return dom;
-}
+static const struct access_masks any_net = {
+	.net = ~0,
+};
 
 static int current_check_access_socket(struct socket *const sock,
 				       struct sockaddr *const address,
@@ -72,7 +54,8 @@ static int current_check_access_socket(struct socket *const sock,
 	struct landlock_id id = {
 		.type = LANDLOCK_KEY_NET_PORT,
 	};
-	const struct landlock_ruleset *const dom = get_current_net_domain();
+	const struct landlock_ruleset *const dom =
+		landlock_match_ruleset(landlock_get_current_domain(), any_net);
 
 	if (!dom)
 		return 0;
-- 
2.47.0

[PATCH v3 3/3] landlock: Optimize scope enforcement

[Mickaël Salaün]

Do not walk through the domain hierarchy when the required scope is not
supported by this domain.  This is the same approach as for filesystem
and network restrictions.

Cc: Günther Noack <gnoack@google.com>
Cc: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
Cc: Tahera Fahimi <fahimitahera@gmail.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20241022151144.872797-4-mic@digikod.net
---

Changes since v2:
* Make the unix_scope variable global to the file and remove
  previous get_current_unix_scope_domain().
---
 security/landlock/task.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/security/landlock/task.c b/security/landlock/task.c
index 4acbd7c40eee..e7f45af87ff5 100644
--- a/security/landlock/task.c
+++ b/security/landlock/task.c
@@ -204,12 +204,16 @@ static bool is_abstract_socket(struct sock *const sock)
 	return false;
 }
 
+static const struct access_masks unix_scope = {
+	.scope = LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET,
+};
+
 static int hook_unix_stream_connect(struct sock *const sock,
 				    struct sock *const other,
 				    struct sock *const newsk)
 {
-	const struct landlock_ruleset *const dom =
-		landlock_get_current_domain();
+	const struct landlock_ruleset *const dom = landlock_match_ruleset(
+		landlock_get_current_domain(), unix_scope);
 
 	/* Quick return for non-landlocked tasks. */
 	if (!dom)
@@ -224,8 +228,8 @@ static int hook_unix_stream_connect(struct sock *const sock,
 static int hook_unix_may_send(struct socket *const sock,
 			      struct socket *const other)
 {
-	const struct landlock_ruleset *const dom =
-		landlock_get_current_domain();
+	const struct landlock_ruleset *const dom = landlock_match_ruleset(
+		landlock_get_current_domain(), unix_scope);
 
 	if (!dom)
 		return 0;
@@ -243,6 +247,10 @@ static int hook_unix_may_send(struct socket *const sock,
 	return 0;
 }
 
+static const struct access_masks signal_scope = {
+	.scope = LANDLOCK_SCOPE_SIGNAL,
+};
+
 static int hook_task_kill(struct task_struct *const p,
 			  struct kernel_siginfo *const info, const int sig,
 			  const struct cred *const cred)
@@ -256,6 +264,7 @@ static int hook_task_kill(struct task_struct *const p,
 	} else {
 		dom = landlock_get_current_domain();
 	}
+	dom = landlock_match_ruleset(dom, signal_scope);
 
 	/* Quick return for non-landlocked tasks. */
 	if (!dom)
@@ -279,7 +288,8 @@ static int hook_file_send_sigiotask(struct task_struct *tsk,
 
 	/* Lock already held by send_sigio() and send_sigurg(). */
 	lockdep_assert_held(&fown->lock);
-	dom = landlock_file(fown->file)->fown_domain;
+	dom = landlock_match_ruleset(landlock_file(fown->file)->fown_domain,
+				     signal_scope);
 
 	/* Quick return for unowned socket. */
 	if (!dom)
-- 
2.47.0

Re: [PATCH v3 1/3] landlock: Refactor filesystem access mask management

[Günther Noack]

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@google.com>
> Cc: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> Signed-off-by: Mickaël Salaün <mic@digikod.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
> 

Re: [PATCH v3 1/3] landlock: Refactor filesystem access mask management

[Mickaël Salaün]

On Thu, Oct 24, 2024 at 04:58:29PM +0200, Günther Noack wrote:
> 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@google.com>
> > Cc: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> > Signed-off-by: Mickaël Salaün <mic@digikod.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))

yep

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

This would be correct, but I prefer to be stricter to catch any
potential future change.

> 
> > +
> >  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
> 
> ?

Sounds good.

> 
> (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.)

landlock_union_access_masks() looks good to me.

> 
> > + *
> > + * @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.

I get your point but I prefer an helper that limits code verbosity and
potential misuse (which is subjective, but still).  With
landlock_match_ruleset(), I think it's easier to check that this
function is called whenever necessary to "match" access masks.  This
pattern is then used in almost every hook implementations.

> 
> 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;

I understand but I prefer to simplify future contributions.

> 
> 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);

OK, I'll go with that.

> 
> [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
> >