Re: [PATCH -next v2 2/6] landlock: abstract walk_to_visible_parent() helper

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

 



Hi,

在 2022/8/30 19:22, Mickaël Salaün 写道:

On 27/08/2022 13:12, Xiu Jianfeng wrote:
This helper will be used in the next commit which supports chmod and
chown access rights restriction.

Signed-off-by: Xiu Jianfeng <xiujianfeng@xxxxxxxxxx>
---
  security/landlock/fs.c | 67 ++++++++++++++++++++++++++++++------------
  1 file changed, 49 insertions(+), 18 deletions(-)

diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index c57f581a9cd5..4ef614a4ea22 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -38,6 +38,44 @@
  #include "ruleset.h"
  #include "setup.h"
+enum walk_result {
+    WALK_CONTINUE,
+    WALK_TO_REAL_ROOT,
+    WALK_TO_DISCONN_ROOT,

Why did you created these results instead of the ones I proposed?


+};
+
+/*
+ * walk to the visible parent, caller should call path_get()/path_put()
+ * before/after this helpler.
+ *
+ * Returns:
+ * - WALK_TO_REAL_ROOT if walk to the real root;
+ * - WALK_TO_DISCONN_ROOT if walk to disconnected root;
+ * - WALK_CONTINUE otherwise.
+ */
+static enum walk_result walk_to_visible_parent(struct path *path)
+{
+    struct dentry *parent_dentry;
+jump_up:
+    if (path->dentry == path->mnt->mnt_root) {
+        if (follow_up(path)) {
+            /* Ignores hidden mount points. */
+            goto jump_up;
+        } else {
+            /* Stop at the real root. */
+            return WALK_TO_REAL_ROOT;
+        }
+    }
+    /* Stops at disconnected root directories. */
+    if (unlikely(IS_ROOT(path->dentry)))
+        return WALK_TO_DISCONN_ROOT;
+    parent_dentry = dget_parent(path->dentry);
+    dput(path->dentry);
+    path->dentry = parent_dentry;
+
+    return WALK_CONTINUE;
+}
+
  /* Underlying object management */
  static void release_inode(struct landlock_object *const object)
@@ -539,8 +577,8 @@ static int check_access_path_dual(
       * restriction.
       */
      while (true) {
-        struct dentry *parent_dentry;
          const struct landlock_rule *rule;
+        enum walk_result wr;

Please make the names understandable. In this case this variable may not be needed anyway.


          /*
           * If at least all accesses allowed on the destination are
@@ -588,20 +626,12 @@ static int check_access_path_dual(
          if (allowed_parent1 && allowed_parent2)
              break;
-jump_up:
-        if (walker_path.dentry == walker_path.mnt->mnt_root) {
-            if (follow_up(&walker_path)) {
-                /* Ignores hidden mount points. */
-                goto jump_up;
-            } else {
-                /*
-                 * Stops at the real root.  Denies access
-                 * because not all layers have granted access.
-                 */
-                break;
-            }
-        }
-        if (unlikely(IS_ROOT(walker_path.dentry))) {
+        wr = walk_to_visible_parent(&walker_path);
+        switch (wr) {
+        case WALK_TO_REAL_ROOT:
+            /* Stop at the real root. */
+            goto out;
+        case WALK_TO_DISCONN_ROOT:
              /*
               * Stops at disconnected root directories.  Only allows
               * access to internal filesystems (e.g. nsfs, which is
@@ -609,12 +639,13 @@ static int check_access_path_dual(
               */
              allowed_parent1 = allowed_parent2 =
                  !!(walker_path.mnt->mnt_flags & MNT_INTERNAL);

Why not include this check in the helper? This is then not checked in patch 3 with current_check_access_path_context_only(), which is a bug.

I get your point, after moving it to the helper, here should be:

              while (true) {
                   ...
                   switch(walk_to_visible_parent(&walker_path)) {
                   case WALK_CONTINUE:
                      break;
                   case WALK_ALLOWED:
                      allowed_parent1 = allowed_parent2 = true;
                      goto out;
                   case WR_DENIED:
                   default:
                      allowed_parent1 = allowed_parent2 = false;
                      goto out;
                    }
            }


+            goto out;
+        case WALK_CONTINUE:
+        default:
              break;
          }
-        parent_dentry = dget_parent(walker_path.dentry);
-        dput(walker_path.dentry);
-        walker_path.dentry = parent_dentry;
      }

+out:
      path_put(&walker_path);
      if (allowed_parent1 && allowed_parent2)
.



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux