Re: [PATCHv3 bpf-next 06/12] bpf: Factor callchain_store function from __bpf_get_stack

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] A Time-of-Check to Time-of-Use (TOCTOU) data race exists in `__bpf_get_stack` when used from a sleepable BPF program, allowing concurrent modification of `trace->nr` that leads to a massive Out-of-Bounds (OOB) memory read and write.
--

--- Patch [1]: [PATCHv3 bpf-next 06/12] bpf: Factor callchain_store function from __bpf_get_stack ---
commit 1569be7d337e247b4d7eb8620dff965cc57a3fc6
Author: Jiri Olsa <jolsa@kernel.org>
bpf: Factor callchain_store function from __bpf_get_stack

This commit factors out the callchain storage logic into a new
callchain_store() function. It handles extracting both just-ip and buildid
data into a user-supplied buffer.

> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> index 463f94ba1cc47..e51e03c4b60ed 100644
> --- a/kernel/bpf/stackmap.c
> +++ b/kernel/bpf/stackmap.c
[ ... ]
> @@ -680,6 +680,29 @@ const struct bpf_func_proto bpf_get_stackid_proto_pe = {
>  	.arg3_type	= ARG_ANYTHING,
>  };
>  
> +static u32 callchain_store(struct perf_callchain_entry *trace, void *buf,
> +			   u32 elem_size, u64 flags)
> +{
> +	bool user_build_id = flags & BPF_F_USER_BUILD_ID;
> +	u32 skip = flags & BPF_F_SKIP_FIELD_MASK;
> +	u32 trace_nr, copy_len;
> +	u64 *ips;
> +
> +	trace_nr = trace->nr - skip;

[Severity: Critical]
This is a pre-existing issue, but could this unprotected re-read of
trace->nr lead to an integer underflow and out-of-bounds memory access?

If __bpf_get_stack() is called from a sleepable BPF program, preemption
is enabled. Between the bounds check in __bpf_get_stack() and this read,
the task could be preempted. If a preempting task running on the same CPU
executes a BPF program and reallocates the identical per-CPU buffer, it
might overwrite trace->nr with a smaller value.

When the sleepable task resumes, this re-read of trace->nr could result in
a value smaller than skip, causing trace_nr to underflow and resulting in
a massive copy_len during the memcpy() below.

> +	copy_len = trace_nr * elem_size;
> +
> +	ips = trace->ip + skip;
> +	if (user_build_id) {
> +		struct bpf_stack_build_id *id_offs = buf;
> +
> +		for (u32 i = 0; i < trace_nr; i++)
> +			id_offs[i].ip = ips[i];
> +	} else {
> +		memcpy(buf, ips, copy_len);
> +	}
> +	return trace_nr;
> +}
> +
>  static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task,
[ ... ]
> @@ -737,21 +759,10 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task,
>  		goto err_fault;
>  	}
>  
> -	trace_nr = trace->nr - skip;
> +	trace_nr = callchain_store(trace, buf, elem_size, flags);

[Severity: Critical]
This isn't a bug introduced by this patch, but this is the preemption
window mentioned above.

The check if (unlikely(!trace) || trace->nr < skip) happens right before
this call. For sleepable BPF programs, preemption can occur between the
bounds check and the re-reading of trace->nr inside callchain_store().

Could we cache the validated trace->nr value (e.g., using READ_ONCE())
and pass it to callchain_store() to avoid this Time-of-Check to
Time-of-Use race?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803210149.296496-7-jolsa@kernel.org?part=1