On Tue, Dec 10, 2024 at 6:52 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
> On Tue, Dec 10, 2024 at 7:34 AM Jann Horn <jannh@xxxxxxxxxx> wrote:
> > Currently, the pointer stored in call->prog_array is loaded in
> > __uprobe_perf_func(), with no RCU annotation and no immediately visible
> > RCU protection, so it looks as if the loaded pointer can immediately be
> > dangling.
> > Later, bpf_prog_run_array_uprobe() starts a RCU-trace read-side critical
> > section, but this is too late. It then uses rcu_dereference_check(), but
> > this use of rcu_dereference_check() does not actually dereference anything.
> >
> > Fix it by aligning the semantics to bpf_prog_run_array(): Let the caller
> > provide rcu_read_lock_trace() protection and then load call->prog_array
> > with rcu_dereference_check().
> >
> > This issue seems to be theoretical: I don't know of any way to reach this
> > code without having handle_swbp() further up the stack, which is already
> > holding a rcu_read_lock_trace() lock, so where we take
> > rcu_read_lock_trace() in __uprobe_perf_func()/bpf_prog_run_array_uprobe()
> > doesn't actually have any effect.
> >
> > Fixes: 8c7dcb84e3b7 ("bpf: implement sleepable uprobes by chaining gps")
> > Suggested-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > Signed-off-by: Jann Horn <jannh@xxxxxxxxxx>
> > ---
> > Changes in v3:
> > - align semantics with bpf_prog_run_array()
> > - correct commit message: the issue is theoretical
> > - remove stable CC
> > - Link to v2: https://lore.kernel.org/r/20241206-bpf-fix-uprobe-uaf-v2-1-4c75c54fe424@xxxxxxxxxx
> >
> > Changes in v2:
> > - remove diff chunk in patch notes that confuses git
> > - Link to v1: https://lore.kernel.org/r/20241206-bpf-fix-uprobe-uaf-v1-1-6869c8a17258@xxxxxxxxxx
> > ---
> > include/linux/bpf.h | 11 +++--------
> > kernel/trace/trace_uprobe.c | 6 +++++-
> > 2 files changed, 8 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index eaee2a819f4c150a34a7b1075584711609682e4c..7fe5cf181511d543b1b100028db94ebb2a44da5d 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -2193,26 +2193,22 @@ bpf_prog_run_array(const struct bpf_prog_array *array,
> > * rcu-protected dynamically sized maps.
> > */
> > static __always_inline u32
> > -bpf_prog_run_array_uprobe(const struct bpf_prog_array __rcu *array_rcu,
> > +bpf_prog_run_array_uprobe(const struct bpf_prog_array *array,
> > const void *ctx, bpf_prog_run_fn run_prog)
> > {
> > const struct bpf_prog_array_item *item;
> > const struct bpf_prog *prog;
> > - const struct bpf_prog_array *array;
> > struct bpf_run_ctx *old_run_ctx;
> > struct bpf_trace_run_ctx run_ctx;
> > u32 ret = 1;
> >
> > might_fault();
> > + RCU_LOCKDEP_WARN(!rcu_read_lock_trace_held(), "no rcu lock held");
> >
> > - rcu_read_lock_trace();
> > migrate_disable();
> >
> > run_ctx.is_uprobe = true;
> >
> > - array = rcu_dereference_check(array_rcu, rcu_read_lock_trace_held());
> > - if (unlikely(!array))
> > - goto out;
>
> I think we should keep this unlikely(NULL) check, bpf_prog_run_array()
> has it and see bpf_prog_array_valid() comment below
Whoops, yeah, I removed it here at some point while moving the
dereference around and then forgot to re-add it; will fix.
