Re: [PATCH bpf-next v5 01/10] bpf: propagate original instruction offset when patching program

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Incorrect `insn_off_in_patch` offset passed during instruction rewrites for pointer arithmetic, may_goto, and LD_ABS/LD_IND.
--

--- Patch [1]: [PATCH bpf-next v5 01/10] bpf: propagate original instruction offset when patching program ---
commit 93b475f9be8b7e4db55e416665410ec667566b5a
Author: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>

bpf: propagate original instruction offset when patching program

This commit updates the BPF verifier to propagate the original instruction's
offset within a patch when calling bpf_patch_insn_data and
adjust_insn_aux_data. This information is needed to correctly map auxiliary
instruction data (like KASAN markings) after instructions are rewritten.

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index d3be972714b22..188ea6205d617 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c

[ ... ]

> @@ -1689,7 +1705,8 @@ int bpf_do_misc_fixups(struct bpf_verifier_env *env)
>  				return -EFAULT;
>  			}
>  
> -			new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
> +			new_prog = bpf_patch_insn_data(env, i + delta, insn_buf,
> +						       cnt, 0);

[Severity: Medium]
For LD_ABS and LD_IND rewrites, the original instruction is completely
replaced by the generated instructions. Should insn_off_in_patch be
set to -1 here instead of 0, to avoid incorrectly identifying the first
setup instruction as the original one?

[ ... ]

> @@ -1742,7 +1759,8 @@ int bpf_do_misc_fixups(struct bpf_verifier_env *env)
>  				*patch++ = BPF_ALU64_IMM(BPF_MUL, off_reg, -1);
>  			cnt = patch - insn_buf;
>  
> -			new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
> +			new_prog = bpf_patch_insn_data(env, i + delta, insn_buf,
> +						       cnt, cnt - 1);

[Severity: Medium]
In this pointer arithmetic mitigation, if a trailing MUL instruction is
appended (when issrc && isneg && !isimm are true), the original instruction
ends up at cnt - 2 instead of cnt - 1. Does passing cnt - 1 here risk
misidentifying the appended MUL instruction as the original instruction?

[ ... ]

> @@ -1787,7 +1805,8 @@ int bpf_do_misc_fixups(struct bpf_verifier_env *env)
>  			insn_buf[6] = BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_AX, stack_off_cnt);
>  			cnt = 7;
>  
> -			new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
> +			new_prog = bpf_patch_insn_data(env, i + delta, insn_buf,
> +						       cnt, 3);

[Severity: Medium]
For timed may_goto, the arbitrary offset 3 points to an intermediate JNE
instruction rather than the original instruction, since the original is
fully replaced. Should this pass -1 instead of 3?

[ ... ]

> @@ -1808,7 +1827,8 @@ int bpf_do_misc_fixups(struct bpf_verifier_env *env)
>  			insn_buf[3] = BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_AX, stack_off);
>  			cnt = 4;
>  
> -			new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
> +			new_prog = bpf_patch_insn_data(env, i + delta, insn_buf,
> +						       cnt, 2);

[Severity: Medium]
Similarly, for non-timed may_goto, the offset 2 points to a SUB instruction.
Since the original may_goto is entirely replaced, would it be more accurate
to pass -1 here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-kasan-v5-0-1c64af8e4e1e@bootlin.com?part=1