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

[sashiko-bot@kernel.org]

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?

>  			break;

[ ... ]

> @@ -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.

[ ... ]

> @@ -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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-kasan-v4-0-d5c177ab8227@bootlin.com?part=1