Re: [RFC PATCH v4 03/15] landlock: landlock_find/insert_rule refactoring

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 17/03/2022 15:29, Konstantin Meskhidze wrote:


3/16/2022 11:27 AM, Mickaël Salaün пишет:

On 09/03/2022 14:44, Konstantin Meskhidze wrote:
A new object union added to support a socket port
rule type. To support it landlock_insert_rule() and
landlock_find_rule() were refactored. Now adding
or searching a rule in a ruleset depends on a
rule_type argument provided in refactored
functions mentioned above.

Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@xxxxxxxxxx>
---

Changes since v3:
* Split commit.
* Refactoring landlock_insert_rule and landlock_find_rule functions.
* Rename new_ruleset->root_inode.

---
  security/landlock/fs.c      |   5 +-
  security/landlock/ruleset.c | 108 +++++++++++++++++++++++++-----------
  security/landlock/ruleset.h |  26 +++++----
  3 files changed, 94 insertions(+), 45 deletions(-)

diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index 97f5c455f5a7..1497948d754f 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -168,7 +168,7 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
      if (IS_ERR(object))
          return PTR_ERR(object);
      mutex_lock(&ruleset->lock);
-    err = landlock_insert_rule(ruleset, object, access_rights);
+    err = landlock_insert_rule(ruleset, object, 0, access_rights, LANDLOCK_RULE_PATH_BENEATH);

For consistency, please use 80 columns everywhere.

   Ok. I got it.

      mutex_unlock(&ruleset->lock);
      /*
       * No need to check for an error because landlock_insert_rule()
@@ -195,7 +195,8 @@ static inline u64 unmask_layers(
      inode = d_backing_inode(path->dentry);
      rcu_read_lock();
      rule = landlock_find_rule(domain,
-            rcu_dereference(landlock_inode(inode)->object));
+            (uintptr_t)rcu_dereference(landlock_inode(inode)->object),
+            LANDLOCK_RULE_PATH_BENEATH);
      rcu_read_unlock();
      if (!rule)
          return layer_mask;
diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index a6212b752549..971685c48641 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -34,7 +34,7 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
          return ERR_PTR(-ENOMEM);
      refcount_set(&new_ruleset->usage, 1);
      mutex_init(&new_ruleset->lock);
-    new_ruleset->root = RB_ROOT;
+    new_ruleset->root_inode = RB_ROOT;
      new_ruleset->num_layers = num_layers;
      /*
       * hierarchy = NULL
@@ -81,10 +81,12 @@ static void build_check_rule(void)
  }

  static struct landlock_rule *create_rule(
-        struct landlock_object *const object,
+        struct landlock_object *const object_ptr,
+        const uintptr_t object_data,
          const struct landlock_layer (*const layers)[],
          const u32 num_layers,
-        const struct landlock_layer *const new_layer)
+        const struct landlock_layer *const new_layer,
+        const u16 rule_type)
  {
      struct landlock_rule *new_rule;
      u32 new_num_layers;
@@ -103,8 +105,16 @@ static struct landlock_rule *create_rule(
      if (!new_rule)
          return ERR_PTR(-ENOMEM);
      RB_CLEAR_NODE(&new_rule->node);
-    landlock_get_object(object);
-    new_rule->object = object;
+
+    switch (rule_type) {
+    case LANDLOCK_RULE_PATH_BENEATH:
+        landlock_get_object(object_ptr);
+        new_rule->object.ptr = object_ptr;
+        break;
+    default:
+        return ERR_PTR(-EINVAL);

This would lead to memory leak. You should at least add a WARN_ON_ONCE(1) here, but a proper solution would be to remove the use of rule_type and only rely on object_ptr and object_data values. You can also add a WARN_ON_ONCE(object_ptr && object_data).


   But rule_type is needed here in coming commits to support network
  rules. For LANDLOCK_RULE_PATH_BENEATH rule type landlock_get_object() is used but for LANDLOCK_RULE_NET_SERVICE is not. Using rule type is convenient for distinguising between fs and network rules.

rule_type is not required to infer if the rule use a pointer or raw data, even with the following commits, because you can rely on object_ptr being NULL or not. This would make create_rule() generic for pointer-based and data-based object, even if not-yet-existing rule types. It is less error-prone to only be able to infer something from one source (i.e. object_ptr and not rule_type).


+    }
+
      new_rule->num_layers = new_num_layers;
      /* Copies the original layer stack. */
      memcpy(new_rule->layers, layers,
@@ -120,7 +130,7 @@ static void free_rule(struct landlock_rule *const rule)
      might_sleep();
      if (!rule)
          return;
-    landlock_put_object(rule->object);
+    landlock_put_object(rule->object.ptr);
      kfree(rule);
  }

