Re: [RFC PATCH bpf-next 11/12] bpf, x86: Add JIT support SDT for probe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] NULL pointer dereference in the x86 JIT compiler for classic BPF (cBPF) programs
- [High] Stack buffer overflow in `bpf_sdt_link_attach()` due to unvalidated `nargs` field
- [High] Out-of-bounds read in the Verifier and JIT compiler via unvalidated SDT argument registers
- [High] The SDT probe trampoline clobbers x86 caller-saved registers, corrupting the execution state of the target BPF program
- [High] Verifier bypass of SDT probe constraints via duplicate `orig_off` map entries
--

commit e3ca8ab99711f4294e8b0182251c50473a63e6d7
Author: Xu Kuohai <xukuohai@huawei.com>
Subject: bpf, x86: Add JIT support SDT for probe

This commit adds x86 JIT support for SDT probes. It jits SDT probe sites to
a 5-byte NOP and patches them to a CALL instruction when attached to an
observer program. It also modifies the BPF trampoline to save and restore
arguments from the func_model's arg_regs instead of fixed BPF registers.

> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 054e043ffcd22..262f1bd74d53f 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -2797,6 +2797,16 @@ st:			insn_off = insn->off;
>  			}
>  
>  			if (!jmp_offset) {
> +				int index = bpf_prog->aux->subprog_start + i - 1;
> +
> +				/*
> +				 * Emit a 5-byte NOP for SDT probe site, so it can be patched to
> +				 * a CALL instruction when observer program is attached.
> +				 */
> +				if (env->insn_aux_data[index].sdt_entry) {

[Severity: Critical]
Does this code dereference a NULL pointer when JIT compiling classic BPF
programs?

When an unprivileged user attaches a classic BPF filter containing a jump to
the next instruction, the kernel translates it to eBPF and invokes
bpf_int_jit_compile() with a NULL verifier environment.

During compilation, do_jit() processes the jump with a zero offset and
evaluates env->insn_aux_data[index].sdt_entry. Since env is NULL, could this
lead to an immediate kernel panic?

> +					emit_nops(&prog, X86_PATCH_SIZE);
> +					break;
> +				}
>  				/*
>  				 * If jmp_padding is enabled, the extra nops will
>  				 * be inserted. Otherwise, optimize out nop jumps.

[ ... ]

> @@ -3087,9 +3101,12 @@ static void restore_regs(const struct btf_func_model *m, u8 **prog,
>  		arg_regs = (m->arg_size[i] + 7) / 8;
>  		if (nr_regs + arg_regs <= 6) {
>  			for (j = 0; j < arg_regs; j++) {
> -				emit_ldx(prog, BPF_DW,
> -					 nr_regs == 5 ? X86_REG_R9 : BPF_REG_1 + nr_regs,
> -					 BPF_REG_FP,
> +				u8 reg = (flags & BPF_TRAMP_F_SDT_PROBE)
> +					? m->arg_regs[nr_regs]
> +					: (nr_regs == 5 ? X86_REG_R9
> +					   : BPF_REG_1 + nr_regs);
> +
> +				emit_ldx(prog, BPF_DW, reg, BPF_REG_FP,

[Severity: High]
Can this lead to an out-of-bounds read if the register index is unvalidated?

If a user provides an out-of-bounds value for arg_reg in an insn_array map
entry, and the verifier is bypassed, could m->arg_regs[nr_regs] hold a value
exceeding the expected register range?

When this unvalidated index is passed to emit_ldx(), it appears to cause an
out-of-bounds read on the reg2hex array during x86 opcode generation,
potentially corrupting memory.

Is there a similar issue in check_sdt_probe() during verification?

kernel/bpf/verifier.c:check_sdt_probe() {
    ...
    u8 reg = val->arg_reg[i];
    struct bpf_reg_state *rs = &regs[reg];
    ...
}

If reg is unvalidated, could this also cause an out-of-bounds read on the
verifier's regs array?

>  					 -stack_size);
>  				stack_size -= 8;
>  				nr_regs++;

[ ... ]

> @@ -3603,7 +3620,7 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *rw_im
>  	}
>  
>  	if (flags & BPF_TRAMP_F_RESTORE_REGS)
> -		restore_regs(m, &prog, regs_off);
> +		restore_regs(m, &prog, regs_off, flags);

[Severity: High]
Does the SDT probe trampoline clobber x86 caller-saved registers here?

The verifier treats the 5-byte NOP as a jump with a zero offset, assuming all
BPF caller-saved registers remain intact. However, when an observer attaches,
the NOP is patched to call the BPF trampoline, which executes the observer
program.

Could the observer program clobber the x86 caller-saved registers (RAX, RCX,
RDX, RDI, RSI, R8)? Since restore_regs() only restores the explicitly tracked
SDT argument registers, the remaining caller-saved registers might remain
corrupted when control returns to the target program, potentially corrupting
its execution state.

>  
>  	/* This needs to be done regardless. If there were fmod_ret programs,
>  	 * the return value is only updated on the stack and still needs to be

[ ... ]

[Severity: High]
Is it possible to bypass the verifier's SDT probe constraints using duplicate
offsets in the insn_array map?

In bpf_insn_array_init(), multiple map entries with the same offset appear to
overwrite each other sequentially:

kernel/bpf/bpf_insn_array.c:bpf_insn_array_init() {
    ...
    env->insn_aux_data[values[i].orig_off].sdt_entry = &values[i];
    ...
}

This means the verifier might only validate the last matching element.
However, bpf_insn_array_get_sdt_probe_by_name() seems to return the first
matching element by scanning sequentially:

kernel/bpf/bpf_insn_array.c:bpf_insn_array_get_sdt_probe_by_name() {
    ...
    if (!strcmp(insn_array->values[j].name, name)) {
        *val = insn_array->values[j];
    ...
}

Could this allow a malicious user to place a malformed, unverified probe
entry before a valid one with the same offset, completely subverting the
verifier?

[Severity: High]
If the verifier is bypassed, could bpf_sdt_link_attach() suffer from a stack
buffer overflow?

When looking up a probe by name, bpf_sdt_link_attach() might retrieve an
unverified entry with an arbitrary nargs value (up to 255):

kernel/bpf/syscall.c:bpf_sdt_link_attach() {
    ...
    for (i = 0; i < sdt->val.nargs; i++) {
        tgt_info.fmodel.arg_size[i] = 8;
        tgt_info.fmodel.arg_regs[i] = sdt->val.arg_reg[i];
    }
    ...
}

Because tgt_info.fmodel.arg_size and arg_regs are statically sized arrays
on the kernel stack, could this loop write out-of-bounds if nargs exceeds
the arrays' bounds?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1782571533.git.xukuohai@huawei.com?part=11