bpf: enforce VFS constraints for xattr related BPF kfuncs

[kernel-patches-daemon-bpf]

Pull request for series with
subject: bpf: enforce VFS constraints for xattr related BPF kfuncs
version: 2
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=1089163

[kernel-patches-daemon-bpf]

Upstream branch: 2ca6723
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1089163
version: 2

[kernel-patches-review-bot]

commit a7636112fd4a8754e015421632feb96636e89334
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

[ ... ]

> @@ -117,10 +132,10 @@ static int bpf_xattr_read_permission(const char *name, struct inode *inode)
>   * @name__str: name of the xattr
>   * @value_p: output buffer of the xattr value
>   *
> - * Get xattr *name__str* of *dentry* and store the output in *value_ptr*.
> + * Get xattr *name__str* of *dentry* and store the output in *value_p*.
>   *
> - * For security reasons, only *name__str* with prefixes "user." or
> - * "security.bpf." are allowed.
> + * For security reasons, only *name__str* values prefixed with "user." or
> + * "security.bpf." are permitted.
>   *
>   * Return: length of the xattr value on success, a negative value on error.
>   */
> @@ -133,6 +148,10 @@ __bpf_kfunc int bpf_get_dentry_xattr(struct dentry *dentry, const char *name__st
>  	void *value;
>  	int ret;
>
> +	ret = bpf_xattr_validate_name(name__str);
> +	if (ret)
> +		return ret;
> +
>  	value_len = __bpf_dynptr_size(value_ptr);
>  	value = __bpf_dynptr_data_rw(value_ptr, value_len);
>  	if (!value)

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. Looking at bpf_xattr_read_permission():

static int bpf_xattr_read_permission(const char *name, struct inode *inode)
{
	if (WARN_ON(!inode))
		return -ENOENT;
    ...

If a BPF program passes a negative dentry, this would intentionally trigger
the warning.

Could this panic the system if panic_on_warn is configured? Should this
gracefully return -ENOENT or -EINVAL without firing WARN_ON()?

Reference: https://lore.kernel.org/bpf/20260503204353.2F800C2BCB4@smtp.kernel.org/

[ ... ]

> @@ -187,10 +206,18 @@ static int bpf_xattr_write_permission(const char *name, struct inode *inode)
>   * @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.
> + * For security reasons, only *name__str* values prefixed with "security.bpf."
> + * are permitted.
> + *
> + * 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 already locked dentry->d_inode.
>   *

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:

static int bpf_xattr_write_permission(const char *name, struct inode *inode)
{
    ...
	return inode_permission(&nop_mnt_idmap, inode, MAY_WRITE);
}

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?

Reference: https://lore.kernel.org/bpf/20260503204353.2F800C2BCB4@smtp.kernel.org/

> @@ -206,7 +233,17 @@ int bpf_set_dentry_xattr_locked(struct dentry *dentry, const char *name__str,
>  	u32 value_len;
>  	int ret;
>
> +	if (flags & ~(XATTR_CREATE | XATTR_REPLACE))
> +		return -EINVAL;
> +
> +	ret = bpf_xattr_validate_name(name__str);
> +	if (ret)
> +		return ret;
> +
>  	value_len = __bpf_dynptr_size(value_ptr);
> +	if (value_len > XATTR_SIZE_MAX)
> +		return -E2BIG;
> +
>  	value = __bpf_dynptr_data(value_ptr, value_len);
>  	if (!value)
>  		return -EINVAL;

[ ... ]

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

BPF LSM programs can attach to hooks that supply negative dentries (e.g.,
inode_create, vfs_mkdir) where d_inode(dentry) is legally NULL.

Looking at bpf_set_dentry_xattr():

__bpf_kfunc int bpf_set_dentry_xattr(struct dentry *dentry, const char *name__str,
				     struct bpf_dynptr *value_p, int flags)
{
	struct inode *inode = d_inode(dentry);
	int ret;

	inode_lock(inode);
    ...

If a BPF program invokes bpf_set_dentry_xattr() on a negative dentry,
inode will be NULL and inode_lock(inode) will unconditionally dereference
the NULL pointer.

Will this trigger a NULL pointer dereference and oops the kernel?

The same issue affects bpf_remove_dentry_xattr().

Reference: https://lore.kernel.org/bpf/20260503204353.2F800C2BCB4@smtp.kernel.org/

[ ... ]

The verifier unconditionally rewrites calls to bpf_set_dentry_xattr() to
bpf_set_dentry_xattr_locked() for hooks that hold inode locks, but doesn't
verify that the dentry argument is the specific context argument that was
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?

Reference: https://lore.kernel.org/bpf/5c447a881915a06db87c17ef1384f42674b0c171858c82e33666cb667101b338@mail.kernel.org/

---

AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

In-Reply-To-Subject: bpf: enforce VFS constraints for xattr related BPF kfuncs
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/25310655349

[kernel-patches-daemon-bpf]

Forwarding comment 4369841351 via email
In-Reply-To: 20260504085428.2865671-1-mattbobrowski@google.com
Patch: https://patchwork.kernel.org/project/netdevbpf/patch/20260504085428.2865671-1-mattbobrowski@google.com/

[kernel-patches-daemon-bpf]

Upstream branch: 7e03354
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1089163
version: 2

[kernel-patches-daemon-bpf]

At least one diff in series https://patchwork.kernel.org/project/netdevbpf/list/?series=1089163 expired. Closing PR.