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 <[email protected]> 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/[email protected]?part=3
