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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] The `emit_kasan_check` function clobbers caller-saved registers %r10 and %r11, leading to memory corruption and verifier bypass.
- [Low] Variable declarations immediately follow a switch case label without an enclosing block, causing a compilation error.
--

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

bpf, x86: emit KASAN checks into x86 JITed programs

Insert KASAN shadow memory checks before memory load and store
operations in JIT-compiled BPF programs.

> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 943a0f315cf2b..cb3c03edc4bde 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c

[ ... ]

> @@ -2592,13 +2622,13 @@ static int do_jit(struct bpf_verifier_env *env, struct bpf_prog *bpf_prog, int *
>  			fallthrough;
>  		case BPF_STX | BPF_ATOMIC | BPF_W:
>  		case BPF_STX | BPF_ATOMIC | BPF_DW:
> +			bool is64 = BPF_SIZE(insn->code) == BPF_DW;
> +			u32 real_src_reg = src_reg;
> +			u32 real_dst_reg = dst_reg;
> +			u8 *branch_target;

[Severity: Low]
Will this cause a build failure? The C standard does not allow variable
declarations immediately following a case label without an enclosing block.

>  			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;
> -				u32 real_src_reg = src_reg;
> -				u32 real_dst_reg = dst_reg;
> -				u8 *branch_target;
>  
>  				/*
>  				 * Can't be implemented with a single x86 insn.
> @@ -2612,7 +2642,19 @@ static int do_jit(struct bpf_verifier_env *env, struct bpf_prog *bpf_prog, int *
>  				if (dst_reg == BPF_REG_0)
>  					real_dst_reg = BPF_REG_AX;
>  
> +				ip += 3;
> +			}
> +			if (!bpf_atomic_is_load_store(insn)) {
> +				err = emit_kasan_check(&prog, real_dst_reg,
> +						       insn, ip, false,
> +						       accesses_stack_only);

[Severity: Critical]
Does emit_kasan_check() preserve all caller-saved registers it needs to?

Looking at emit_kasan_check(), it emits pushes for rax, rcx, rdx, rsi, rdi,
r8, and r9 before calling C functions like __asan_loadX, but it does not
appear to save r10 (BPF_REG_AX) or r11 (AUX_REG).

For complex atomic operations, do_jit() saves BPF_REG_0 into BPF_REG_AX
just before this call:

    /* Will need RAX as a CMPXCHG operand so save R0 */
    emit_mov_reg(&prog, true, BPF_REG_AX, BPF_REG_0);

If emit_kasan_check() calls a C function which clobbers r10, will this
corrupt the value restored into BPF_REG_0 and potentially break verifier
assumptions?

> +				if (err)
> +					return err;
>  				branch_target = prog;
> +			}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604-kasan-v2-0-c066e627fda8@bootlin.com?part=5