Re: [PATCH v2 bpf-next 1/2] bpf: enforce VFS constraints for xattr related BPF kfuncs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] The unlocked BPF kfuncs `bpf_set_dentry_xattr` and `bpf_remove_dentry_xattr` unconditionally call `inode_lock(inode)` without checking if the inode is NULL, leading to a kernel panic.
--

commit 1eddd22ded9c4ecb43cf15dc45c01aaba4a3f609
Author: Matt Bobrowski <mattbobrowski@google.com>

bpf: enforce VFS constraints for xattr related BPF kfuncs

Enforce VFS constraints and semantics regarding name and value lengths
within the xattr related BPF kfuncs. Specifically, reject names that
are empty or longer than XATTR_NAME_MAX, and values larger than
XATTR_SIZE_MAX. Also validate the supplied flags to ensure that only
XATTR_CREATE and XATTR_REPLACE can be used alongside the default flag
value 0.

Fixes: 56467292794b ("bpf: fs/xattr: Add BPF kfuncs to set and remove xattrs")
Closes: https://lore.kernel.org/bpf/20260429221005.6D1C6C19425@smtp.kernel.org/

> diff --git a/fs/bpf_fs_kfuncs.c b/fs/bpf_fs_kfuncs.c
> index 9d27be058494..eb9aef1e135e 100644
> --- a/fs/bpf_fs_kfuncs.c
> +++ b/fs/bpf_fs_kfuncs.c

[ ... ]

> @@ -274,11 +315,19 @@ __bpf_kfunc_start_defs();
>   * @value_p: xattr value
>   * @flags: flags to pass into filesystem operations
>   *
> - * Set xattr *name__str* of *dentry* to the value in *value_ptr*.
> + * Set xattr *name__str* of *dentry* to the value in *value_p*.
>   *
>   * For security reasons, only *name__str* with prefix "security.bpf."
>   * is allowed.
>   *
> + * The length constraints imposed on both the xattr name and value abide those
> + * that are also enforced by the VFS. Additionally, the flags argument respects
> + * what's enforced by the VFS in the same way. By default, the flag value of 0
> + * is permitted and an xattr will be created if it does not exist, or the value
> + * will be replaced if the xattr already exists. More course grained control
> + * over these exact semantics is permitted by explicitly specifying either
> + * XATTR_CREATE or XATTR_REPLACE.
> + *
>   * The caller has not locked dentry->d_inode.
>   *
>   * Return: 0 on success, a negative value on error.

This isn't a regression introduced by this patch, but there is a pre-existing
flaw when bpf_set_dentry_xattr and bpf_remove_dentry_xattr call inode_lock
unconditionally.

Can sleepable BPF LSM hooks such as bpf_lsm_inode_create, bpf_lsm_inode_mkdir,
and bpf_lsm_inode_mknod receive negative dentries where d_inode is NULL?

If so, since these hooks are not in the d_inode_locked_hooks list, the BPF
verifier would allow programs attached to them to call the unlocked kfuncs
bpf_set_dentry_xattr and bpf_remove_dentry_xattr. When called with a negative
dentry, the kfuncs extract a NULL inode via d_inode(dentry) and immediately
invoke inode_lock(inode).

This results in down_write(&NULL->i_rwsem), triggering a fatal NULL pointer
dereference. The WARN_ON(!inode) safety check located later in
bpf_xattr_write_permission is unreachable because the system crashes earlier
during lock acquisition.

Could we add a NULL check for the inode before attempting to lock it?

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