Re: [PATCH v3 -next 01/18] fs: pass struct path to notify_change()

[sashiko-bot@kernel.org]

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 <caixinchen1@huawei.com>

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/20261027121151.1887650-1-caixinchen1@huawei.com?part=1