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 issue(s) to consider:
> - [High] `bpf_is_reg64()` incorrectly identifies 32-bit `BPF_PROBE_MEM` and `BPF_PROBE_MEM32` loads as 64-bit defining instructions, bypassing explicit zero-extensions.
> - [Low] Stale comment referencing a deleted parameter of `bpf_is_reg64`.
> --
> 
> --- Patch [4]: [PATCH bpf-next v2 4/5] bpf: simplify the bpf_is_reg64() ---
> commit ee97fd37754941b6dd81dccf5f68e7f82c74ea6d
> Author: Eduard Zingerman <[email protected]>
> 
> bpf: simplify the bpf_is_reg64()
> 
> 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.
> 
> > 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 *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);
> [ ... ]
> > +	/* non 64-bit loads */
> > +	if (class == BPF_LDX && mode == BPF_MEM && size != BPF_DW)
> > +		return false;
> 
> [Severity: High]
> Does this condition miss BPF_PROBE_MEM and BPF_PROBE_MEM32 loads?
> 
> Because this check strictly matches the BPF_MEM mode, it seems these other
> 32-bit load modes would bypass this check and fall through to returning true.
> 
> 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, potentially
> 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 are
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 == BPF_STX && size != BPF_DW)
> > +		return false;
> [ ... ]
> > @@ -623,7 +659,7 @@ int bpf_opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
> >  			 *       BPF_STX + SRC_OP, so it is safe to pass NULL
> >  			 *       here.
> >  			 */
> > -			if (bpf_is_reg64(&insn, load_reg, NULL, DST_OP)) {
> > +			if (bpf_is_reg64(&insn)) {
> 
> [Severity: Low]
> This isn't a bug, but should the comment directly above this call be removed
> as well? 
> 
> It references a fourth parameter that has been deleted by this patch, which
> could be confusing for future readers.
> 
> >  				if (class == BPF_LD &&
> >  				    BPF_MODE(code) == BPF_IMM)
> >  					i++;