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

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.

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

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