[PATCH] landlock: Fix use-after-free of the source's parent directory

[PATCH] landlock: Fix use-after-free of the source's parent directory

[Norbert Szetei]

current_check_refer_path() reads old_dentry->d_parent without holding a
reference nor a lock on it, and then dereferences it in
collect_domain_accesses() and in the audit record.

A reference on a child does not pin its parent: __d_move() reassigns
dentry->d_parent and drops the reference the child held on its former
parent.  hook_path_rename() is not affected because the rename path calls
lock_rename() before the hook, so the source cannot be reparented under
it.  hook_path_link() has no such protection: do_linkat() holds a
reference on the source dentry but neither locks nor references its
parent, so a concurrent rename(2) can reparent the source while
security_path_link() runs, and the former parent can then be removed and
freed while the hook walks it.

Any process able to sandbox itself with LANDLOCK_ACCESS_FS_REFER can
trigger this with a linkat(2) loop racing rename(2) and rmdir(2):

  BUG: KASAN: slab-use-after-free in collect_domain_accesses+0x278/0x290
  Read of size 4 at addr ffff888160bd53f4 by task llrepro2/549
   collect_domain_accesses+0x278/0x290
   current_check_refer_path+0x952/0x1120
   security_path_link+0x1be/0x320
   filename_linkat+0x342/0x6d0
   __x64_sys_linkat+0xfa/0x150
  Freed by task 562:
   kmem_cache_free+0x139/0x4c0
   i_callback+0x4b/0x80
   rcu_core+0x7dc/0x10a0

Take a reference on the parent with dget_parent(), and release it once
the hierarchy walk and the audit record are done.

