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

["Alexis Lothoré" <alexis.lothore@bootlin.com>]

On Sat Apr 25, 2026 at 8:08 AM CEST, sashiko-bot wrote:
> 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?

I have CONFIG_X86_KERNEL_IBT=y in my testing kernel, so if that's indeed
a valid scenario, I may have been lucky (or rather, unlucky) to not see the
corresponding crash while running the full selftests set. I'll dig into
that.

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

As mentioned in the previous comments, BPF_ST/BPF_LD are indeed 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?

Atomics were skipped for the RFC, but that's a point I want to tackle
in the next revision.

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com