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

On Sat, 2026-08-01 at 01:34 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issu=
e(s) to consider:
> - [High] `bpf_is_reg64()` incorrectly identifies 32-bit `BPF_PROBE_MEM` a=
nd `BPF_PROBE_MEM32` loads as 64-bit defining instructions, bypassing expli=
cit zero-extensions.
> - [Low] Stale comment referencing a deleted parameter of `bpf_is_reg64`.
> --
>=20
> --- Patch [4]: [PATCH bpf-next v2 4/5] bpf: simplify the bpf_is_reg64() -=
--
> commit ee97fd37754941b6dd81dccf5f68e7f82c74ea6d
> Author: Eduard Zingerman <[email protected]>
>=20
> bpf: simplify the bpf_is_reg64()
>=20
> This patch simplifies the bpf_is_reg64() function by removing unused
> parameters and code paths related to BPF_JMP instructions. It also
> streamlines the condition expressions for identifying 64-bit registers.
>=20
> > diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> > index 6125598d16a84..ac0a388b40218 100644
> > --- a/kernel/bpf/fixups.c
> > +++ b/kernel/bpf/fixups.c
> [ ... ]
> > @@ -44,6 +44,42 @@ 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);
> [ ... ]
> > +	/* non 64-bit loads */
> > +	if (class =3D=3D BPF_LDX && mode =3D=3D BPF_MEM && size !=3D BPF_DW)
> > +		return false;
>=20
> [Severity: High]
> Does this condition miss BPF_PROBE_MEM and BPF_PROBE_MEM32 loads?
>=20
> Because this check strictly matches the BPF_MEM mode, it seems these othe=
r
> 32-bit load modes would bypass this check and fall through to returning t=
rue.
>=20
> Could this cause bpf_insn_def32() to inappropriately return -1, skipping
> necessary zero-extension for architectures that rely on explicit BPF_ZEXT=
_REG
> patching? If so, the upper 32 bits might contain garbage data, potentiall=
y
> leading to out-of-bounds memory accesses if the register is later used in=
 an
> address calculation.

I don't think this is exploitable. Both BPF_PROBE_MEM and BPF_PROBE_MEM32 a=
re
inserted by bpf_convert_ctx_accesses, that directly modify insn->code,
w/o call to bpf_patch_*(). Hence bpf_is_reg64 is not invoked for these
converted instructions second time. Hence the aux->zext_dst flag for such
instructions retains it's original inference.

Nevertheless, I modified condition to include BPF_PROBE_MEM{,32}
to make it future proof.

> > +
> > +	/* atomics, see insn_def_regno() */
> > +	if (class =3D=3D BPF_STX && size !=3D BPF_DW)
> > +		return false;
> [ ... ]
> > @@ -623,7 +659,7 @@ int bpf_opt_subreg_zext_lo32_rnd_hi32(struct bpf_ve=
rifier_env *env,
> > =C2=A0			 *=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 BPF_STX + SRC_OP, so it=
 is safe to pass NULL
> > =C2=A0			 *=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 here.
> > =C2=A0			 */
> > -			if (bpf_is_reg64(&insn, load_reg, NULL, DST_OP)) {
> > +			if (bpf_is_reg64(&insn)) {
>=20
> [Severity: Low]
> This isn't a bug, but should the comment directly above this call be remo=
ved
> as well?=20
>=20
> It references a fourth parameter that has been deleted by this patch, whi=
ch
> could be confusing for future readers.
>=20
> > =C2=A0				if (class =3D=3D BPF_LD &&
> > =C2=A0				=C2=A0=C2=A0=C2=A0 BPF_MODE(code) =3D=3D BPF_IMM)
> > =C2=A0					i++;