Re: [RFC PATCH 2/4] tracing/probes: Compile all fetchargs into a single BPF program per event

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Early termination of the BPF fetch program upon a single memory fault skips the processing of all subsequent probe arguments, exposing uninitialized ring-buffer memory to userspace.
- [Critical] The generated BPF program bypasses the verifier and leaves `stack_depth` as 0, causing JIT to omit stack allocation and leading to stack corruption during memory fetches.
- [Critical] Direct invocation of `copy_from_kernel_nofault` using `BPF_EMIT_CALL` violates kCFI and architecture calling conventions.
- [High] Failures from `bpf_prog_select_runtime` are silently ignored because the code checks `IS_ERR(prog)` instead of the `err` out-parameter.
- [High] Missing `refcnt` initialization of the manually allocated BPF program leads to a permanent memory leak.
--

commit fb399d3b83423ab453a35bd3c084a85256eaf7df
Author: Masami Hiramatsu (Google) <mhiramat@kernel.org>

tracing/probes: Compile all fetchargs into a single BPF program per event

This commit compiles trace probe arguments into a single BPF program to
reduce prologue and dispatching overhead. It supports basic BPF-compatible
arguments and falls back to the interpreter loop if non-BPF operations
are required.

> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> index 18c2121223440..0deb53c22ae39 100644
> --- a/kernel/trace/trace_probe.c
> +++ b/kernel/trace/trace_probe.c
[ ... ]
> +static void trace_probe_compile_bpf(struct trace_probe *tp)
> +{
[ ... ]
> +			case FETCH_OP_DEREF:
> +				/* Add offset: R3 = R0 + code->offset (src) */
> +				insns[i++] = BPF_MOV64_REG(BPF_REG_2, BPF_REG_0);
> +				if (code->offset)
> +					insns[i++] = BPF_ALU64_IMM(BPF_ADD, BPF_REG_2,
> +								   code->offset);
> +				/* R1 = dst (R10 - 8 on stack) */
> +				insns[i++] = BPF_MOV64_REG(BPF_REG_1, BPF_REG_10);
> +				insns[i++] = BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8);

[Severity: Critical]
Does this stack access corrupt the caller's stack frame?

Since this manually constructed BPF program bypasses the verifier,
prog->aux->stack_depth remains 0. Because the stack depth is 0, the JIT
compiler will not allocate a stack frame for the program. When the program
writes to R10 - 8, it might overwrite the caller's stack frame or
callee-saved registers, potentially leading to kernel panics.

> +				/* R3 = size */
> +				insns[i++] = BPF_MOV64_IMM(BPF_REG_3, sizeof(unsigned long));
> +				/* Call copy_from_kernel_nofault(dst, src, size) */
> +				insns[i++] = BPF_EMIT_CALL(copy_from_kernel_nofault);

[Severity: Critical]
Can this direct call to copy_from_kernel_nofault trigger a kCFI panic?

copy_from_kernel_nofault is a standard C function with a 3-argument
signature, but BPF_EMIT_CALL executes a BPF helper call which expects
to pass 5 arguments in 64-bit BPF registers. Under Control Flow Integrity,
invoking this through the interpreter's 5-argument function pointer might
cause a signature mismatch and panic the kernel. It could also break the
standard calling convention on 32-bit architectures.

> +				/* if (R0 < 0) return R0; */
> +				insns[i++] = BPF_JMP_IMM(BPF_JSGE, BPF_REG_0, 0, 1);
> +				insns[i++] = BPF_EXIT_INSN();

[Severity: Critical]
If a memory fault occurs here and the program exits early, does it leave the
remaining trace event ring buffer fields uninitialized?

Since trace_event_buffer_reserve() leaves the ring buffer uninitialized,
terminating the BPF program immediately on a single fault without zeroing
the current field or processing subsequent arguments might expose stale
kernel memory from the stack or heap to userspace when the buffer is
subsequently committed.

> +				/* R0 = *(unsigned long *)(R10 - 8) */
> +				insns[i++] = BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_10, -8);
> +				break;
[ ... ]
> +	/* Epilogue: return R9 (0) */
> +	insns[i++] = BPF_MOV64_REG(BPF_REG_0, BPF_REG_9);
> +	insns[i++] = BPF_EXIT_INSN();
> +
> +	prog = bpf_prog_alloc(bpf_prog_size(i), 0);
> +	if (!prog)
> +		goto out;

[Severity: High]
Is a prog->aux->refcnt initialization needed here?

bpf_prog_alloc() zero-initializes the bpf_prog_aux structure, leaving
refcnt as 0. When bpf_prog_put() is later called during cleanup,
atomic64_dec_and_test(0) will evaluate to false, which causes the
program memory to never be freed.

> +
> +	prog->len = i;
> +	memcpy(prog->insnsi, insns, prog->len * sizeof(struct bpf_insn));
> +	prog->type = BPF_PROG_TYPE_KPROBE;
> +
> +	prog = bpf_prog_select_runtime(prog, &err);
> +	if (IS_ERR(prog))
> +		goto out;
> +	tp->prog = prog;

[Severity: High]
Does this error check properly handle failures from bpf_prog_select_runtime?

bpf_prog_select_runtime() does not return ERR_PTR on failure; instead, it
returns the program pointer and sets the out-parameter err to a negative
error code. Checking IS_ERR(prog) evaluates to false, which might cause
the error to be ignored. If JIT compilation fails and the interpreter is
unavailable, this leaves prog->bpf_func as NULL, leading to a NULL pointer
dereference later.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/178291352217.1566898.14481561093843379745.stgit@devnote2?part=2