Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] fs: __notify_change() in ntfs_stream_setattr_common() bypasses base inode LSM checks - [Low] nfsd: stale @dentry kernel-doc parameter in nfsd4_deleg_getattr_conflict() --
commit 205fb5135661a50181cb638aff919af8443d55ff Author: Cai Xinchen <[email protected]> fs: pass struct path to notify_change() This commit updates notify_change() and do_truncate() to accept a struct path pointer instead of a struct dentry pointer alongside the mount idmap. It also introduces __notify_change() without the security_inode_setattr() check and uses it in ntfs_stream_setattr_common() when forwarding attribute updates to the base inode. > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 1de6c6d757c3e..3a08c56a27c60 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c [ ... ] > @@ -10531,7 +10533,7 @@ static int cb_getattr_update_times(struct dentry > *dentry, struct nfs4_delegation > * caller must put the reference. > */ > __be32 > -nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry, > +nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, const struct path *path, > struct nfs4_delegation **pdp) [Severity: Low] Since the second parameter of nfsd4_deleg_getattr_conflict() in fs/nfsd/nfs4state.c was changed from dentry to path, should the kernel-doc comment right above it also be updated from @dentry to @path to avoid scripts/kernel-doc warnings when building with W=1 or htmldocs? [ ... ] > diff --git a/fs/ntfs/named_stream.c b/fs/ntfs/named_stream.c > index 95a3c05f98211..f0b577a6f7c3e 100644 > --- a/fs/ntfs/named_stream.c > +++ b/fs/ntfs/named_stream.c > @@ -897,7 +897,7 @@ static int ntfs_stream_setattr_common(const struct > mnt_idmap *idmap, > goto out; > } > if (base_attr.ia_valid) > - err = notify_change(idmap, base_dentry, &base_attr, > + err = __notify_change(idmap, base_dentry, &base_attr, > NULL); > else > err = 0; [Severity: High] Does switching to __notify_change() in ntfs_stream_setattr_common() skip required LSM and EVM checks on the base inode when a user calls chmod, chown, or utimensat on an NTFS named stream? The stream inode (vi) and the base inode (base_vi) are separate struct inode objects with distinct security state and potentially different file types. First, NTFS permits named data streams on directories, and ntfs_stream_setattr_common() preserves base_vi's S_IFMT when forwarding mode changes: else if (base_attr.ia_valid & ATTR_MODE) base_attr.ia_mode = (base_attr.ia_mode & ~S_IFMT) | (base_vi->i_mode & S_IFMT); However, ntfs_read_locked_attr_inode() and ntfs_stream_inode_refresh() always force the stream inode's mode to S_IFREG: base_vi = VFS_I(ni->ext.base_ntfs_ino); vi->i_uid = base_vi->i_uid; vi->i_gid = base_vi->i_gid; vi->i_mode = (base_vi->i_mode & ~S_IFMT) | S_IFREG; In SELinux, the initial notify_change() on the stream dentry checks the stream inode with sclass SECCLASS_FILE, whereas base_vi has sclass SECCLASS_DIR. Skipping security_inode_setattr() on base_dentry via __notify_change() allows a caller with file setattr permission (but not dir setattr permission) to modify the base directory's mode, ownership, or timestamps through a directory stream. Second, ntfs_setxattr() rejects stream inodes with -EOPNOTSUPP while allowing security.* xattr changes on base_vi. If base_vi is relabeled via setxattr() while a stream inode is cached in memory (such as via an open stream file descriptor), security_inode_post_setxattr() (including selinux_inode_post_setxattr(), smack_inode_post_setxattr(), and evm_reset_status()) only updates base_vi's security state, leaving the stream inode's cached label and EVM status stale. Furthermore, in fs/attr.c, do_notify_change() skips security_inode_setattr() when path is NULL, but still runs security_inode_post_setattr() at the end: if (path) { error = security_inode_setattr(idmap, dentry, attr); if (error) return error; } ... if (!error) { fsnotify_change(dentry, ia_valid); security_inode_post_setattr(idmap, dentry, ia_valid); } Wouldn't calling __notify_change() on base_dentry bypass selinux_inode_setattr(), smack_inode_setattr(), and evm_inode_setattr() on base_dentry while still invoking evm_inode_post_setattr(), causing EVM to write a new valid security.evm HMAC onto base_dentry without prior integrity verification on base_dentry? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
