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

On Sun, 2026-08-02 at 21:26 +0000, [email protected] wrote:
> 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.

I think this is a pre-existing bug.
I'll add a test.

...

> > +/*
> > + * 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?

No it won't.
convert_ctx_access() downgrades those operations to BPF_ALU.
There is also a test to check this.

...