Re: [PATCH bpf-next v2 3/5] bpf: infer zext_dst based on static register liveness analysis

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Dynamically downgraded 64-bit arena operations and pointer casts are missing explicit zero-extension on architectures requiring it.

Pre-existing issues:
- [High] Kfuncs bpf_mem_cgroup_memory_events and bpf_mem_cgroup_vm_events do not properly validate negative enum values, leading to out-of-bounds array reads.
--

--- Patch [3]: [PATCH bpf-next v2 3/5] bpf: infer zext_dst based on static register liveness analysis ---
commit 625c020066cfbb5a384d20ff5e1ad79faa898949
Author: Eduard Zingerman <[email protected]>

bpf: infer zext_dst based on static register liveness analysis

This commit replaces the verifier's subreg_def based logic with computing
zext_dst statically as part of bpf_compute_live_registers(). It tracks the
liveness of the upper and lower halves of registers separately to emit explicit
zero extensions on architectures that need them.

Link: https://lore.kernel.org/bpf/CAGKGUv=sOuqQtA1Ub-5JXfA4FPosJFYKAQE4B79cK+P1erxqtg@mail.gmail.com/

[ ... ]

> diff --git a/kernel/bpf/liveness.c b/kernel/bpf/liveness.c
> index d0bd94c60fee2..77b84b367be74 100644
> --- a/kernel/bpf/liveness.c
> +++ b/kernel/bpf/liveness.c
> @@ -2316,9 +2317,18 @@ int bpf_compute_live_registers(struct bpf_verifier_env *env)
>  	}
>  
>  	for (i = 0; i < insn_cnt; ++i) {
> +		int def32 = bpf_insn_def32(&insns[i]);
> +		u32 out = state[i].out;
>  		u32 in = state[i].in;
>  
>  		insn_aux[i].live_regs_before = mask_lo(in) | mask_hi(in);
> +		/*
> +		 * On architectures where 32-bit operations do not reset upper halves
> +		 * of the registers, the verifier needs to zero extend a destination
> +		 * register if an instruction defines a 32-bit subregister and the
> +		 * upper half of that register is alive after the instruction.
> +		 */
> +		insn_aux[i].zext_dst = def32 >= 0 && (mask_hi(out) & BIT(def32));
>  	}

[Severity: High]
Does this statically evaluate zext_dst before some instructions are dynamically
downgraded?

If an ALU64 operation with a PTR_TO_ARENA operand is statically evaluated
here, zext_dst evaluates to false because it is still a 64-bit instruction.

Later during verification, adjust_reg_min_max_vals() sets needs_zext = true.
Then bpf_do_misc_fixups() downgrades the instruction in-place to a 32-bit ALU
operation:

kernel/bpf/fixups.c:bpf_do_misc_fixups() {
    ...
    if (env->insn_aux_data[i + delta].needs_zext)
        /* Convert BPF_CLASS(insn->code) == BPF_ALU64 to 32-bit ALU */
        insn->code = BPF_ALU | BPF_OP(insn->code) | BPF_SRC(insn->code);
    ...
}

Since this bypasses bpf_patch_insn_data(), zext_dst remains false. Could this
cause bpf_opt_subreg_zext_lo32_rnd_hi32() to skip injecting the required zero
extension, leaving garbage in the upper 32 bits on architectures like s390x
and potentially leading to out-of-bounds accesses?

[ ... ]

[Severity: High]
This is a pre-existing issue, but I noticed a potential out-of-bounds array
read in some memcg kfuncs due to missing negative value checks on enums.

In mm/bpf_memcontrol.c:bpf_mem_cgroup_memory_events():

    if (unlikely(event >= MEMCG_NR_MEMORY_EVENTS))
        return (unsigned long)-1;

    return atomic_long_read(&memcg->memory_events[event]);

Because the BPF verifier treats the event enum as a signed integer and doesn't
check enum ranges, can a BPF program pass a negative value to bypass this
check (e.g., -1 >= MEMCG_NR_MEMORY_EVENTS evaluates to false) and cause an
out-of-bounds read?

Similarly in mm/memcontrol.c:memcg_vm_event_item_valid():

    if (idx >= NR_VM_EVENT_ITEMS)
        return false;

    return !BAD_STAT_IDX(memcg_events_index(idx));

Could a negative idx bypass this bounds check and cause an out-of-bounds
access inside memcg_events_index()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3