@@ -156,26 +166,38 @@ static void build_check_ruleset(void)
   * access rights.
   */
  static int insert_rule(struct landlock_ruleset *const ruleset,
-        struct landlock_object *const object,
+        struct landlock_object *const object_ptr,
+        const uintptr_t object_data,

Can you move rule_type here for this function and similar ones? It makes sense to group object-related arguments.


          const struct landlock_layer (*const layers)[],
-        size_t num_layers)
+        size_t num_layers, u16 rule_type)
  {
      struct rb_node **walker_node;
      struct rb_node *parent_node = NULL;
      struct landlock_rule *new_rule;
+    uintptr_t object;
+    struct rb_root *root;

      might_sleep();
      lockdep_assert_held(&ruleset->lock);
-    if (WARN_ON_ONCE(!object || !layers))
-        return -ENOENT;

You can leave this code here.

 But anyway in coming commits with network rules this code will be moved into case LANDLOCK_RULE_PATH_BENEATH: ....

Yes, but without rule_type you don't need to duplicate this check, just to remove object_ptr from WARN_ON_ONCE() and replace the rule_type switch/case with if (object_ptr).

You can change to this:

--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -194,43 +194,49 @@ static void build_check_ruleset(void)
  */
 static int insert_rule(struct landlock_ruleset *const ruleset,
 		struct landlock_object *const object_ptr,
-		const uintptr_t object_data,
+		uintptr_t object_data, /* move @rule_type here */
 		const struct landlock_layer (*const layers)[],
-		size_t num_layers, u16 rule_type)
+		size_t num_layers, const enum landlock_rule_type rule_type)
 {
 	struct rb_node **walker_node;
 	struct rb_node *parent_node = NULL;
 	struct landlock_rule *new_rule;
-	uintptr_t object;
 	struct rb_root *root;

 	might_sleep();
 	lockdep_assert_held(&ruleset->lock);
-	/* Choose rb_tree structure depending on a rule type */
+
+	if (WARN_ON_ONCE(!layers))
+		return -ENOENT;
+	if (WARN_ON_ONCE(object_ptr && object_data))
+		return -EINVAL;
+
+	/* Chooses the rb_tree according to the rule type. */
 	switch (rule_type) {
 	case LANDLOCK_RULE_PATH_BENEATH:
-		if (WARN_ON_ONCE(!object_ptr || !layers))
+		if (WARN_ON_ONCE(!object_ptr))
 			return -ENOENT;
-		object = (uintptr_t)object_ptr;
+		object_data = (uintptr_t)object_ptr;
 		root = &ruleset->root_inode;
 		break;
 	case LANDLOCK_RULE_NET_SERVICE:
-		if (WARN_ON_ONCE(!object_data || !layers))
-			return -ENOENT;
-		object = object_data;
+		if (WARN_ON_ONCE(object_ptr))
+			return -EINVAL;
 		root = &ruleset->root_net_port;
 		break;
 	default:
+		WARN_ON_ONCE(1);
 		return -EINVAL;
 	}
+
 	walker_node = &root->rb_node;
 	while (*walker_node) {
 		struct landlock_rule *const this = rb_entry(*walker_node,
 				struct landlock_rule, node);

-		if (this->object.data != object) {
+		if (this->object.data != object_data) {
 			parent_node = *walker_node;
-			if (this->object.data < object)
+			if (this->object.data < object_data)
 				walker_node = &((*walker_node)->rb_right);
 			else
 				walker_node = &((*walker_node)->rb_left);


This highlight an implicit error handling for a port value of 0. I'm not sure if this should be allowed or not though. If not, it should be an explicit service_port check in add_rule_net_service(). A data value of zero might be legitimate for this use case or not-yet-existing data-based rule types. Anyway, this kind of check is specific to the use case and should not be part of insert_rule().




-    walker_node = &(ruleset->root.rb_node);
+    /* Choose rb_tree structure depending on a rule type */
+    switch (rule_type) {
+    case LANDLOCK_RULE_PATH_BENEATH:
+        if (WARN_ON_ONCE(!object_ptr || !layers))
+            return -ENOENT;
+        object = (uintptr_t)object_ptr;
+        root = &ruleset->root_inode;
+        break;
+    default:
+        return -EINVAL;
+    }
+    walker_node = &root->rb_node;
      while (*walker_node) {
          struct landlock_rule *const this = rb_entry(*walker_node,
                  struct landlock_rule, node);

-        if (this->object != object) {
+        if (this->object.data != object) {
              parent_node = *walker_node;
-            if (this->object < object)
+            if (this->object.data < object)
                  walker_node = &((*walker_node)->rb_right);
              else
                  walker_node = &((*walker_node)->rb_left);
@@ -207,11 +229,15 @@ static int insert_rule(struct landlock_ruleset *const ruleset,
           * Intersects access rights when it is a merge between a
           * ruleset and a domain.
           */
-        new_rule = create_rule(object, &this->layers, this->num_layers,
-                &(*layers)[0]);
+        switch (rule_type) {
+        case LANDLOCK_RULE_PATH_BENEATH:

Same here and for the following code, you should replace such switch/case with an if (object_ptr).
   What about coming commits with network rule_type support?

This will still works.



+            new_rule = create_rule(object_ptr, 0, &this->layers, this->num_layers,
+                           &(*layers)[0], rule_type);
+            break;
+        }
          if (IS_ERR(new_rule))
              return PTR_ERR(new_rule);
-        rb_replace_node(&this->node, &new_rule->node, &ruleset->root);
+        rb_replace_node(&this->node, &new_rule->node, &ruleset->root_inode);

Use the root variable here. Same for the following code and patches.

 What about your suggestion to use 2 rb_tress to support different rule_types:
      1. root_inode - for filesystem objects
          2. root_net_port - for network port objects
????

I was talking about the root variable you declared a few line before. The conversion from ruleset->root to ruleset->root_inode is fine.


[...]

@@ -465,20 +501,28 @@ struct landlock_ruleset *landlock_merge_ruleset(
   */
  const struct landlock_rule *landlock_find_rule(
          const struct landlock_ruleset *const ruleset,
-        const struct landlock_object *const object)
+        const uintptr_t object_data, const u16 rule_type)
  {
      const struct rb_node *node;

-    if (!object)
+    if (!object_data)

object_data can be 0. You need to add a test with such value.

We need to be sure that this change cannot affect the current FS code.

  I got it. I will refactor it.

Well, 0 means a port 0, which might not be correct, but this check should not be performed by landlock_merge_ruleset().




          return NULL;
-    node = ruleset->root.rb_node;
+
+    switch (rule_type) {
+    case LANDLOCK_RULE_PATH_BENEATH:
+        node = ruleset->root_inode.rb_node;
+        break;
+    default:
+        return ERR_PTR(-EINVAL);

This is a bug. There is no check for such value. You need to check and update all call sites to catch such errors. Same for all new use of ERR_PTR().

Sorry, I did not get your point.
Do you mean I should check the correctness of rule_type in above function which calls landlock_find_rule() ??? Why can't I add such check here?

landlock_find_rule() only returns NULL or a valid pointer, not an error.

[...]



[Index of Archives]     [Netfitler Users]     [Berkeley Packet Filter]     [LARTC]     [Bugtraq]     [Yosemite Forum]

  Powered by Linux