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

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

Thanks! I'll fix a few minor comment issues but otherwise the patch
looks good (i.e. no need to send a v2).

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

It's filename_linkat().

> 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

In fact it's any FS handling.

> 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

I'll describe both references cases.

> +	 * 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