Re: [PATCH bpf-next v6 9/9] selftests/bpf: add tests to validate KASAN on JIT programs

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] BPF JIT KASAN instrumentation fails to save R10/R11 registers, corrupting BPF register state during atomic operations and memory probes.
- [High] BPF JIT emits KASAN checks for BPF_PROBE_MEM instructions, bypassing exception tables and causing log spam/DoS on expected invalid memory accesses.
--

--- Patch [9]: [PATCH bpf-next v6 9/9] selftests/bpf: add tests to validate KASAN on JIT programs ---
commit cb1fd6ebd1a7c30b4a1e50931fc142f072a09a37
Author: Alexis Lothoré (eBPF Foundation) <[email protected]>

selftests/bpf: add tests to validate KASAN on JIT programs

Add a basic KASAN test runner that loads and test-run programs that can
trigger memory management bugs. The test captures kernel logs and ensure
that the expected KASAN splat is emitted. The runner covers different
cases and settings, including specialized tests for atomic ops.

> 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 and r11 here?

If the compiler allocates r10 in the KASAN helper, or if a KASAN report is
triggered calling kasan_report(), it appears r10 could be clobbered. This
could corrupt the BPF register state for operations that rely on BPF_REG_AX
(which maps to x86 r10).

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

[ ... ]

> @@ -2449,6 +2583,11 @@ static int do_jit(struct bpf_verifier_env *env, struct bpf_prog *bpf_prog, int *
>  				/* 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 if (!accesses_stack_only) {
> +				err = emit_kasan_check(env, &prog, src_reg,
> +						       insn, ip, false);
> +				if (err)
> +					return err;
>  			}

[Severity: High]
Does this code emit KASAN checks for BPF_PROBE_MEM and BPF_PROBE_MEMSX
instructions?

Tracing programs frequently probe pointers that may be NULL or point to
userspace memory using BPF_PROBE_MEM. The verifier emits these
instructions to use the BPF exception table, which safely faults without
crashing or logging.

If emit_kasan_check() is called here unconditionally, it evaluates the
pointer using __asan_load*, which will call kasan_report() on NULL or
userspace pointers. Won't this bypass the exception table entirely and
print a KASAN BUG report to dmesg for every expected invalid pointer,
potentially causing log spam?

>  			if (BPF_MODE(insn->code) == BPF_PROBE_MEMSX ||
>  			    BPF_MODE(insn->code) == BPF_MEMSX)

[ ... ]

> @@ -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]
Could this call to emit_kasan_check() corrupt the BPF register state?

Just above, BPF_REG_0 is saved into BPF_REG_AX, which is mapped to x86 r10.
Since emit_kasan_check() explicitly skips saving r10, any clobbering of
r10 by the KASAN helpers will silently corrupt the saved value of BPF_REG_0.

>  				branch_target = prog;
> +			}

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