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

[Jiri Olsa <olsajiri@gmail.com>]

On Mon, Aug 03, 2026 at 09:30:50PM +0000, sashiko-bot@kernel.org wrote:
> 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?

there's preemption fix for this later in the patchset

jirka