Re: [PATCH bpf-next v1 03/10] bpf: Turn loads of __arena pointer members into arena pointers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: arena_member fixup corrupts PTR_TO_MEM scalar loads
--

commit 5458ef081d9f29cc60192d4b6fde6d8fd3989b75
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Turn loads of __arena pointer members into arena pointers

This commit translates loads of __arena pointer members in kernel structs
into arena pointers in the verifier. It appends a fixup to subtract the
kernel's virtual memory start address from the loaded value, and adds logic
to ensure paths disagreeing on member types are rejected.

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index b2fa7e8bedf3a..bbb653df6b6f0 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c
[ ... ]
> @@ -870,6 +870,36 @@ int bpf_opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
>   *     struct __sk_buff    -> struct sk_buff
>   *     struct bpf_sock_ops -> struct sock
>   */
> +/*
> + * A load through an __arena pointer member of a kernel struct reads the kernel
> + * address of arena memory. Append the conversion to the arena pointer form,
> + * (u32)(kaddr - kern_vm_start), that the struct_ops trampoline applies to
> + * __arena arguments. A __nullable member keeps NULL as NULL, tested on the
> + * full kernel address. Returns the number of insns in @buf, 0 when the insn
> + * loads no such member, or an error when it does so on some paths only.
> + */
> +static int arena_member_fixup(struct bpf_verifier_env *env, int idx, struct bpf_insn *insn,
> +			      struct bpf_insn *buf)
> +{
> +	struct bpf_insn_aux_data *aux = &env->insn_aux_data[idx];
> +	int cnt = 0;
> +	u32 base;
> +
> +	if (!aux->arena_member)
> +		return 0;
> +	if (aux->non_arena_member) {
> +		verbose(env, "an insn loads an arena pointer member on one path and a different member on another\n");
> +		return -EINVAL;
> +	}

[Severity: High]
Because non_arena_member might not be set by the PTR_TO_MEM path (as noted 
below), could this check fail to reject the mismatch?

If it fails to reject, arena_member_fixup() blindly appends a
SUB kern_vm_start instruction to the shared load. At runtime, if the
PTR_TO_MEM path executes, its scalar load would be corrupted by the
subtraction, potentially leading to out-of-bounds access or data corruption
if the result is used as an index or offset.

> +
> +	base = bpf_arena_get_kern_vm_start(env->prog->aux->arena);
> +	buf[cnt++] = *insn;
> +	if (aux->arena_member_nullable)
> +		buf[cnt++] = BPF_JMP_IMM(BPF_JEQ, insn->dst_reg, 0, 1);
> +	buf[cnt++] = BPF_ALU32_IMM(BPF_SUB, insn->dst_reg, (s32)base);
> +	return cnt;
> +}
[ ... ]
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 70fd44a6e1c56..a24a2bab802b3 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
[ ... ]
> @@ -6661,6 +6697,11 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
>  	if (ret < 0)
>  		return ret;
>  
> +	if (ret == PTR_TO_ARENA)
> +		return check_arena_member_load(env, regs, value_regno, flag, tname, field_name);
> +	if (atype == BPF_READ && value_regno >= 0)
> +		env->insn_aux_data[env->insn_idx].non_arena_member = true;

[Severity: High]
Does this code miss setting the conflict flag for non-BTF pointers? 

The verifier's save_aux_ptr_type() permits sharing an instruction across 
branches where one path uses PTR_TO_BTF_ID and the other uses PTR_TO_MEM
(by merging them via allow_trust_mismatch). 

If the PTR_TO_BTF_ID path reads an __arena member, check_ptr_to_btf_access() 
sets aux->arena_member = true. However, when the PTR_TO_MEM path is
evaluated, check_mem_access() does not set aux->non_arena_member = true for 
memory pointers.

> +
>  	if (ret != PTR_TO_BTF_ID) {
>  		/* just mark; */
>

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