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

On Sat, 2026-08-01 at 01:42 +0000, [email protected] wrote:
> 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?

Yeap, need to figure something out.

...