Re: [PATCH v2 -next 03/18] fs: pass struct path to xattr helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ksmbd: Mismatched mount and dentry in ksmbd_vfs_fill_dentry_attrs() causes incorrect ID mapping and kernel warnings
- [High] smb: UAF via lockless d_parent access in smb_inherit_dacl and dotdot population
--

commit 9779da3303c8331c79c9d2b6c3b16ab94a463c50
Author: Cai Xinchen <caixinchen1@huawei.com>
Subject: fs: pass struct path to xattr helpers

This patch switches vfs_setxattr(), vfs_getxattr(), vfs_listxattr(), and
vfs_removexattr() to take a struct path instead of a struct dentry and
mnt_idmap. It updates various callers across the tree to pass the path
directly, reducing the need to manually derive the idmap and dentry from it.

> diff --git a/fs/smb/server/smb_common.c b/fs/smb/server/smb_common.c
> --- a/fs/smb/server/smb_common.c
> +++ b/fs/smb/server/smb_common.c
> @@ -493,7 +492,6 @@ int ksmbd_populate_dot_dotdot_entries(struct ksmbd_work *work, int info_level,
>  
>  			ksmbd_kstat.kstat = &kstat;
>  			rc = ksmbd_vfs_fill_dentry_attrs(work,
> -							 idmap,
>  							 dentry,
>  							 &ksmbd_kstat);

[Severity: High]
Can this sequence lead to a use-after-free on the parent directory?

In ksmbd_populate_dot_dotdot_entries(), the ".." entry is resolved locklessly
via dir->filp->f_path.dentry->d_parent. This unpinned dentry is then passed 
into ksmbd_vfs_fill_dentry_attrs(). 

Because this patch forces the helper to wrap the unpinned dentry into a new
struct path, if the parent is concurrently renamed and removed, passing it
into sleeping VFS helpers could access freed memory once the RCU grace period
expires.

[ ... ]

> diff --git a/fs/smb/server/smbacl.c b/fs/smb/server/smbacl.c
> --- a/fs/smb/server/smbacl.c
> +++ b/fs/smb/server/smbacl.c
> @@ -1199,8 +1199,8 @@ int smb_inherit_dacl(struct ksmbd_conn *conn,
>  	struct smb_acl *parent_pdacl;
>  	struct smb_ntsd *parent_pntsd = NULL;
>  	struct smb_sid owner_sid, group_sid;
> -	struct dentry *parent = path->dentry->d_parent;
> -	const struct mnt_idmap *idmap = mnt_idmap(path->mnt);
> +	struct path parent_path = { .mnt = path->mnt,
> +				    .dentry = path->dentry->d_parent };

[Severity: High]
Could this lockless d_parent access cause a use-after-free?

This code constructs a new struct path using path->dentry->d_parent without
acquiring a reference via dget_parent(). If the child dentry is concurrently
renamed and the old parent is removed, the unreferenced parent dentry could
be queued for RCU freeing.

Because this unpinned dentry is then passed to ksmbd_vfs_get_sd_xattr()
which can sleep, the RCU grace period might expire during use.

>  	int inherited_flags = 0, flags = 0, i, nt_size = 0, pdacl_size;
>  	int rc = 0, pntsd_type, ppntsd_size, acl_len, aces_size;
>  	unsigned int dacloffset;
> @@ -1209,8 +1209,8 @@ int smb_inherit_dacl(struct ksmbd_conn *conn,
>  	char *aces_base;
>  	bool is_dir = S_ISDIR(d_inode(path->dentry)->i_mode);
>  
> -	ppntsd_size = ksmbd_vfs_get_sd_xattr(conn, idmap,
> -					    parent, &parent_pntsd);
> +	ppntsd_size = ksmbd_vfs_get_sd_xattr(conn, &parent_path,
> +					    &parent_pntsd);

[ ... ]

> diff --git a/fs/smb/server/vfs.c b/fs/smb/server/vfs.c
> --- a/fs/smb/server/vfs.c
> +++ b/fs/smb/server/vfs.c
> @@ -1913,7 +1896,7 @@ int ksmbd_vfs_fill_dentry_attrs(struct ksmbd_work *work,
>  				   KSMBD_SHARE_FLAG_STORE_DOS_ATTRS)) {
>  		struct xattr_dos_attrib da;
>  
> -		rc = ksmbd_vfs_get_dos_attrib_xattr(idmap, dentry, &da);
> +		rc = ksmbd_vfs_get_dos_attrib_xattr(&path, &da);

[Severity: High]
Will this synthetic path construction cause VFS invariant warnings or
incorrect ID mapping?

The ksmbd_vfs_fill_dentry_attrs() helper internally constructs the path using
the share root's mount (share_conf->vfs_path.mnt) and the caller-provided
dentry. If the dentry belongs to a nested mount within the share, the path
mount and dentry superblocks will mismatch.

When this mismatched path is passed down to vfs_getxattr(), mnt_idmap() will
extract the ID mapping of the share root instead of the nested submount,
causing incorrect permission checks. Underlying filesystems like OverlayFS
also enforce this invariant with a WARN_ON, which can panic systems.

Could we update the callers (like process_query_dir_entries) to pass down
their valid path instead of just the dentry?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015125425.3246251-1-caixinchen1@huawei.com?part=3