Re: [PATCH bpf-next v9 4/8] bpf, x86: emit KASAN checks in x86 JITed programs

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 2642 bytes --]

> The main instructions being targeted are BPF_ST, BPF_STX and BPF_LDX,
> but not all of them are being instrumented:
> - if the load/store instruction is in fact accessing the program stack,
>   emit_kasan_check silently skips the instrumentation, as we can already
>   benefit from guard pages to monitor stack accesses.
> - if the load/store instruction is a BPF_PROBE_MEM or a BPF_PROBE_ATOMIC
>   instruction, we do not instrument it, as the passed address can fault
>   (hence the custom fault management with BPF_PROBE_XXX instructions),
>   and so the corresponding kasan check could fault as well.

This isn't a bug, but could the changelog be more precise about where
the stack-access skip happens?

The text says emit_kasan_check() "silently skips the instrumentation"
for stack accesses, but the decision is actually made in do_jit() by the
new accesses_stack_only test:

arch/x86/net/bpf_jit_comp.c:do_jit() {
    ...
    insn_idx = i - 1 + bpf_prog->aux->subprog_start;
    accesses_stack_only =
        env ? !env->insn_aux_data[insn_idx].non_stack_access :
              false;
    ...
}

which reads the verifier's per-instruction non_stack_access flag set by
mark_non_stack_access() at kernel/bpf/verifier.c:6665.

emit_kasan_check() only skips when the address register is literally
BPF_REG_FP or BPF_REG_PARAMS. The list also omits BPF_PROBE_MEM32
(arena) accesses, which are handled by separate case labels and likewise
left uninstrumented.

[ ... ]

> @@ -1914,6 +2032,10 @@
>  		EMIT_ENDBR();
>
>  	ip = image + addrs[i - 1] + (prog - temp);
> +	insn_idx = i - 1 + bpf_prog->aux->subprog_start;
> +	accesses_stack_only =
> +		env ? !env->insn_aux_data[insn_idx].non_stack_access :
> +		      false;

This isn't a bug, but would a positively named predicate read more
directly here?

When env is NULL the fallback value false means "this access is not
stack-only", so every load/store takes the !accesses_stack_only path and
calls emit_kasan_check(), which immediately returns 0 because of its own
if (!env) return 0; guard at arch/x86/net/bpf_jit_comp.c:1129.

The intent (no instrumentation without a verifier env) is expressed
through a double negative plus a second guard in the callee. Would
accesses_stack_only = !env || !env->insn_aux_data[insn_idx].non_stack_access
(or a want_kasan_check predicate) make the no-env case self-evident and
drop the redundant callee check?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/33766012402