Re: [PATCH bpf-next v6 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 caller-saved register R10 (BPF_REG_AX), causing BPF state corruption during atomic fetch operations and constant blinding.
--

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

This commit adds KASAN shadow memory checks before load and store operations
in JIT-compiled BPF programs on x86. It aims to detect memory safety bugs at
runtime by emitting calls to standard KASAN C helper functions.

> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 0b8b5dfe37ab0..b7881b9953107 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)
> +	/* 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.
> +	 */

[Severity: High]
Is it safe to skip saving R10 (BPF_REG_AX) here?

Since R10 is a caller-saved register under the System V ABI, the compiler
is free to use it in __asan_loadX or __asan_storeX. Furthermore, if a
KASAN warning triggers, the kasan_report() slow path will clobber R10
before returning.

> +
> +	/* mov rdi, addr_reg */
> +	EMIT_mov(BPF_REG_1, addr_reg);

[ ... ]

> @@ -2288,6 +2416,12 @@ static int do_jit(struct bpf_verifier_env *env, struct bpf_prog *bpf_prog, int *
>  				insn_off = outgoing_arg_base - outgoing_rsp - insn_off - 16;
>  				dst_reg = BPF_REG_FP;
>  			}
> +			if (!accesses_stack_only) {
> +				err = emit_kasan_check(env, &prog, dst_reg,
> +						       insn, ip, true);
> +				if (err)
> +					return err;
> +			}

[Severity: High]
Could emit_kasan_check clobber the source register here if constant
blinding is used?

When bpf_jit_blind_constants rewrites a BPF_ST instruction, it places the
unblinded constant into BPF_REG_AX (R10) and issues a BPF_STX with
BPF_REG_AX as the source register. If R10 is clobbered by the KASAN check,
does this cause a store of garbage memory?

>  			emit_stx(&prog, BPF_SIZE(insn->code), dst_reg, src_reg, insn_off);
>  			break;

[ ... ]

> @@ -2510,28 +2649,42 @@ 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:
> -			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;
> -
> +		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;
> +			u8 *pprog;
> +			bool is_atomic_fetch =
> +				(insn->imm == (BPF_AND | BPF_FETCH) ||
> +				 insn->imm == (BPF_OR | BPF_FETCH) ||
> +				 insn->imm == (BPF_XOR | BPF_FETCH));
> +			if (is_atomic_fetch) {
>  				/*
>  				 * Can't be implemented with a single x86 insn.
>  				 * Need to do a CMPXCHG loop.
>  				 */
>  
>  				/* Will need RAX as a CMPXCHG operand so save R0 */
> +				pprog = prog;
>  				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;
> -
> +				ip += (prog - pprog);
> +			}
> +			if (!bpf_atomic_is_load_store(insn)) {
> +				if (!accesses_stack_only) {
> +					err = emit_kasan_check(env, &prog,
> +							       real_dst_reg,
> +							       insn, ip, true);
> +					if (err)
> +						return err;
> +				}

[Severity: High]
Will this result in BPF state corruption during atomic fetch operations?

Here, BPF_REG_0 (RAX) is temporarily saved to BPF_REG_AX (R10) just prior
to calling emit_kasan_check(). If emit_kasan_check() clobbers R10, won't
BPF_REG_0 be restored with corrupted data after the operation?

>  				branch_target = prog;
> +			}

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