Re: [PATCH bpf-next] bpf: fix crash in bpf_[set|remove]_dentry_xattr for negative dentries

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incomplete Fix: Negative dentries still trigger WARN_ON splats in bpf_get_dentry_xattr and _locked variants
- [High] BPF verifier blindly substitutes locked kfuncs, allowing arbitrary VFS lock bypass
- [High] Missing VFS parameter sanitization allows out-of-bounds xattr sizes and arbitrary flags
--

commit 07ec989f2fe3141fcb87e29833232a3f0591690d
Author: Matt Bobrowski <mattbobrowski@google.com>

bpf: fix crash in bpf_[set|remove]_dentry_xattr for negative dentries

This commit adds a NULL check for the inode in bpf_set_dentry_xattr() and
bpf_remove_dentry_xattr() to prevent a NULL pointer dereference when a
negative dentry is passed. If the inode is NULL, the functions now return
-EINVAL instead of attempting to lock it.

Closes: https://lore.kernel.org/bpf/1587cbf4-1293-4e25-ad24-c970836a1686@std.uestc.edu.cn/

> diff --git a/fs/bpf_fs_kfuncs.c b/fs/bpf_fs_kfuncs.c
> index 9d27be058494..a0d58d5e3022 100644
> --- a/fs/bpf_fs_kfuncs.c
> +++ b/fs/bpf_fs_kfuncs.c
> @@ -289,6 +289,9 @@ __bpf_kfunc int bpf_set_dentry_xattr(struct dentry *dentry, const char *name__st
>  	struct inode *inode = d_inode(dentry);
>  	int ret;
>  
> +	if (!inode)
> +		return -EINVAL;

Are similar checks needed for bpf_get_dentry_xattr() and the _locked variants
(bpf_set_dentry_xattr_locked() and bpf_remove_dentry_xattr_locked())?

If a negative dentry is passed to those functions, they will extract a NULL
inode and pass it to bpf_xattr_write_permission() or
bpf_xattr_read_permission(), which will trigger a WARN_ON(!inode) splat:

fs/bpf_fs_kfuncs.c:bpf_set_dentry_xattr_locked() {
	...
	struct inode *inode = d_inode(dentry);
	...
	ret = bpf_xattr_write_permission(name__str, inode);
	...
}

Could a user-controlled BPF program intentionally pass a negative dentry to
trigger this warning, causing a denial of service on systems with
panic_on_warn enabled?

> +
>  	inode_lock(inode);
>  	ret = bpf_set_dentry_xattr_locked(dentry, name__str, value_p, flags);

This isn't a regression from this patch, but does the BPF verifier safely
handle the substitution of these locked kfuncs?

If a program is attached to an LSM hook that holds an inode lock, the
verifier unconditionally rewrites calls to bpf_set_dentry_xattr() to
bpf_set_dentry_xattr_locked(). The verifier doesn't check if the dentry
argument provided to the kfunc is the specific context argument that the
VFS already locked.

Can a BPF program obtain a trusted pointer to an unrelated, unlocked dentry
and pass it to the kfunc, causing the substituted _locked variant to execute
__vfs_setxattr() without acquiring the proper VFS locks?

Additionally, do these kfuncs bypass necessary VFS parameter sanitization?

The standard setxattr system call sanitizes inputs in setxattr_copy(),
rejecting sizes larger than XATTR_SIZE_MAX and masking flags. Since
bpf_set_dentry_xattr() and its _locked variant directly invoke __vfs_setxattr(),
a BPF program can supply arbitrary unvalidated flags and massive values up
to 4GB.

Could this bypass lead to huge allocations, integer overflows, or
out-of-bounds accesses in drivers that assume VFS-enforced bounds?

>  	inode_unlock(inode);
>  	return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260429205438.2601592-1-mattbobrowski@google.com?part=1