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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] `emit_kasan_check` fails to save the caller-saved register %r10 (BPF_REG_AX), which corrupts BPF_REG_0 and memory pointers during atomic fetch operations.
--

--- Patch [4]: [PATCH bpf-next v7 4/9] bpf, x86: emit KASAN checks in x86 JITed programs ---
commit 6bca50db6e9af9416363fe2e60f4e42f924950c9
Author: Alexis Lothoré (eBPF Foundation) <[email protected]>

bpf, x86: emit KASAN checks in x86 JITed programs

Insert KASAN shadow memory checks before memory load and store
operations in JIT-compiled BPF programs. This helps detect memory safety
bugs such as use-after-free and out-of-bounds accesses at runtime.

> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 13ba3232993ac..8134d028a2f2c 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c

[ ... ]

> @@ -1110,6 +1121,92 @@ static void maybe_emit_1mod(u8 **pprog, u32 reg, bool is64)
>  	*pprog = prog;
>  }
>  
> +static int emit_kasan_check(struct bpf_verifier_env *env, u8 **pprog,
> +			    u32 addr_reg, struct bpf_insn *insn, u8 *ip,
> +			    bool is_write)
> +{
> +#ifdef CONFIG_BPF_JIT_KASAN
> +	u32 bpf_size = BPF_SIZE(insn->code);
> +	s32 off = insn->off;
> +	u8 *prog = *pprog;
> +	void *kasan_func;
> +
> +	if (!env)
> +		return 0;
> +
> +	/* Derive KASAN check function from access type and size */
> +	switch (bpf_size) {
> +	case BPF_B:
> +		kasan_func = is_write ? __asan_store1 : __asan_load1;
> +		break;
> +	case BPF_H:
> +		kasan_func = is_write ? __asan_store2 : __asan_load2;
> +		break;
> +	case BPF_W:
> +		kasan_func = is_write ? __asan_store4 : __asan_load4;
> +		break;
> +	case BPF_DW:
> +		kasan_func = is_write ? __asan_store8 : __asan_load8;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	/* Save rax */
> +	EMIT1(0x50);
> +	/* Save rcx */
> +	EMIT1(0x51);
> +	/* Save rdx */
> +	EMIT1(0x52);
> +	/* Save rsi */
> +	EMIT1(0x56);
> +	/* Save rdi */
> +	EMIT1(0x57);
> +	/* Save r8 */
> +	EMIT2(0x41, 0x50);
> +	/* Save r9 */
> +	EMIT2(0x41, 0x51);
> +	/*
> +	 * SystemV ABI states that we should also save r10/r11, but in
> +	 * practice those registers are _not_ used by the limited set of
> +	 * kasan helpers we are calling here, so that's fine not to save those.
> +	 */
> +
> +	/* mov rdi, addr_reg */

[Severity: High]
Is it safe to skip saving %r10 and %r11 here?

In the x86 BPF JIT, %r10 is mapped to BPF_REG_AX. During atomic fetch
operations, BPF_REG_0 is stashed into BPF_REG_AX before calling
emit_kasan_check():

arch/x86/net/bpf_jit_comp.c:do_jit() {
    ...
    /* Will need RAX as a CMPXCHG operand so save R0 */
    emit_mov_reg(&prog, true, BPF_REG_AX, BPF_REG_0);
    if (src_reg == BPF_REG_0)
        real_src_reg = BPF_REG_AX;
    if (dst_reg == BPF_REG_0)
        real_dst_reg = BPF_REG_AX;

    if (!bpf_atomic_is_load_store(insn)) {
        if (!accesses_stack_only) {
            err = emit_kasan_check(env, &prog, real_dst_reg, insn, ip, true);
    ...
}

Since the KASAN helpers are standard C functions, they are permitted by the
System V AMD64 ABI to clobber caller-saved registers like %r10 (especially if
kasan_report is called).

If %r10 is clobbered, could this corrupt the stashed BPF_REG_0 and potentially
cause a crash when real_dst_reg is used as a memory pointer in the subsequent
emit_ldx() call?

arch/x86/net/bpf_jit_comp.c:do_jit() {
    ...
    /* Load old value */
    emit_ldx(&prog, BPF_SIZE(insn->code), BPF_REG_0, real_dst_reg, insn->off);
    ...
    /* Restore R0 after clobbering RAX */
    emit_mov_reg(&prog, true, BPF_REG_0, BPF_REG_AX);
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4