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

["Alexis Lothoré" <alexis.lothore@bootlin.com>]

On Wed Jul 8, 2026 at 10:19 AM CEST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] Incorrect or inconsistent calculation of `insn_off_in_patch` causes loss or misassignment of `aux_data`.
> - [Medium] A single `insn_off_in_patch` integer is insufficient to propagate KASAN memory markings to patches that emit multiple memory accesses.
> --
>
> --- Patch [1]: [PATCH bpf-next v4 01/10] bpf: propagate original instruction offset when patching program ---
> commit 6ebaaf2368d2745f393ec38eb53d5857d5ec62b8
> Author: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
>
> bpf: propagate original instruction offset when patching program
>
> When the verifier patches an ebpf program with bpf_patch_insn_data, it
> then calls adjust_insn_aux_data to make sure that insn_aux_data takes
> into account the newly inserted patch.
>
>> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
>> index 12a8a4eb757f1..f515fc9669ce2 100644
>> --- a/kernel/bpf/fixups.c
>> +++ b/kernel/bpf/fixups.c
>
> [ ... ]
>
>> @@ -856,16 +862,20 @@ int bpf_convert_ctx_accesses(struct bpf_verifier_env *env)
>>  			if (!ops->convert_ctx_access)
>>  				continue;
>>  			convert_ctx_access = ops->convert_ctx_access;
>> +			insn_off_in_patch = 0;
>
> [Severity: Medium]
> Will this properly track context accesses that are converted into multiple
> chained memory loads?
>
> By relying on a single integer to track the offset, it seems we might fail to
> account for multi-load patches emitted by ops->convert_ctx_access() (for
> example, reading skb->dev followed by dev->ifindex). The secondary memory
> accesses will receive zeroed aux_data and lack the required KASAN
> non_stack_access marking.
>
> Additionally, since the original instruction is completely replaced here, does
> it make sense to unconditionally assign its aux_data to the first generated
> instruction, which might not even be the actual memory access?

Hmmm, true, there's an issue here. I have checked many XXX_convert_ctx
accesses and naively assumed that the generated patch was always a
single ldx or stx, but indeed, there are corner cases:
- bpf_convert_ctx: __skb->ifindex becomes LDX + JPM + LDX
- bpf_convert_ctx: __skb->pkt_type becomes LDX + AND
- bpf_convert_ctx: __skb->queue_mapping becomes (JMP +) ST or STX
- and many more

For all of those XXX_convert_ctx_accesses, the accessed memory should
always be checked with KASAN since we are dealing with ctx pointers, so
it may be simpler just to pass -1 as insn_off_patches to
bpf_patch_insn_data for all of those, so that any LDX/STX/ST in the new
patch is instrumented.

>> @@ -1464,7 +1475,7 @@ static int add_hidden_subprog(struct bpf_verifier_env *env, struct bpf_insn *pat
>>  	 * ones for the hidden subprog. Hence all of the adjustment operations
>>  	 * in bpf_patch_insn_data are no-ops.
>>  	 */
>> -	prog = bpf_patch_insn_data(env, env->prog->len - 1, patch, len);
>> +	prog = bpf_patch_insn_data(env, env->prog->len - 1, patch, len, -1);
>
> [Severity: Medium]
> Is it intentional to pass -1 for the offset here?
>
> Since the original instruction is explicitly preserved in the patch buffer
> in bpf_do_misc_fixups():
>
> 	*patch++ = env->prog->insnsi[insn_cnt - 1];
>
> Passing -1 to bpf_patch_insn_data() will cause the aux_data associated with
> that instruction to be lost.

True, but: unless I am missing some corner cases, eBPF programs end with BPF_EXIT,
so it is pointless to forward an original instruction offset. But for
the sake of separating concerns, I'll update this to pass 0 instead of
-1.

>> @@ -1743,7 +1765,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, -1);
>
> [Severity: Medium]
> In this pointer arithmetic rewrite, the original ALU instruction is preserved
> in the patch buffer (*patch++ = *insn).
>
> Should we calculate and pass its exact offset inside the patch buffer instead
> of passing -1, which discards its aux_data?

Ditto: having the exact instruction offset for this case is not useful
as it is not a load or store, but I'll still update the offset value (-1
-> cnt - 1)

Alexis

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com