Re: [PATCH bpf-next v3 4/5] bpf: simplify the bpf_is_reg64()

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] bpf_is_reg64() fails to identify BPF_ADDR_SPACE_CAST operations with imm == 1U << 16 as 32-bit when the arena has BPF_F_NO_USER_CONV set, leading to missing zero-extension and possible sandbox escape.
- [Critical] Unconditional overwrite of `zext_dst` destroys manual overrides for `PTR_TO_ARENA` arithmetic, leading to missing zero-extensions and arbitrary kernel memory access.
--

--- Patch [4]: [PATCH bpf-next v3 4/5] bpf: simplify the bpf_is_reg64() ---
commit 4ece654ab9bc1fb0f5d2f2cc247cae7c94f2cb89
Author: Eduard Zingerman <[email protected]>

bpf: simplify the bpf_is_reg64()

This commit simplifies bpf_is_reg64() by removing unused parameters,
removing BPF_JMP instructions handling, and streamlining conditions. It
also moves the function to kernel/bpf/fixups.c.

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index 6125598d16a84..7b3510d46b377 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c

[ ... ]

> @@ -44,6 +44,49 @@ static int insn_def_regno(const struct bpf_insn *insn)
>  	}
>  }
>  
> +/*
> + * For use only in combination with insn_def_regno() >= 0.
> + * Returns TRUE if the destination register operates on 64-bit,
> + * otherwise return FALSE.
> + */
> +static bool bpf_is_reg64(struct bpf_insn *insn)
> +{
> +	u8 class = BPF_CLASS(insn->code);
> +	u8 mode = BPF_MODE(insn->code);
> +	u8 size = BPF_SIZE(insn->code);
> +	u8 op = BPF_OP(insn->code);
> +	bool mode_mem;
> +
> +	/* subregister endiness swap */
> +	if ((class == BPF_ALU || class == BPF_ALU64) && op == BPF_END && insn->imm != 64)
> +		return false;
> +
> +	/* w0 += 1 */
> +	if (class == BPF_ALU && op != BPF_END)
> +		return false;
> +
> +	/* cast from as(1) to as(0), PTR_TO_ARENA is 32-bit */
> +	if (insn->code == (BPF_ALU64 | BPF_MOV | BPF_X) &&
> +	    insn->off == BPF_ADDR_SPACE_CAST && insn->imm == 1)
> +		return false;

[Severity: Critical]
Does this logic miss identifying BPF_ADDR_SPACE_CAST operations with
imm == 1U << 16 as 32-bit?

If the arena has BPF_F_NO_USER_CONV set, bpf_do_misc_fixups() rewrites
all BPF_ADDR_SPACE_CAST instructions (including imm == 1U << 16) into
a 32-bit MOV to explicitly clear the upper 32 bits.

However, since this falls through and evaluates to 64-bit, the static
liveness analysis evaluates zext_dst to false. When
bpf_opt_subreg_zext_lo32_rnd_hi32() runs, it skips inserting a
BPF_ZEXT_REG zero-extension.

Could this leave the upper 32 bits un-zeroed on architectures requiring
explicit zero-extension for 32-bit operations (e.g., s390x) or that
sign-extend them (e.g., riscv64), leading to out-of-bounds memory accesses?

> +
> +	/* non 64-bit, non signed extended loads */
> +	mode_mem = mode == BPF_MEM || mode == BPF_PROBE_MEM || mode == BPF_PROBE_MEM32;
> +	if (class == BPF_LDX && mode_mem && size != BPF_DW)
> +		return false;
> +
> +	/* atomics, see insn_def_regno() */
> +	if (class == BPF_STX && size != BPF_DW)
> +		return false;
> +
> +	/* both LD_IND and LD_ABS return 32-bit data. */
> +	if (class == BPF_LD && (mode == BPF_IND || mode == BPF_ABS))
> +		return false;
> +
> +	/* Conservatively return true at default. */
> +	return true;
> +}
> +
> +/*
> + * Return the 32-bit subregister defined by INSN, or -1 if INSN does not
> + * explicitly define a 32-bit value.
> + */
> +int bpf_insn_def32(struct bpf_insn *insn)
> +{
> +	int dst_reg = insn_def_regno(insn);
> +
> +	if (dst_reg < 0 || bpf_is_reg64(insn))
> +		return -1;

[Severity: Critical]
Would this unconditional behavior inadvertently overwrite manual overrides
for PTR_TO_ARENA arithmetic?

During verifier simulation, adjust_reg_min_max_vals() manually forces
aux->zext_dst = true on arena arithmetic to ensure zero-extension is
applied by the JIT.

However, the static liveness pass in bpf_compute_live_registers() runs
afterwards and does an unconditional assignment based on def32. Because
bpf_is_reg64() evaluates 64-bit ALU operations as 64-bit, def32 here
becomes -1.

Could this cause the liveness pass to blindly clear the manually set
zext_dst flag to false? If bpf_do_misc_fixups() then rewrites the
instruction to a 32-bit operation, it might miss the necessary zero
extensions, potentially allowing an escape from the arena boundaries.

> +
> +	return dst_reg;
> +}

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