Re: [PATCH RFC bpf-next 5/8] bpf, x86: emit KASAN checks into x86 JITed programs

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [High] Missing KASAN instrumentation for immediate-based memory stores (BPF_ST | BPF_MEM) and incorrect write detection.
- [High] Missing KASAN instrumentation for standard atomic memory operations (BPF_STX | BPF_ATOMIC).
- [High] Incorrect IP calculation in emit_kasan_check() leads to wrong jump offsets and kernel crashes when IBT is enabled.
--

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

bpf, x86: emit KASAN checks into x86 JITed programs

This commit inserts KASAN shadow memory checks before load and store
operations in JIT-compiled BPF programs. It aims to detect memory safety bugs
while explicitly skipping stack accesses and faulting PROBE instructions.

> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index b90103bd00809..111fe1d551219 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
[ ... ]
> @@ -2242,6 +2244,11 @@ st:			if (is_imm8(insn->off))
>  		case BPF_STX | BPF_MEM | BPF_H:
>  		case BPF_STX | BPF_MEM | BPF_W:
>  		case BPF_STX | BPF_MEM | BPF_DW:
> +			err = emit_kasan_check(&prog, dst_reg, insn,
> +					       image + addrs[i - 1],
> +					       accesses_stack);
> +			if (err)
> +				return err;
>  			emit_stx(&prog, BPF_SIZE(insn->code), dst_reg, src_reg, insn->off);
>  			break;

Is the correct instruction pointer being passed to emit_kasan_check() here?

Earlier in do_jit(), the local ip variable is updated to account for
conditionally emitted prefixes:

    if (bpf_insn_is_indirect_target(env, bpf_prog, i - 1))
        EMIT_ENDBR();

    ip = image + addrs[i - 1] + (prog - temp);

Passing image + addrs[i - 1] directly ignores the size of the ENDBR prefix.
Could this cause the relative jump offset in emit_call() to be off by 4
bytes, leading to a kernel crash when branching to the KASAN function?

Additionally, looking at the code surrounding this hunk:

        /* ST: *(u8*)(dst_reg + off) = imm */
    case BPF_ST | BPF_MEM | BPF_B:
        if (is_ereg(dst_reg))
            EMIT2(0x41, 0xC6);
        else
            EMIT1(0xC6);
        goto st;
[ ... ]
    case BPF_ST | BPF_MEM | BPF_DW:
        EMIT2(add_1mod(0x48, dst_reg), 0xC7);

st:     if (is_imm8(insn->off))
            EMIT2(add_1reg(0x40, dst_reg), insn->off);
        else
            EMIT1_off32(add_1reg(0x80, dst_reg), insn->off);

        EMIT(imm32, bpf_size_to_x86_bytes(BPF_SIZE(insn->code)));
        break;

Are immediate-based memory stores (BPF_ST | BPF_MEM) meant to be skipped?

Unlike BPF_STX, there is no emit_kasan_check() added here, which seems to
allow uninstrumented writes to kernel memory.

If instrumentation is added here, there might be a second issue. Looking at
the helper:

static int emit_kasan_check(..., struct bpf_insn *insn, ...)
{
    bool is_write = BPF_CLASS(insn->code) == BPF_STX;
    ...
}

Since BPF_ST has a different class, would this evaluate to false and
incorrectly emit a read check instead of a write check?

[ ... ]
> @@ -2390,6 +2397,12 @@ st:			if (is_imm8(insn->off))
>  				/* populate jmp_offset for JAE above to jump to start_of_ldx */
>  				start_of_ldx = prog;
>  				end_of_jmp[-1] = start_of_ldx - end_of_jmp;
> +			} else {
> +				err = emit_kasan_check(&prog, src_reg, insn,
> +						       image + addrs[i - 1],
> +						       accesses_stack);
> +				if (err)
> +					return err;
>  			}
>  			if (BPF_MODE(insn->code) == BPF_PROBE_MEMSX ||
>  			    BPF_MODE(insn->code) == BPF_MEMSX)

Further down in the function, atomic operations are handled:

    case BPF_STX | BPF_ATOMIC | BPF_W:
    case BPF_STX | BPF_ATOMIC | BPF_DW:
        if (insn->imm == (BPF_AND | BPF_FETCH) ||
            insn->imm == (BPF_OR | BPF_FETCH) ||
            insn->imm == (BPF_XOR | BPF_FETCH)) {
            bool is64 = BPF_SIZE(insn->code) == BPF_DW;

Should standard atomic memory operations be instrumented as well?

While the commit message notes that BPF_PROBE_ATOMIC instructions are skipped
because they handle faults, standard BPF_STX | BPF_ATOMIC operations perform
memory modifications without fault-handling semantics.

Since emit_kasan_check() is omitted here, could an invalid base pointer used
in an atomic operation bypass KASAN and corrupt memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260413-kasan-v1-0-1a5831230821@bootlin.com?part=5