Re: [PATCH v6 02/17] landlock: refactors landlock_find/insert_rule

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

 





7/8/2022 7:57 PM, Mickaël Salaün пишет:

On 08/07/2022 16:20, Konstantin Meskhidze (A) wrote:


7/8/2022 4:56 PM, Mickaël Salaün пишет:

On 08/07/2022 14:53, Konstantin Meskhidze (A) wrote:


7/7/2022 7:44 PM, Mickaël Salaün пишет:

On 21/06/2022 10:22, Konstantin Meskhidze wrote:
Adds a new object union to support a socket port
rule type. Refactors landlock_insert_rule() and
landlock_find_rule() to support coming network
modifications. 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 v5:
* Formats code with clang-format-14.

Changes since v4:
* Refactors insert_rule() and create_rule() functions by deleting
rule_type from their arguments list, it helps to reduce useless code.

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

---
  security/landlock/fs.c      |   7 ++-
  security/landlock/ruleset.c | 105 ++++++++++++++++++++++++++----------
  security/landlock/ruleset.h |  27 +++++-----
  3 files changed, 96 insertions(+), 43 deletions(-)

diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index e6da08ed99d1..46aedc2a05a8 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -173,7 +173,8 @@ 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);
      mutex_unlock(&ruleset->lock);
      /*
       * No need to check for an error because landlock_insert_rule()
@@ -204,7 +205,9 @@ find_rule(const struct landlock_ruleset *const domain,
      inode = d_backing_inode(dentry);
      rcu_read_lock();
      rule = landlock_find_rule(
-        domain, rcu_dereference(landlock_inode(inode)->object));
+        domain,
+        (uintptr_t)rcu_dereference(landlock_inode(inode)->object),
+        LANDLOCK_RULE_PATH_BENEATH);
      rcu_read_unlock();
      return rule;
  }
diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index a3fd58d01f09..5f13f8a12aee 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -35,7 +35,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
@@ -69,7 +69,8 @@ static void build_check_rule(void)
  }

  static struct landlock_rule *
-create_rule(struct landlock_object *const object,
+create_rule(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)
  {
@@ -90,8 +91,15 @@ create_rule(struct landlock_object *const object,
      if (!new_rule)
          return ERR_PTR(-ENOMEM);
      RB_CLEAR_NODE(&new_rule->node);
-    landlock_get_object(object);
-    new_rule->object = object;
+
+    if (object_ptr) {
+        landlock_get_object(object_ptr);
+        new_rule->object.ptr = object_ptr;
+    } else if (object_ptr && object_data) {

Something is wrong with this second check: else + object_ptr?

It was your suggestion to use it like this:
" ....You can also add a WARN_ON_ONCE(object_ptr && object_data)."

Please check it here:
https://lore.kernel.org/linux-security-module/bc44f11f-0eaa-a5f6-c5dc-1d36570f1be1@xxxxxxxxxxx/


Yes, but the error is in the "else", you should write:
if (WARN_ON_ONCE(object_ptr && object_data))
    return ERR_PTR(-EINVAL);

…and this should be before the `if (object_ptr) {` line (to avoid
erronous landlock_get_object() call), just after the `if (!new_rule)` check.

    Ok. I got it.

  Maybe we could delete this check here cause we have it in the upper insert_rule() function??

...
     if (WARN_ON_ONCE(!layers))
         return -ENOENT;
------>    if (WARN_ON_ONCE(object_ptr && object_data))
         return -EINVAL;
     /* Chooses rb_tree structure depending on a rule type. */
     switch (rule_type) {
     case LANDLOCK_RULE_PATH_BENEATH:
         if (WARN_ON_ONCE(!object_ptr))
             return -ENOENT;
         object_data = (uintptr_t)object_ptr;
         root = &ruleset->root_inode;
         break;
     default:
         WARN_ON_ONCE(1);
         return -EINVAL;
     }
...

This is double check here. What do you think?

This check is indeed done twice, and for now create_rule() is only
called from insert_rule(), but I prefer to keep it in both location to
not get bitten in the future were it could be called from other
locations. The compiler may be smart enough to remove the redundant
checks though.

I'll send a patch to improve this part.

 Ok. thanks!!!
.



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

  Powered by Linux