Cc: stable@vger.kernel.org
Fixes: b91c3e4ea756 ("landlock: Add support for file reparenting with LANDLOCK_ACCESS_FS_REFER")
Signed-off-by: Norbert Szetei <norbert@doyensec.com>
---
 security/landlock/fs.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index 30aa6ce13590..200c83372bbe 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -1298,11 +1298,12 @@ static int current_check_refer_path(struct dentry *const old_dentry,
 	/*
 	 * old_dentry may be the root of the common mount point and
 	 * !IS_ROOT(old_dentry) at the same time (e.g. with open_tree() and
-	 * OPEN_TREE_CLONE).  We do not need to call dget(old_parent) because
-	 * we keep a reference to old_dentry.
+	 * OPEN_TREE_CLONE).  Pins the parent in both cases: a reference on
+	 * old_dentry does not pin its parent, which may then be freed after a
+	 * concurrent rename(2).
 	 */
-	old_parent = (old_dentry == mnt_dir.dentry) ? old_dentry :
-						      old_dentry->d_parent;
+	old_parent = (old_dentry == mnt_dir.dentry) ? dget(old_dentry) :
+						      dget_parent(old_dentry);
 
 	/* new_dir->dentry is equal to new_dentry->d_parent */
 	allow_parent1 = collect_domain_accesses(subject->domain, mnt_dir.dentry,
@@ -1311,8 +1312,10 @@ static int current_check_refer_path(struct dentry *const old_dentry,
 	allow_parent2 = collect_domain_accesses(subject->domain, mnt_dir.dentry,
 						new_dir->dentry,
 						&layer_masks_parent2);
-	if (allow_parent1 && allow_parent2)
+	if (allow_parent1 && allow_parent2) {
+		dput(old_parent);
 		return 0;
+	}
 
 	/*
 	 * To be able to compare source and destination domain access rights,
@@ -1324,8 +1327,10 @@ static int current_check_refer_path(struct dentry *const old_dentry,
 		    subject->domain, &mnt_dir, access_request_parent1,
 		    &layer_masks_parent1, &request1, old_dentry,
 		    access_request_parent2, &layer_masks_parent2, &request2,
-		    exchange ? new_dentry : NULL))
+		    exchange ? new_dentry : NULL)) {
+		dput(old_parent);
 		return 0;
+	}
 
 	if (request1.access) {
 		request1.audit.u.path.dentry = old_parent;
@@ -1335,6 +1340,7 @@ static int current_check_refer_path(struct dentry *const old_dentry,
 		request2.audit.u.path.dentry = new_dir->dentry;
 		landlock_log_denial(subject, &request2);
 	}
+	dput(old_parent);
 
 	/*
 	 * This prioritizes EACCES over EXDEV for all actions, including
-- 
2.55.0

Re: [PATCH] landlock: Fix use-after-free of the source's parent directory

[Günther Noack]

On Sat, Aug 22, 2026 at 02:29:00PM +0200, Norbert Szetei wrote:
> current_check_refer_path() reads old_dentry->d_parent without holding a
> reference nor a lock on it, and then dereferences it in
> collect_domain_accesses() and in the audit record.
> 
> A reference on a child does not pin its parent: __d_move() reassigns
> dentry->d_parent and drops the reference the child held on its former
> parent.  hook_path_rename() is not affected because the rename path calls
> lock_rename() before the hook, so the source cannot be reparented under
> it.  hook_path_link() has no such protection: do_linkat() holds a
> reference on the source dentry but neither locks nor references its
> parent, so a concurrent rename(2) can reparent the source while
> security_path_link() runs, and the former parent can then be removed and
> freed while the hook walks it.
> 
> Any process able to sandbox itself with LANDLOCK_ACCESS_FS_REFER can
> trigger this with a linkat(2) loop racing rename(2) and rmdir(2):
> 
>   BUG: KASAN: slab-use-after-free in collect_domain_accesses+0x278/0x290
>   Read of size 4 at addr ffff888160bd53f4 by task llrepro2/549
>    collect_domain_accesses+0x278/0x290
>    current_check_refer_path+0x952/0x1120
>    security_path_link+0x1be/0x320
>    filename_linkat+0x342/0x6d0
>    __x64_sys_linkat+0xfa/0x150
>   Freed by task 562:
>    kmem_cache_free+0x139/0x4c0
>    i_callback+0x4b/0x80
>    rcu_core+0x7dc/0x10a0
> 
> Take a reference on the parent with dget_parent(), and release it once
> the hierarchy walk and the audit record are done.
> 
> Cc: stable@vger.kernel.org
> Fixes: b91c3e4ea756 ("landlock: Add support for file reparenting with LANDLOCK_ACCESS_FS_REFER")
> Signed-off-by: Norbert Szetei <norbert@doyensec.com>
> ---
>  security/landlock/fs.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index 30aa6ce13590..200c83372bbe 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -1298,11 +1298,12 @@ static int current_check_refer_path(struct dentry *const old_dentry,
>  	/*
>  	 * old_dentry may be the root of the common mount point and
>  	 * !IS_ROOT(old_dentry) at the same time (e.g. with open_tree() and
> -	 * OPEN_TREE_CLONE).  We do not need to call dget(old_parent) because
> -	 * we keep a reference to old_dentry.
> +	 * OPEN_TREE_CLONE).  Pins the parent in both cases: a reference on
> +	 * old_dentry does not pin its parent, which may then be freed after a
> +	 * concurrent rename(2).
>  	 */
> -	old_parent = (old_dentry == mnt_dir.dentry) ? old_dentry :
> -						      old_dentry->d_parent;
> +	old_parent = (old_dentry == mnt_dir.dentry) ? dget(old_dentry) :
> +						      dget_parent(old_dentry);
>  
>  	/* new_dir->dentry is equal to new_dentry->d_parent */
>  	allow_parent1 = collect_domain_accesses(subject->domain, mnt_dir.dentry,
> @@ -1311,8 +1312,10 @@ static int current_check_refer_path(struct dentry *const old_dentry,
>  	allow_parent2 = collect_domain_accesses(subject->domain, mnt_dir.dentry,
>  						new_dir->dentry,
>  						&layer_masks_parent2);
> -	if (allow_parent1 && allow_parent2)
> +	if (allow_parent1 && allow_parent2) {
> +		dput(old_parent);
>  		return 0;
> +	}
>  
>  	/*
>  	 * To be able to compare source and destination domain access rights,
> @@ -1324,8 +1327,10 @@ static int current_check_refer_path(struct dentry *const old_dentry,
>  		    subject->domain, &mnt_dir, access_request_parent1,
>  		    &layer_masks_parent1, &request1, old_dentry,
>  		    access_request_parent2, &layer_masks_parent2, &request2,
> -		    exchange ? new_dentry : NULL))
> +		    exchange ? new_dentry : NULL)) {
> +		dput(old_parent);
>  		return 0;
> +	}
>  
>  	if (request1.access) {
>  		request1.audit.u.path.dentry = old_parent;
> @@ -1335,6 +1340,7 @@ static int current_check_refer_path(struct dentry *const old_dentry,
>  		request2.audit.u.path.dentry = new_dir->dentry;
>  		landlock_log_denial(subject, &request2);
>  	}
> +	dput(old_parent);
>  
>  	/*
>  	 * This prioritizes EACCES over EXDEV for all actions, including
> -- 
> 2.55.0

Reviewed-by: Günther Noack <gnoack3000@gmail.com>
Tested-by: Günther Noack <gnoack3000@gmail.com>

Thank you for the bug report and patch, Norbert!  Excellent finding!
I can validate the bug and that your patch fixes the problem.

–Günther