On Fri Aug 28, 2026 at 12:30 PM CEST, bot+bpf-ci wrote:
>> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
>> index 13ba3232993ac..adb62216dd204 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;
>
> Does emit_kasan_check need to re-read insn->off here rather than taking
> the effective displacement as a parameter? At the BPF_STX | BPF_MEM site
> (line 2458), do_jit() remaps the offset for stack arguments:
>
> insn_off = insn->off;
> if (dst_reg == BPF_REG_PARAMS) {
> insn_off = outgoing_arg_base - outgoing_rsp - insn_off - 16;
> dst_reg = BPF_REG_FP;
> }
> ...
> emit_kasan_check(env, &prog, dst_reg, insn, ip, true);
> emit_stx(&prog, BPF_SIZE(insn->code), dst_reg, src_reg, insn_off);
>
> The check receives the remapped dst_reg (BPF_REG_FP) but re-derives the
> offset from insn->off, so the check validates 'rbp + insn->off' while
> emit_stx() writes to 'rbp + insn_off'.
This one is right, I managed to reproduce it with a sample program
similar to those for the args on stack selftests, JIT hardening
enabled and cpuv4:
- arg pushed on stack with cpuv4: BPF_ST|BPF_MEM|BPF_DW with dst_reg =
BPF_REG_PARAMS
- hardening + ST on constant: constant pushed on stack is blinded,
program is patched with MOV + XOR + STX, the last one is marked as
non-stack accessing, per commit 1/8
- JIT compiler enters the branch mentioned above and uses wrong offset
for the KASAN check, so we insert an unwanted stack access check AND
we check it at the wrong offset.
This made me realize that the series did not have this specific issues,
as args-on-stack feature has been merged quite recently. Rather than
adjusting the offset for a check that isn't really wanted (this is still
a stack access), I plan to just skip the KASAN check insertion if
dst_reg == BPF_REG_PARAMS (or src_reg == BPF_REG_PARAMS, for LDX).
> The BPF_ST | BPF_MEM case (line 2432) has a second issue: the check
> runs before any register remapping, so it is given the raw dst_reg:
>
> if (!accesses_stack_only) {
> err = emit_kasan_check(env, &prog, dst_reg, insn, ip, true);
> ...
> }
> emit_st(&prog, insn, dst_reg, outgoing_arg_base, outgoing_rsp);
>
> When dst_reg == BPF_REG_PARAMS (MAX_BPF_REG == 11), emit_st() remaps
> both the register and offset internally, but emit_kasan_check() receives
> the raw value. reg2hex[] has no entry for index 11, so reg2hex[11] == 0
> and EMIT_mov(BPF_REG_1, BPF_REG_PARAMS) encodes 'mov rdi, rax'. The
> check then validates 'rax + insn->off' - RAX being BPF R0, an arbitrary
> program value - while the store goes to the outgoing-argument slot.
>
> In the BPF_DW / insn->off == -8 sub-case of BPF_ST, emit_st() performs
> no memory access at all (the value is materialized into R9), yet a full
> __asan_store8 check is still emitted.
Will be fixed the same way: skipping kasan check if dst_reg ==
BPF_REG_PARAMS.
> The BPF_LDX path (line 2645) handles this correctly: it remaps to
> BPF_REG_FP before calling emit_kasan_check(), and the incoming-arg case
> skips the check entirely.
And I'll make sure to skip the KASAN check in the LDX path as well.
>
> Would passing the effective displacement to emit_kasan_check() as an
> explicit parameter fix both the BPF_STX offset mismatch and let the
> BPF_ST path perform the remap before the check?
>
>> + u8 *prog = *pprog;
>> + void *kasan_func;
>> +
>> + if (!env)
>> + return 0;
>
> [ ... ]
>
>> @@ -1911,10 +2026,12 @@ static int do_jit(struct bpf_verifier_env *env,
>> struct bpf_prog *bpf_prog, int *
>> const s32 imm32 = insn->imm;
>> u32 dst_reg = insn->dst_reg;
>> u32 src_reg = insn->src_reg;
>> + bool accesses_stack_only;
>> u8 b2 = 0, b3 = 0;
>> u8 *start_of_ldx;
>> s64 jmp_offset;
>> s32 insn_off;
>> + int insn_idx;
>> u8 jmp_cond;
>> u8 *func;
>> int nops;
>> @@ -1931,6 +2048,10 @@ static int do_jit(struct bpf_verifier_env *env,
>> struct bpf_prog *bpf_prog, int *
>> EMIT_ENDBR();
>>
>> ip = image + addrs[i - 1] + (prog - temp);
>> + insn_idx = i - 1 + bpf_prog->aux->subprog_start;
>> + accesses_stack_only =
>> + env ? !env->insn_aux_data[insn_idx].non_stack_access :
>> + false;
>
> This isn't a bug, but when env == NULL, accesses_stack_only is set to
> false ('not stack-only, do instrument'), yet every call site then
> enters emit_kasan_check() which only no-ops because of its own 'if
> (!env) return 0;' guard. Would 'env ? ... : true' make the fail-safe
> default self-evident and avoid depending on emit_kasan_check()'s
> internal guard for correctness?
I'm not really convinced it would make things better. Having 'env ? ...
: true' would also require to rename the variable to something likely
mouthful, as accesses_something_else_other_than_stack (yes, I'm
exaggerating a bit, but not so much), and as discussed in previous
reviews, the emit_kasan_checks calls will stay anyway, even in the no-op
case.
> Also, the '!env' test plus 'insn_idx += aux->subprog_start' is already
> encapsulated by bpf_insn_is_indirect_target() (kernel/bpf/core.c:1646-
> 1652). Would a small helper like bpf_insn_accesses_non_stack(env, prog,
> i - 1) keep the arch-independent invariant in one place and let other
> JITs reuse it when they gain KASAN support?
There's a clear intent _not_ to bring KASAN support to other JIT
compilers, so there's no much value in a dedicated helper just to check
this flag. If that ever changes, I'll happily reconsider.
[...]
>> @@ -2572,28 +2711,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;
>> + }
>> branch_target = prog;
>> + }
>> + if (is_atomic_fetch) {
>> /* Load old value */
>> emit_ldx(&prog, BPF_SIZE(insn->code),
>> BPF_REG_0, real_dst_reg, insn->off);
>
> This isn't a bug, but does the restructured atomic case now make
> branch_target a dead store on the non-fetch paths? branch_target is
> written for every non-load/store atomic (plain BPF_ADD, BPF_XCHG,
> BPF_CMPXCHG without BPF_FETCH) but only read under 'if
> (is_atomic_fetch)'. Would moving 'branch_target = prog;' into the
> following 'if (is_atomic_fetch)' block keep the KASAN check outside the
> retry loop while restoring the definition-next-to-use property?
Indeed, I'll bring it back under the relevant if branch.
> Also, the new local 'u8 *pprog;' shadows the file-wide convention where
> pprog is the u8 ** out-parameter of every emit_* helper.
True, I'll rename it.
Alexis
--
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
