Re: [PATCHv4 bpf-next 10/12] bpf: Disable preemption in __bpf_get_stack

[Jiri Olsa <olsajiri@gmail.com>]

On Wed, Aug 05, 2026 at 11:34:58AM -0700, Andrii Nakryiko wrote:
> On Wed, Aug 5, 2026 at 2:30 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > From: Daniel Borkmann <daniel@iogearbox.net>
> >
> > get_perf_callchain() returns a per-CPU perf_callchain_entry buffer and
> > releases its recursion slot via put_callchain_entry() before returning,
> > so nothing keeps the entry reserved while __bpf_get_stack() consumes
> > it below.
> >
> > A preemptible BPF program (e.g. a non-sleepable raw tracepoint program
> > on a PREEMPT kernel, which runs under migrate_disable() but not
> > preempt_disable()) can be scheduled out between obtaining the entry
> > and the copy. Another task scheduled on the same CPU then reuses the
> > same per-CPU buffer and overwrites trace->nr with a larger value.
> > copy_len is then computed from the inflated trace->nr and can exceed
> > the caller's buffer, causing an out-of-bounds write in the memcpy()
> > and in the build_id path.
> >
> > The rcu_read_lock() taken here alone does not prevent this. It is
> > only taken on the may_fault path, and under CONFIG_PREEMPT_RCU it does
> > not disable preemption; it merely keeps perf's callchain buffer array
> > alive (freed via call_rcu()) and does nothing to stop another task
> > from reusing the entry.
> >
> > Disable preemption around obtaining the callchain entry and copying
> > it into the caller's buffer, so the entry cannot be reused underneath
> > us and trace->nr stays bounded by max_depth. Build ID resolution may
> > fault and is therefore deferred until after preemption is re-enabled;
> > by then the instruction pointers have already been copied into buf,
> > so it operates only on that private copy. Note, preempt_disable() also
> > subsumes the buffer-lifetime guarantee the rcu_read_lock() provided,
> > since a preempt-disabled section is an RCU read-side critical section
> > for the callchain buffers' call_rcu() reclaim.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: c195651e565a ("bpf: add bpf_get_stack helper")
> > Reported-by: Tao Chen <chen.dylane@linux.dev>
> > Closes: https://lore.kernel.org/bpf/20260206090653.1336687-1-chen.dylane@linux.dev/
> > Reported-by: STAR Labs SG <info@starlabs.sg>
> > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> > [ changed Fixes: commit ]
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  kernel/bpf/stackmap.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> > index eabeaef31b63..789fe35b893a 100644
> > --- a/kernel/bpf/stackmap.c
> > +++ b/kernel/bpf/stackmap.c
> > @@ -817,6 +817,7 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task,
> >
> >         if (may_fault)
> >                 rcu_read_lock(); /* need RCU for perf's callchain below */
> > +       preempt_disable();
> 
> nit: asymmetrical to preempt_enable, I'll move it to before
> rcu_read_lock, so we can have proper nesting

ugh, nice.. thanks

jirka

> 
> 
> 
> >
> >         if (kernel && task) {
> >                 trace = get_callchain_entry_for_task(task, max_depth);
> > @@ -828,6 +829,7 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task,
> >         if (unlikely(!trace) || trace->nr < skip) {
> >                 if (may_fault)
> >                         rcu_read_unlock();
> > +               preempt_enable();
> >                 goto err_fault;
> >         }
> >
> > @@ -836,6 +838,7 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task,
> >         /* trace should not be dereferenced after this point */
> >         if (may_fault)
> >                 rcu_read_unlock();
> > +       preempt_enable();
> >
> >         return callchain_finalize(buf, size, trace_nr, elem_size, flags, may_fault);
> >
> > --
> > 2.54.0
> >