Re: [RFC PATCH 1/3] landlock: walk parent dir without taking references

["Mickaël Salaün" <mic@digikod.net>]

On Wed, Jun 04, 2025 at 01:45:43AM +0100, Tingmao Wang wrote:
> This commit replaces dget_parent with a direct read of d_parent. By
> holding rcu read lock we should be safe in terms of not reading freed
> memory, but this is still problematic due to move+unlink, as will be shown
> with the test in the next commit.
> 
> Note that follow_up is still used when walking up a mountpoint.
> 
> Signed-off-by: Tingmao Wang <m@maowtm.org>
> ---
>  security/landlock/fs.c | 40 ++++++++++++++++++++++------------------
>  1 file changed, 22 insertions(+), 18 deletions(-)
> 
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index 6fee7c20f64d..923737412cfa 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -361,7 +361,7 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
>   * Returns NULL if no rule is found or if @dentry is negative.
>   */
>  static const struct landlock_rule *
> -find_rule(const struct landlock_ruleset *const domain,
> +find_rule_rcu(const struct landlock_ruleset *const domain,
>  	  const struct dentry *const dentry)
>  {
>  	const struct landlock_rule *rule;
> @@ -375,10 +375,10 @@ find_rule(const struct landlock_ruleset *const domain,
>  		return NULL;
>  
>  	inode = d_backing_inode(dentry);
> -	rcu_read_lock();
> +	if (unlikely(!inode))
> +		return NULL;
>  	id.key.object = rcu_dereference(landlock_inode(inode)->object);
>  	rule = landlock_find_rule(domain, id);
> -	rcu_read_unlock();
>  	return rule;
>  }
>  
> @@ -809,9 +809,11 @@ static bool is_access_to_paths_allowed(
>  		is_dom_check = false;
>  	}
>  
> +	rcu_read_lock();
> +
>  	if (unlikely(dentry_child1)) {
>  		landlock_unmask_layers(
> -			find_rule(domain, dentry_child1),
> +			find_rule_rcu(domain, dentry_child1),
>  			landlock_init_layer_masks(
>  				domain, LANDLOCK_MASK_ACCESS_FS,
>  				&_layer_masks_child1, LANDLOCK_KEY_INODE),
> @@ -821,7 +823,7 @@ static bool is_access_to_paths_allowed(
>  	}
>  	if (unlikely(dentry_child2)) {
>  		landlock_unmask_layers(
> -			find_rule(domain, dentry_child2),
> +			find_rule_rcu(domain, dentry_child2),
>  			landlock_init_layer_masks(
>  				domain, LANDLOCK_MASK_ACCESS_FS,
>  				&_layer_masks_child2, LANDLOCK_KEY_INODE),
> @@ -831,7 +833,6 @@ static bool is_access_to_paths_allowed(
>  	}
>  
>  	walker_path = *path;
> -	path_get(&walker_path);
>  	/*
>  	 * We need to walk through all the hierarchy to not miss any relevant
>  	 * restriction.
> @@ -880,7 +881,7 @@ static bool is_access_to_paths_allowed(
>  				break;
>  		}
>  
> -		rule = find_rule(domain, walker_path.dentry);
> +		rule = find_rule_rcu(domain, walker_path.dentry);
>  		allowed_parent1 = allowed_parent1 ||
>  				  landlock_unmask_layers(
>  					  rule, access_masked_parent1,
> @@ -897,10 +898,14 @@ static bool is_access_to_paths_allowed(
>  			break;
>  jump_up:
>  		if (walker_path.dentry == walker_path.mnt->mnt_root) {
> +			/* follow_up gets the parent and puts the passed in path */
> +			path_get(&walker_path);
>  			if (follow_up(&walker_path)) {
> +				path_put(&walker_path);

path_put() cannot be safely called in a RCU read-side critical section
because it can free memory which can sleep, and also because it can wait
for a lock.  However, we can call rcu_read_unlock() before and
rcu_read_lock() after (if we hold a reference).

>  				/* Ignores hidden mount points. */
>  				goto jump_up;
>  			} else {
> +				path_put(&walker_path);
>  				/*
>  				 * Stops at the real root.  Denies access
>  				 * because not all layers have granted access.
> @@ -920,11 +925,11 @@ static bool is_access_to_paths_allowed(
>  			}
>  			break;
>  		}
> -		parent_dentry = dget_parent(walker_path.dentry);
> -		dput(walker_path.dentry);
> +		parent_dentry = walker_path.dentry->d_parent;
>  		walker_path.dentry = parent_dentry;
>  	}
> -	path_put(&walker_path);
> +
> +	rcu_read_unlock();
>  
>  	if (!allowed_parent1) {
>  		log_request_parent1->type = LANDLOCK_REQUEST_FS_ACCESS;
> @@ -1045,12 +1050,11 @@ static bool collect_domain_accesses(
>  					       layer_masks_dom,
>  					       LANDLOCK_KEY_INODE);
>  
> -	dget(dir);
> -	while (true) {
> -		struct dentry *parent_dentry;
> +	rcu_read_lock();
>  
> +	while (true) {
>  		/* Gets all layers allowing all domain accesses. */
> -		if (landlock_unmask_layers(find_rule(domain, dir), access_dom,
> +		if (landlock_unmask_layers(find_rule_rcu(domain, dir), access_dom,
>  					   layer_masks_dom,
>  					   ARRAY_SIZE(*layer_masks_dom))) {
>  			/*
> @@ -1065,11 +1069,11 @@ static bool collect_domain_accesses(
>  		if (dir == mnt_root || WARN_ON_ONCE(IS_ROOT(dir)))
>  			break;
>  
> -		parent_dentry = dget_parent(dir);
> -		dput(dir);
> -		dir = parent_dentry;
> +		dir = dir->d_parent;
>  	}
> -	dput(dir);
> +
> +	rcu_read_unlock();
> +
>  	return ret;
>  }
>  
> -- 
> 2.49.0
> 
>