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

On Wed, Apr 29, 2026 at 10:10:00PM +0000, [email protected] wrote:
> 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 <[email protected]>
> 
> 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/[email protected]/
> 
> > 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?

Yes, dropping the WARN_ON(!inode) makes sense to me. By definition,
attempting to set an extended attribute on a negative dentry is
considered as an invalid operation, so it should be handled
gracefully.

> > +
> >  	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?

Probably a valid concern and the current approach does seem a little
fragile. This too should be reviewed in further depth separately
though.

> 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?

I believe this is a valid concern and should be addressed separately
in a follow up patch. Thank you for bringing this to our attention.

> >  	inode_unlock(inode);
> >  	return ret;
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1