Re: [PATCH 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 4 potential issue(s) to consider:
- [High] bpf_set_dentry_xattr and bpf_remove_dentry_xattr will crash the kernel with a NULL pointer dereference if passed a negative dentry.
- [High] bpf_xattr_write_permission() bypasses IS_APPEND() checks, allowing BPF programs to modify xattrs on append-only files.
- [High] bpf_xattr_read_permission and bpf_xattr_write_permission use WARN_ON(!inode), allowing BPF programs to trivially spam the kernel log or trigger panics.
- [Medium] bpf_cgroup_read_xattr() misses the new VFS name length constraint check.
--

commit 6ac57f528df35ba363e277e2447059faac6477b7
Author: Matt Bobrowski <mattbobrowski@google.com>

bpf: enforce VFS constraints for xattr related BPF kfuncs

This commit enforces VFS constraints and semantics regarding name and value
lengths within the xattr related BPF kfuncs. It rejects names that are empty
or longer than XATTR_NAME_MAX, and values larger than XATTR_SIZE_MAX. It also
validates the supplied flags to ensure only XATTR_CREATE and XATTR_REPLACE can
be used alongside the default flag value 0.

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..a48cf35dedd2 100644
> --- a/fs/bpf_fs_kfuncs.c
> +++ b/fs/bpf_fs_kfuncs.c

[ ... ]

> @@ -117,10 +132,10 @@ static int bpf_xattr_read_permission(const char *name, struct inode *inode)

This wasn't introduced by this patch, but do bpf_xattr_read_permission() and
bpf_xattr_write_permission() use WARN_ON(!inode), allowing BPF programs to
trivially spam the kernel log or trigger panics?

BPF LSM programs can attach to hooks that inherently supply negative dentries
(like vfs_mkdir), where d_inode(dentry) is legally NULL. Passing a negative
dentry would intentionally trigger the warning in these permission checks:

static int bpf_xattr_write_permission(const char *name, struct inode *inode)
{
	if (WARN_ON(!inode))
		return -EINVAL;

This could panic the system if panic_on_warn is configured. Should these checks
gracefully return -ENOENT or -EINVAL without firing a WARN_ON()?

>   * @name__str: name of the xattr
>   * @value_p: output buffer of the xattr value
>   *

[ ... ]

> @@ -187,10 +206,18 @@ static int bpf_xattr_write_permission(const char *name, struct inode *inode)

This wasn't introduced by this patch, but does bpf_xattr_write_permission()
bypass IS_APPEND() checks, allowing BPF programs to modify xattrs on
append-only files?

Standard VFS xattr modifications enforce restrictions via xattr_permission(),
which explicitly checks IS_IMMUTABLE(inode) || IS_APPEND(inode) to block
metadata tampering on append-only files. However, bpf_xattr_write_permission()
bypasses xattr_permission() and calls inode_permission(..., MAY_WRITE)
directly.

Because inode_permission() checks IS_IMMUTABLE but not IS_APPEND, could BPF
programs be erroneously permitted to modify security.bpf.* xattrs on files
marked as append-only?

>   * @value_p: xattr value
>   * @flags: flags to pass into filesystem operations
>   *

[ ... ]

> @@ -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*.

This problem wasn't introduced by this patch, but will bpf_set_dentry_xattr()
and bpf_remove_dentry_xattr() crash the kernel with a NULL pointer dereference
if passed a negative dentry?

BPF LSM programs can attach to hooks that inherently supply negative dentries
(such as inode_create or vfs_mkdir), where d_inode(dentry) is legally NULL. If
a BPF program invokes bpf_set_dentry_xattr() or bpf_remove_dentry_xattr() on
such a negative dentry, the kfunc will assign inode = NULL and subsequently
call inode_lock(inode).

Because inode_lock() unconditionally dereferences the inode pointer, would this
immediately trigger a NULL pointer dereference and oops the kernel?

>   *
>   * For security reasons, only *name__str* with prefix "security.bpf."
>   * is allowed.

[ ... ]

> @@ -327,14 +376,15 @@ __bpf_kfunc int bpf_remove_dentry_xattr(struct dentry *dentry, const char *name_
>   * @name__str: name of the xattr
>   * @value_p: output buffer of the xattr value
>   *
> - * Get xattr *name__str* of *cgroup* and store the output in *value_ptr*.
> + * Get xattr *name__str* of *cgroup* and store the output in *value_p*.
>   *
> - * For security reasons, only *name__str* with prefix "user." is allowed.
> + * For security reasons, only *name__str* values prefixed with "user." are
> + * permitted.
>   *
>   * Return: length of the xattr value on success, a negative value on error.
>   */
>  __bpf_kfunc int bpf_cgroup_read_xattr(struct cgroup *cgroup, const char *name__str,
> -					struct bpf_dynptr *value_p)
> +				      struct bpf_dynptr *value_p)
>  {
>  	struct bpf_dynptr_kern *value_ptr = (struct bpf_dynptr_kern *)value_p;
>  	u32 value_len;

Does bpf_cgroup_read_xattr() miss the new VFS name length constraint check?

The commit message explicitly states the goal is to "Enforce VFS constraints
and semantics regarding name and value lengths within the xattr related BPF
kfuncs", specifically rejecting names that are empty or longer than
XATTR_NAME_MAX.

While bpf_xattr_validate_name() was correctly added to the dentry-related
kfuncs like bpf_get_dentry_xattr() and bpf_set_dentry_xattr(), it appears to
be completely omitted in bpf_cgroup_read_xattr(). Could this allow BPF
programs to bypass the intended length constraints when reading cgroup xattrs?

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