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 issu=
e(s) to consider:
> - [Critical] bpf_is_reg64() fails to identify BPF_ADDR_SPACE_CAST operati=
ons with imm =3D=3D 1U << 16 as 32-bit when the arena has BPF_F_NO_USER_CON=
V set, leading to missing zero-extension and possible sandbox escape.
> - [Critical] Unconditional overwrite of `zext_dst` destroys manual overri=
des for `PTR_TO_ARENA` arithmetic, leading to missing zero-extensions and a=
rbitrary kernel memory access.
> --
>=20
> --- Patch [4]: [PATCH bpf-next v3 4/5] bpf: simplify the bpf_is_reg64() -=
--
> commit 4ece654ab9bc1fb0f5d2f2cc247cae7c94f2cb89
> Author: Eduard Zingerman <[email protected]>
>=20
> bpf: simplify the bpf_is_reg64()
>=20
> 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.
>=20
> > 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
>=20
> [ ... ]
>=20
> > @@ -44,6 +44,49 @@ static int insn_def_regno(const struct bpf_insn *ins=
n)
> > =C2=A0	}
> > =C2=A0}
> > =C2=A0
> > +/*
> > + * For use only in combination with insn_def_regno() >=3D 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 =3D BPF_CLASS(insn->code);
> > +	u8 mode =3D BPF_MODE(insn->code);
> > +	u8 size =3D BPF_SIZE(insn->code);
> > +	u8 op =3D BPF_OP(insn->code);
> > +	bool mode_mem;
> > +
> > +	/* subregister endiness swap */
> > +	if ((class =3D=3D BPF_ALU || class =3D=3D BPF_ALU64) && op =3D=3D BPF=
_END && insn->imm !=3D 64)
> > +		return false;
> > +
> > +	/* w0 +=3D 1 */
> > +	if (class =3D=3D BPF_ALU && op !=3D BPF_END)
> > +		return false;
> > +
> > +	/* cast from as(1) to as(0), PTR_TO_ARENA is 32-bit */
> > +	if (insn->code =3D=3D (BPF_ALU64 | BPF_MOV | BPF_X) &&
> > +	=C2=A0=C2=A0=C2=A0 insn->off =3D=3D BPF_ADDR_SPACE_CAST && insn->imm =
=3D=3D 1)
> > +		return false;
>=20
> [Severity: Critical]
> Does this logic miss identifying BPF_ADDR_SPACE_CAST operations with
> imm =3D=3D 1U << 16 as 32-bit?
>=20
> If the arena has BPF_F_NO_USER_CONV set, bpf_do_misc_fixups() rewrites
> all BPF_ADDR_SPACE_CAST instructions (including imm =3D=3D 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 n=
ot
> > + * explicitly define a 32-bit value.
> > + */
> > +int bpf_insn_def32(struct bpf_insn *insn)
> > +{
> > +	int dst_reg =3D insn_def_regno(insn);
> > +
> > +	if (dst_reg < 0 || bpf_is_reg64(insn))
> > +		return -1;
>=20
> [Severity: Critical]
> Would this unconditional behavior inadvertently overwrite manual override=
s
> 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.

...