Re: [PATCH bpf-next 1/2] bpf: add bpf_init_inode_xattr kfunc for atomic inode labeling

[sashiko-bot@kernel.org]

Sashiko AI review found 4 potential issue(s):
- [Critical] BPF programs can trigger a heap buffer overflow by calling bpf_init_inode_xattr multiple times.
- [Critical] Context spoofing via backward jump leads to arbitrary kernel memory read/write.
- [High] Storing name__str directly into the xattr array leads to a Use-After-Free during filesystem operations.
- [High] Missing prefix validation on the xattr name allows BPF programs to spoof standard security xattrs.
--

commit 40ce6a42efab6a64fca9ea5bd1aec81b4faa93f2
Author: David Windsor <dwindsor@gmail.com>

bpf: add bpf_init_inode_xattr kfunc for atomic inode labeling

Add bpf_init_inode_xattr() kfunc for BPF LSM programs to atomically set
xattrs via inode_init_security hook using lsm_get_xattr_slot().

Link: https://kernsec.org/pipermail/linux-security-module-archive/2022-October/034878.html [1]
Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6bcdfd2cac55 [2]

> diff --git a/fs/bpf_fs_kfuncs.c b/fs/bpf_fs_kfuncs.c
> --- a/fs/bpf_fs_kfuncs.c
> +++ b/fs/bpf_fs_kfuncs.c
[ ... ]
> @@ -353,6 +354,68 @@ __bpf_kfunc int bpf_cgroup_read_xattr(struct cgroup *cgroup, const char *name__s
>  }
>  #endif /* CONFIG_CGROUPS */
>  
> +/* Called from the verifier fixup of bpf_init_inode_xattr(). */
> +__bpf_kfunc int bpf_init_inode_xattr_impl(struct xattr *xattrs, int *xattr_count,
> +					  const char *name__str,
> +					  const struct bpf_dynptr *value_p)
> +{
[ ... ]
> +	name_len = strlen(name__str);
> +	if (name_len == 0 || name_len > XATTR_NAME_MAX)
> +		return -EINVAL;

Does this code need to validate the xattr prefix? Other BPF xattr kfuncs
enforce a prefix like "security.bpf." to prevent interfering with other
security modules. 

Without this validation, could a BPF program pass a name like "selinux" and
inadvertently spoof standard security xattrs?

[ ... ]
> +	xattr = lsm_get_xattr_slot(xattrs, xattr_count);
> +	if (!xattr) {
> +		kfree(xattr_value);
> +		return -ENOSPC;
> +	}
> +
> +	xattr->name = name__str;

Can this result in a use-after-free of the name string? The name__str
argument typically resides in a BPF map. 

Since the filesystem's initxattrs callback can perform disk I/O and sleep
after the BPF hook completes, what happens if the BPF program is unloaded
and its maps are freed concurrently? 

The name pointer might then point to freed memory when the filesystem
attempts to write the xattr.

> +	xattr->value = xattr_value;
> +	xattr->value_len = value_len;
> +
> +	return 0;
> +}
[ ... ]
> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c
[ ... ]
> @@ -725,6 +725,26 @@ int bpf_convert_ctx_accesses(struct bpf_verifier_env *env)
>  		}
>  	}
>  
> +	if (env->needs_ctx_spill) {
> +		if (epilogue_cnt) {
> +			/* gen_epilogue already saved ctx to the stack */
> +			env->ctx_stack_off = -(s16)subprogs[0].stack_depth;
> +		} else {
> +			cnt = 0;
> +			subprogs[0].stack_depth += 8;
> +			env->ctx_stack_off = -(s16)subprogs[0].stack_depth;
> +			insn_buf[cnt++] = BPF_STX_MEM(BPF_DW, BPF_REG_FP,
> +						      BPF_REG_1,
> +						      env->ctx_stack_off);
> +			insn_buf[cnt++] = env->prog->insnsi[0];

Is it safe to prepend the context spill instruction at the beginning of the
program? Because the original BPF program does not use R1, the verifier's
liveness analysis will mark R1 as dead at the start of the program.

If a BPF program writes a user-controlled pointer into R1 and creates a
bounded loop jumping back to the start of the program, could it trick the
verifier into accepting the jump and saving a fake context to the stack slot? 

This could allow the kfunc fixup to extract xattrs and xattr_count from an
attacker-controlled pointer, potentially leading to arbitrary kernel memory
reads or writes.

> +			new_prog = bpf_patch_insn_data(env, 0, insn_buf, cnt);
> +			if (!new_prog)
> +				return -ENOMEM;
[ ... ]
> diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
> --- a/security/bpf/hooks.c
> +++ b/security/bpf/hooks.c
[ ... ]
> @@ -28,8 +28,11 @@ static int __init bpf_lsm_init(void)
>  	return 0;
>  }
>  
> +#define BPF_LSM_INODE_INIT_XATTRS 1
> +
>  struct lsm_blob_sizes bpf_lsm_blob_sizes __ro_after_init = {
>  	.lbs_inode = sizeof(struct bpf_storage_blob),
> +	.lbs_xattr_count = BPF_LSM_INODE_INIT_XATTRS,
>  };

Because lbs_xattr_count is set to 1, security_inode_init_security() will
allocate exactly 1 xattr slot for BPF LSMs.

Since a BPF program could call bpf_init_inode_xattr() multiple times, and
lsm_get_xattr_slot() increments the slot index without bounds checking,
could multiple calls write past the end of the new_xattrs heap array?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260427001602.38353-1-dwindsor@gmail.com?part=1