Re: [RFC PATCH 2/9] Refactor per-layer information in rulesets and rules

Tingmao Wang <m@xxxxxxxxxx> · Thu, 6 Mar 2025 02:58:01 +0000

On 3/4/25 19:49, Mickaël Salaün wrote:

We could indeed have a pointer in the  landlock_hierarchy and have a
dedicated bit in each layer's access_masks to indicate that this layer
is supervised.  This should simplify the whole patch series.

That seems sensible.  I did consider using the landlock_hierarchy, but 
chose the current way as it initially seemed more sensible, but on 
second thought this means that we have to carefully increment all the 
refcounts on domain merge etc.  On the other hand storing the supervisor 
pointer in the hierarchy, if we have an extra bit in struct access_masks 
then we can quickly determine if supervisors are involved without 
effectively walking a linked list, which is nice.

Actually, just to check, is the reason why we have the access_masks FAM 
in the ruleset purely for performance? Initially I wasn't sure if each 
layer correspond 1-to-1 with landlock_hierarchy, since otherwise it 
seemed to me you could just put the access mask in the hierarchy too.
In other words, is it right to assume that, if a domain has 3 layers, 
for example, then domain->hierarchy correspond to the third layer, 
domain->hierarchy->parent correspond to the second, and
d->h->parent->parent would be the first layer's hierarchy?



This patch doesn't make any functional changes nor add any
supervise specific stuff.  It is purely to pave the way for
future patches.

Signed-off-by: Tingmao Wang <m@xxxxxxxxxx>
---
  security/landlock/ruleset.c  | 29 +++++++++---------
  security/landlock/ruleset.h  | 59 ++++++++++++++++++++++--------------
  security/landlock/syscalls.c |  2 +-
  3 files changed, 52 insertions(+), 38 deletions(-)

diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index 69742467a0cf..2cc6f7c5eb1b 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -31,9 +31,8 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
  {
  	struct landlock_ruleset *new_ruleset;
  
-	new_ruleset =
-		kzalloc(struct_size(new_ruleset, access_masks, num_layers),
-			GFP_KERNEL_ACCOUNT);
+	new_ruleset = kzalloc(struct_size(new_ruleset, layer_stack, num_layers),
+			      GFP_KERNEL_ACCOUNT);
  	if (!new_ruleset)
  		return ERR_PTR(-ENOMEM);
  	refcount_set(&new_ruleset->usage, 1);
@@ -104,8 +103,9 @@ static bool is_object_pointer(const enum landlock_key_type key_type)
  
  static struct landlock_rule *
  create_rule(const struct landlock_id id,
-	    const struct landlock_layer (*const layers)[], const u32 num_layers,
-	    const struct landlock_layer *const new_layer)
+	    const struct landlock_rule_layer (*const layers)[],
+	    const u32 num_layers,
+	    const struct landlock_rule_layer *const new_layer)
  {
  	struct landlock_rule *new_rule;
  	u32 new_num_layers;
@@ -201,7 +201,7 @@ static void build_check_ruleset(void)
   */
  static int insert_rule(struct landlock_ruleset *const ruleset,
  		       const struct landlock_id id,
-		       const struct landlock_layer (*const layers)[],
+		       const struct landlock_rule_layer (*const layers)[],
  		       const size_t num_layers)
  {
  	struct rb_node **walker_node;
@@ -284,7 +284,7 @@ static int insert_rule(struct landlock_ruleset *const ruleset,
  
  static void build_check_layer(void)
  {
-	const struct landlock_layer layer = {
+	const struct landlock_rule_layer layer = {

It's not useful to rename this struct.

  		.level = ~0,
  		.access = ~0,
  	};
@@ -299,7 +299,7 @@ int landlock_insert_rule(struct landlock_ruleset *const ruleset,
  			 const struct landlock_id id,
  			 const access_mask_t access)
  {
-	struct landlock_layer layers[] = { {
+	struct landlock_rule_layer layers[] = { {
  		.access = access,
  		/* When @level is zero, insert_rule() extends @ruleset. */
  		.level = 0,
@@ -344,7 +344,7 @@ static int merge_tree(struct landlock_ruleset *const dst,
  	/* Merges the @src tree. */
  	rbtree_postorder_for_each_entry_safe(walker_rule, next_rule, src_root,
  					     node) {
-		struct landlock_layer layers[] = { {
+		struct landlock_rule_layer layers[] = { {
  			.level = dst->num_layers,
  		} };
  		const struct landlock_id id = {
@@ -389,8 +389,9 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
  		err = -EINVAL;
  		goto out_unlock;
  	}
-	dst->access_masks[dst->num_layers - 1] =
-		landlock_upgrade_handled_access_masks(src->access_masks[0]);
+	dst->layer_stack[dst->num_layers - 1].access_masks =
+		landlock_upgrade_handled_access_masks(
+			src->layer_stack[0].access_masks);
  
  	/* Merges the @src inode tree. */
  	err = merge_tree(dst, src, LANDLOCK_KEY_INODE);
@@ -472,8 +473,8 @@ static int inherit_ruleset(struct landlock_ruleset *const parent,
  		goto out_unlock;
  	}
  	/* Copies the parent layer stack and leaves a space for the new layer. */
-	memcpy(child->access_masks, parent->access_masks,
-	       flex_array_size(parent, access_masks, parent->num_layers));
+	memcpy(child->layer_stack, parent->layer_stack,
+	       flex_array_size(parent, layer_stack, parent->num_layers));
  
  	if (WARN_ON_ONCE(!parent->hierarchy)) {
  		err = -EINVAL;
@@ -644,7 +645,7 @@ bool landlock_unmask_layers(const struct landlock_rule *const rule,
  	 * E.g. /a/b <execute> + /a <read> => /a/b <execute + read>
  	 */
  	for (layer_level = 0; layer_level < rule->num_layers; layer_level++) {
-		const struct landlock_layer *const layer =
+		const struct landlock_rule_layer *const layer =
  			&rule->layers[layer_level];
  		const layer_mask_t layer_bit = BIT_ULL(layer->level - 1);
  		const unsigned long access_req = access_request;
diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
index 52f4f0af6ab0..a2605959f733 100644
--- a/security/landlock/ruleset.h
+++ b/security/landlock/ruleset.h
@@ -21,9 +21,10 @@
  #include "object.h"
  
  /**
- * struct landlock_layer - Access rights for a given layer
+ * struct landlock_rule_layer - Stores the access rights for a
+ * given layer in a rule.
   */
-struct landlock_layer {
+struct landlock_rule_layer {
  	/**
  	 * @level: Position of this layer in the layer stack.
  	 */
@@ -102,10 +103,11 @@ struct landlock_rule {
  	 */
  	u32 num_layers;
  	/**
-	 * @layers: Stack of layers, from the latest to the newest, implemented
-	 * as a flexible array member (FAM).
+	 * @layers: Stack of layers, from the latest to the newest,
+	 * implemented as a flexible array member (FAM). Only
+	 * contains layers that has a rule for this object.
  	 */
-	struct landlock_layer layers[] __counted_by(num_layers);
+	struct landlock_rule_layer layers[] __counted_by(num_layers);
  };
  
  /**
@@ -124,6 +126,18 @@ struct landlock_hierarchy {
  	refcount_t usage;
  };
  
+/**
+ * struct landlock_ruleset_layer - Store per-layer information
+ * within a domain (or a non-merged ruleset)
+ */
+struct landlock_ruleset_layer {
+	/**
+	 * @access_masks: Contains the subset of filesystem and
+	 * network actions that are restricted by a layer.
+	 */
+	struct access_masks access_masks;
+};
+
  /**
   * struct landlock_ruleset - Landlock ruleset
   *
@@ -187,18 +201,17 @@ struct landlock_ruleset {
  			 */
  			u32 num_layers;
  			/**
-			 * @access_masks: Contains the subset of filesystem and
-			 * network actions that are restricted by a ruleset.
-			 * A domain saves all layers of merged rulesets in a
-			 * stack (FAM), starting from the first layer to the
-			 * last one.  These layers are used when merging
-			 * rulesets, for user space backward compatibility
-			 * (i.e. future-proof), and to properly handle merged
-			 * rulesets without overlapping access rights.  These
-			 * layers are set once and never changed for the
-			 * lifetime of the ruleset.
+			 * @layer_stack: A domain saves all layers of merged
+			 * rulesets in a stack (FAM), starting from the first
+			 * layer to the last one.  These layers are used when
+			 * merging rulesets, for user space backward
+			 * compatibility (i.e. future-proof), and to properly
+			 * handle merged rulesets without overlapping access
+			 * rights.  These layers are set once and never
+			 * changed for the lifetime of the ruleset.
  			 */
-			struct access_masks access_masks[];
+			struct landlock_ruleset_layer
+				layer_stack[] __counted_by(num_layers);
  		};
  	};
  };
@@ -248,7 +261,7 @@ landlock_union_access_masks(const struct landlock_ruleset *const domain)
  
  	for (layer_level = 0; layer_level < domain->num_layers; layer_level++) {
  		union access_masks_all layer = {
-			.masks = domain->access_masks[layer_level],
+			.masks = domain->layer_stack[layer_level].access_masks,
  		};
  
  		matches.all |= layer.all;
@@ -296,7 +309,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 |= fs_mask;
+	ruleset->layer_stack[layer_level].access_masks.fs |= fs_mask;
  }
  
  static inline void
@@ -308,7 +321,7 @@ 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 |= net_mask;
+	ruleset->layer_stack[layer_level].access_masks.net |= net_mask;
  }
  
  static inline void
@@ -319,7 +332,7 @@ landlock_add_scope_mask(struct landlock_ruleset *const ruleset,
  
  	/* Should already be checked in sys_landlock_create_ruleset(). */
  	WARN_ON_ONCE(scope_mask != mask);
-	ruleset->access_masks[layer_level].scope |= mask;
+	ruleset->layer_stack[layer_level].access_masks.scope |= mask;
  }
  
  static inline access_mask_t
@@ -327,7 +340,7 @@ landlock_get_fs_access_mask(const struct landlock_ruleset *const ruleset,
  			    const u16 layer_level)
  {
  	/* Handles all initially denied by default access rights. */
-	return ruleset->access_masks[layer_level].fs |
+	return ruleset->layer_stack[layer_level].access_masks.fs |
  	       _LANDLOCK_ACCESS_FS_INITIALLY_DENIED;
  }
  
@@ -335,14 +348,14 @@ 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].net;
+	return ruleset->layer_stack[layer_level].access_masks.net;
  }
  
  static inline access_mask_t
  landlock_get_scope_mask(const struct landlock_ruleset *const ruleset,
  			const u16 layer_level)
  {
-	return ruleset->access_masks[layer_level].scope;
+	return ruleset->layer_stack[layer_level].access_masks.scope;
  }
  
  bool landlock_unmask_layers(const struct landlock_rule *const rule,
diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
index a9760d252fc2..ead9b68168ad 100644
--- a/security/landlock/syscalls.c
+++ b/security/landlock/syscalls.c
@@ -313,7 +313,7 @@ static int add_rule_path_beneath(struct landlock_ruleset *const ruleset,
  		return -ENOMSG;
  
  	/* Checks that allowed_access matches the @ruleset constraints. */
-	mask = ruleset->access_masks[0].fs;
+	mask = landlock_get_fs_access_mask(ruleset, 0);
  	if ((path_beneath_attr.allowed_access | mask) != mask)
  		return -EINVAL;
  
--
2.39.5










