Re: [PATCH bpf-next v3 02/10] bpf: mark instructions accessing program stack

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `is_mem_insn` helper incorrectly excludes `BPF_ATOMIC`, causing inline atomic operations to bypass KASAN checks.
- [Medium] Early return in `adjust_insn_aux_data` bypasses KASAN marking for 1-to-1 patched memory instructions.
--

commit 90d3117a6d0883383f557efe547dcd0c0e5bc53a
Author: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>

bpf: mark instructions accessing program stack

In order to prepare to emit KASAN checks in JITed programs, JIT compilers
need to be aware about whether some load/store instructions are targeting
the bpf program stack, as those should not be monitored. The verifier marks
instructions depending on whether they access memory other than the stack,
taking care to shift instruction marking when the verifier patches
instructions.

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index 1f340211b65c2..0b58a02a71795 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c
> @@ -152,6 +152,17 @@ static int get_callee_stack_depth(struct bpf_verifier_env *env,
>  }
>  #endif
>  
> +static bool is_mem_insn(struct bpf_insn *insn)
> +{
> +	if (BPF_MODE(insn->code) != BPF_MEM &&
> +	    BPF_MODE(insn->code) != BPF_MEMSX)
> +		return false;

[Severity: High]
Does this logic inadvertently exclude BPF_ATOMIC operations from KASAN bounds
checking?

When bpf_kptr_xchg is inlined into an atomic exchange (via fixup_bpf_calls()),
the verifier replaces the CALL with a BPF_ATOMIC instruction. Because this
function specifically requires BPF_MEM or BPF_MEMSX, it returns false for
BPF_ATOMIC.

Consequently, the newly generated atomic operation inherits
non_stack_access = false from the original CALL instruction. The JIT will then
assume the map value access is a stack access and skip KASAN instrumentation.
Could this be updated to include BPF_ATOMIC?

> +
> +	return BPF_CLASS(insn->code) == BPF_ST ||
> +		BPF_CLASS(insn->code) == BPF_STX ||
> +		BPF_CLASS(insn->code) == BPF_LDX;
> +}

[ ... ]

> @@ -183,7 +194,19 @@ 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 = insn_has_def32(insn + i);
> +		if (i == off + insn_off_in_patch) {
> +			data[i].non_stack_access = data[off + cnt - 1].non_stack_access;
> +			data[off + cnt - 1].non_stack_access = false;
> +		} 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
> +	 */
> +	if (is_mem_insn(insn + off + cnt - 1) && insn_off_in_patch != cnt - 1)
> +		data[off + cnt - 1].non_stack_access = true;

[Severity: Medium]
What happens when a non-memory instruction is replaced with a single memory
instruction (i.e., cnt == 1)?

Looking at adjust_insn_aux_data(), there is an early return if cnt == 1:

	data[off].zext_dst = insn_has_def32(insn + off + cnt - 1);

	if (cnt == 1)
		return;

Since this new logic is placed after the early return, if a non-memory
instruction (like a helper call) is replaced 1-to-1 with a memory instruction
(e.g., BPF_LDX_MEM for bpf_get_func_ip), it skips evaluating is_mem_insn().

The newly generated instruction would then blindly inherit
non_stack_access = false, causing the JIT to skip KASAN bounds checks.

Is it possible to move the 1-to-1 handling before the early return, or
explicitly check it there?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701-kasan-v3-0-bd09bb942d86@bootlin.com?part=2