Re: [PATCH bpf-next v7 1/9] bpf: mark instructions accessing program stack

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] The `adjust_insn_aux_data()` logic leaks the `non_stack_access` flag onto non-memory instructions when a patch entirely drops the original instruction.
- [Low] Duplicated original instructions in a patch sequence lose their `non_stack_access` marking.
--

--- Patch [1]: [PATCH bpf-next v7 1/9] bpf: mark instructions accessing program stack ---
commit e45a03bdb90ce436777e9a9558206c04d0705394
Author: Alexis Lothoré (eBPF Foundation) <[email protected]>

bpf: mark instructions accessing program stack

This commit adds tracking in the BPF verifier for load/store instructions
that access memory other than the BPF program stack. It modifies instruction
patching logic to retain or conservatively mark these instructions to aid
JIT compilers in emitting KASAN checks.

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index 65b441e4a3517..edcb0cbbb13de 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c
[ ... ]
> @@ -239,8 +259,25 @@ static void adjust_insn_aux_data(struct bpf_verifier_env *env,
>  		/* Expand insni[off]'s seen count to the patched range. */
>  		data[i].seen = old_seen;
>  		data[i].zext_dst = bpf_insn_def32(new_prog, insn + i) >= 0;
> +		if (!memcmp(insn + i, original_insn, sizeof(struct bpf_insn))) {
> +			data[i].non_stack_access =
> +				data[off + cnt - 1].non_stack_access;
> +			data[off + cnt - 1].non_stack_access = false;

[Severity: Low]
Does this logic correctly handle patch sequences that might duplicate the
original instruction?

If a patch duplicates the original instruction, the first match correctly
inherits the non_stack_access marking, but then actively clears the source
value by setting it to false here.

Any subsequent duplicated original instruction would then inherit a cleared
false marking, leading to missing KASAN runtime instrumentation checks for
that memory access.

> +		} else if (is_mem_insn(insn + i)) {
> +			data[i].non_stack_access = true;
> +		}
>  	}
>  
> +	/*
> +	 * Last slot instruction could be a newly generated
> +	 * BPF_ST/BPF_LDX/BPF_STX, systematically mark it for non-stack access
> +	 * if it is not the original instruction, otherwise keep the
> +	 * original marking
> +	 */
> +	if (is_mem_insn(insn + off + cnt - 1) &&
> +	    memcmp(insn + off + cnt - 1, original_insn, sizeof(struct bpf_insn)))
> +		data[off + cnt - 1].non_stack_access = true;

[Severity: Low]
Can the non_stack_access flag leak onto non-memory instructions if a patch
entirely drops the original instruction?

A preceding memmove copies the original instruction's marking to the final
patched instruction's slot:

	memmove(data + off + cnt - 1, data + off,
		sizeof(struct bpf_insn_aux_data) * (prog_len - off - cnt + 1));

If the original memory instruction is replaced by a sequence that does not
contain the exact original instruction and ends in a non-memory instruction,
this final check fails to reset the inherited true flag back to false since it
only evaluates is_mem_insn().

While the leaked flag is currently benign because JIT compilers exclusively
evaluate the flag for memory instructions, it seems like an unintended state
for the verifier auxiliary data.

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