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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Missing zero-extension for BPF_ADDR_SPACE_CAST allows out-of-bounds arena access on architectures needing explicit zero-extension.
--

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

bpf: infer zext_dst based on static register liveness analysis

This patch replaces dynamic subreg_def tracking with a static computation of 
zext_dst during register liveness analysis. The verifier now tracks the usage 
of upper and lower register halves separately to infer whether a 32-bit 
subregister definition requires explicit zero extension.

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

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 896bc5aaaaaff..e51cf5238f60a 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
[ ... ]
> @@ -15026,18 +14941,14 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
>  				if (insn->imm) {
>  					/* off == BPF_ADDR_SPACE_CAST */
>  					mark_reg_unknown(env, regs, insn->dst_reg);
> -					if (insn->imm == 1) { /* cast from as(1) to as(0) */
> +					if (insn->imm == 1) /* cast from as(1) to as(0) */
>  						dst_reg->type = PTR_TO_ARENA;
> -						/* PTR_TO_ARENA is 32-bit */
> -						dst_reg->subreg_def = env->insn_idx + 1;
> -					}

[Severity: Critical]
Does this missing zero-extension for BPF_ADDR_SPACE_CAST allow out-of-bounds
arena access on architectures needing explicit zero-extension?

The patch removed the dynamic dst_reg->subreg_def = env->insn_idx + 1
assignment here. Since a BPF_ADDR_SPACE_CAST instruction is encoded as
BPF_ALU64 | BPF_MOV | BPF_X, the new bpf_insn_def32() function returns -1,
which evaluates zext_dst to false in the static liveness analysis.

Because check_alu_op() handles the cast but does not manually set
aux->zext_dst = true (unlike how it was handled in adjust_reg_min_max_vals()),
the subsequent zero-extension insertion pass is skipped.

On architectures like s390x, wouldn't this leave uninitialized garbage in the
upper 32 bits, allowing arbitrary kernel memory read/writes when the JIT adds
this register to the 64-bit arena base?

>  				} else if (insn->off == 0) {
>  					/* case: R1 = R2
>  					 * copy register state to dest reg
>  					 */

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