Re: [PATCH bpf-next 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] Incorrect static liveness tracking for 64-bit conditional jumps leads to missing zero-extensions (`zext_dst`), resulting in verifier/runtime state divergence and potential privilege escalation.
--

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

bpf: infer zext_dst based on static register liveness analysis

This commit replaces the dynamic subreg_def based zero-extension
tracking with static liveness analysis to correctly infer zext_dst
and prevent missing zero-extensions on architectures like s390x.

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 c996b8a03e792..ea002550671a8 100644
> --- a/kernel/bpf/liveness.c
> +++ b/kernel/bpf/liveness.c
> @@ -2317,9 +2318,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: Critical]
Does this code correctly evaluate liveness for 64-bit conditional jumps?

The new zext_dst logic relies on mask_hi(out) accurately reflecting if the
upper half of a register is alive.

However, looking at compute_insn_live_regs() in kernel/bpf/liveness.c for
conditional jumps:

    const u8 class = BPF_CLASS(insn->code);
    const u8 code = BPF_OP(insn->code);
    ...
    case BPF_JMP:
    case BPF_JMP32:
        switch (code) {
        ...
        default:
            def = 0;
            if (BPF_SRC(insn->code) == BPF_K)
                use = dst;
            else
                use = dst | (code == BPF_JMP32 ? src : src32);
        }

Can the code variable ever equal BPF_JMP32?

BPF_OP() extracts the operation code (which is a multiple of 16), while
BPF_JMP32 is a class (0x06). This means the check code == BPF_JMP32 evaluates
to false, and the source register usage always falls back to src32, even for
64-bit jumps (BPF_JMP).

If a 32-bit operation defines a register, and it is followed by a 64-bit
conditional jump using that register as a source with no other uses of the
upper 32 bits, the upper half won't be marked as alive.

As a result, zext_dst wouldn't be set, which could lead to the JIT skipping
the mandatory zero extension. This would cause the 64-bit jump to compare
against uninitialized garbage in the upper bits, potentially allowing
out-of-bounds access.

Should this check use class == BPF_JMP32 instead?

>  	}

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