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

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@huawei.com

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.

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