Re: [PATCH RFC bpf-next 4/8] bpf, x86: add helper to emit kasan checks in x86 JITed programs

Re: [PATCH RFC bpf-next 4/8] bpf, x86: add helper to emit kasan checks in x86 JITed programs

[Alexis Lothoré]

On Sat Apr 25, 2026 at 7:46 AM CEST, sashiko-bot wrote:
> Sashiko AI review found 3 potential issue(s):
> - [High] Missing call depth accounting for Retbleed mitigations leads to call depth counter underflow.
> - [Medium] BPF_ST instructions are incorrectly classified as memory reads instead of writes.
> - [Low] Flawed stack realignment logic wastes stack space.
> --

[...]

>> @@ -1301,6 +1305,95 @@ static void emit_store_stack_imm64(u8 **pprog, int reg, int stack_off, u64 imm64
>>  	emit_stx(pprog, BPF_DW, BPF_REG_FP, reg, stack_off);
>>  }
>>  
>> +static int emit_kasan_check(u8 **pprog, u32 addr_reg, struct bpf_insn *insn,
>> +			    u8 *ip, bool accesses_stack)
>> +{
>> +#ifdef CONFIG_BPF_JIT_KASAN
>> +	bool is_write = BPF_CLASS(insn->code) == BPF_STX;
>
> Does this code incorrectly classify BPF_ST instructions as memory reads?
>
> The BPF instruction set includes the BPF_ST class for storing immediate
> values into memory. Because BPF_CLASS(insn->code) for BPF_ST evaluates to
> 0x02, the is_write condition evaluates to false. This causes the helper to
> inject KASAN load checks instead of store checks, leading to inaccurate
> error reporting.

This helper is called in do_jit only for BPF_STX and BPF_LDX insns in the next
commit, so it will never wrongly classify wrongly a BPF_ST insn. But I
can add a comment, or even enforce a stricter check to make it
future-proof.

>> +	u32 bpf_size = BPF_SIZE(insn->code);
>> +	s32 off = insn->off;
>> +	u8 *prog = *pprog;
>> +	void *kasan_func;
>> +
>> +	if (accesses_stack)
>> +		return 0;
>> +
>> +	/* Derive KASAN check function from access type and size */
>> +	switch (bpf_size) {
>
> [ ... ]
>
>> +	/* Save r10 */
>> +	EMIT2(0x41, 0x52);
>> +	/* Save r11 */
>> +	EMIT2(0x41, 0x53);
>> +	/* We have pushed 72 bytes, realign stack to 16 bytes: sub rsp, 8 */
>> +	EMIT4(0x48, 0x83, 0xEC, 8);
>
> Is this stack realignment logic correct?
>
> The x86 BPF JIT prologue pushes a variable number of callee-saved registers
> and subtracts an 8-byte aligned stack_depth, meaning the stack pointer can
> be 8-byte misaligned prior to the check. If the stack is initially 8-byte
> misaligned, pushing 72 bytes and subtracting 8 preserves the 8-byte
> misalignment. Because the Linux kernel is compiled with 8-byte stack
> alignment, the sub rsp, 8 instruction seems unnecessary and wastes 8 bytes
> of stack space.

Hmmm, I have been assuming that stack was aligned when reaching the
emit_kasan_check, but indeed, as we could be anywhere in the prog, rsp
can already be misaligned. However, I understand for SytemV ABI, sec
3.2.2 that the stack must systematically be aligned on 16 bytes before
any function call. So I'll have to find a way to make this part a bit
smarter and add the alignment only if needed.

>> +
>> +	/* mov rdi, addr_reg */
>> +	EMIT_mov(BPF_REG_1, addr_reg);
>> +
>> +	/* add rdi, off (if offset is non-zero) */
>> +	if (off) {
>
> [ ... ]
>
>> +	/* Adjust ip to account for the instrumentation generated so far */
>> +	ip += (prog - *pprog);
>> +	/* call kasan_func */
>> +	if (emit_call(&prog, kasan_func, ip))
>> +		return -ERANGE;
>
> Does this call need call depth accounting for Retbleed mitigations?
>
> When CONFIG_CALL_DEPTH_TRACKING is enabled, every call instruction executed
> in the kernel must be properly accounted for to increment the call depth
> counter. Other dynamically emitted calls in the BPF JIT explicitly emit
> this tracking via x86_call_depth_emit_accounting() prior to emitting the
> actual call.
>
> By omitting this accounting, the call to the KASAN function will not
> increment the call depth counter. When the KASAN function returns, its
> return thunk will decrement the counter, causing it to underflow. This
> causes the return thunks to assume the Return Stack Buffer is empty and
> trigger expensive RSB stuffing on subsequent returns.

I completely missed out this (and that's likely not the only one). I'll
take a look into it.

Alexis

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