Re: [PATCH bpf-next 1/3] bpf: Add simple xattr support to bpffs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unprivileged users can exhaust kernel memory via unbounded user.* extended attributes on bpffs.
- [High] NULL pointer dereference in bpf_fs_initxattrs() if an LSM returns a NULL xattr value.
--

commit 57543eec66ace3d4d002256a01cff4f1797f61e2
Author: Daniel Borkmann <daniel@iogearbox.net>

bpf: Add simple xattr support to bpffs

This commit adds support for extended attributes on bpffs inodes, allowing user
space and BPF LSM programs to attach metadata to pinned objects or directories.
The xattr store is in-memory and lives only for the lifetime of the mount.

> diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
> index 25c06a0118258..638aa996cbaf4 100644
> --- a/kernel/bpf/inode.c
> +++ b/kernel/bpf/inode.c

[ ... ]

> @@ -762,22 +821,151 @@ static int bpf_show_options(struct seq_file *m, struct dentry *root)
>  	return 0;
>  }

[ ... ]

> +static int bpf_fs_xattr_set(const struct xattr_handler *handler,
> +			    struct mnt_idmap *idmap, struct dentry *unused,
> +			    struct inode *inode, const char *name,
> +			    const void *value, size_t size, int flags)
> +{
> +	struct bpf_fs_inode *bi = BPF_FS_I(inode);
> +	struct simple_xattrs *xattrs;
> +	struct simple_xattr *old;
> +
> +	name = xattr_full_name(handler, name);
> +	xattrs = simple_xattrs_lazy_alloc(&bi->xattrs, value, flags);
> +	if (IS_ERR_OR_NULL(xattrs))
> +		return PTR_ERR(xattrs);
> +	old = simple_xattr_set(xattrs, name, value, size, flags);

[Severity: High]
Does this code allow an unprivileged user to exhaust kernel memory?

Because bpffs can be mounted in an unprivileged user namespace
(FS_USERNS_MOUNT), a user could create directories and repeatedly set user.*
extended attributes.

Since simple_xattr_set() does not enforce per-inode limits like
simple_xattr_set_limited() does, couldn't this lead to an unbounded memory
allocation and soft lockups during rhashtable traversals?

> +	if (IS_ERR(old))
> +		return PTR_ERR(old);
> +	simple_xattr_free_rcu(old);
> +	inode_set_ctime_current(inode);
> +	return 0;
> +}

[ ... ]

> +static int bpf_fs_initxattrs(struct inode *inode,
> +			     const struct xattr *xattr_array, void *fs_info)
> +{
> +	struct bpf_fs_inode *bi = BPF_FS_I(inode);
> +	const struct xattr *xattr;
> +	size_t len;
> +
> +	CLASS(simple_xattrs, xattrs)();
> +	if (IS_ERR(xattrs))
> +		return PTR_ERR(xattrs);
> +
> +	for (xattr = xattr_array; xattr->name != NULL; xattr++) {
> +		CLASS(simple_xattr, new_xattr)(xattr->value, xattr->value_len);
> +		if (IS_ERR(new_xattr))
> +			break;
> +
> +		len = strlen(xattr->name) + 1;
> +		new_xattr->name = kmalloc(XATTR_SECURITY_PREFIX_LEN + len,

[Severity: High]
Can this cause a NULL pointer dereference?

If an LSM program returns a NULL xattr value, the simple_xattr class
constructor will call simple_xattr_alloc() which can return NULL.

The IS_ERR(new_xattr) check evaluates to false for a NULL pointer, leading
to a NULL pointer dereference when accessing new_xattr->name here.

> +					  GFP_KERNEL_ACCOUNT);
> +		if (!new_xattr->name)
> +			break;
> +
> +		memcpy(new_xattr->name, XATTR_SECURITY_PREFIX,
> +		       XATTR_SECURITY_PREFIX_LEN);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522135626.120732-1-daniel@iogearbox.net?part=1