linux-kernel - Re: [PATCH bpf v3] bpf: Fix theoretical prog_array UAF in __uprobe_perf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAG48ez29vVYF2yWDQEJb=_+u+737aUm4idfkhkEtFQjHivv7-g@mail.gmail.com>
Date: Tue, 10 Dec 2024 20:02:45 +0100
From: Jann Horn <jannh@...gle.com>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, 
	John Fastabend <john.fastabend@...il.com>, Andrii Nakryiko <andrii@...nel.org>, 
	Martin KaFai Lau <martin.lau@...ux.dev>, Eduard Zingerman <eddyz87@...il.com>, Song Liu <song@...nel.org>, 
	Yonghong Song <yonghong.song@...ux.dev>, KP Singh <kpsingh@...nel.org>, 
	Stanislav Fomichev <sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>, 
	Steven Rostedt <rostedt@...dmis.org>, Masami Hiramatsu <mhiramat@...nel.org>, 
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>, Delyan Kratunov <delyank@...com>, bpf@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH bpf v3] bpf: Fix theoretical prog_array UAF in __uprobe_perf_func()

On Tue, Dec 10, 2024 at 6:52 PM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
> On Tue, Dec 10, 2024 at 7:34 AM Jann Horn <jannh@...gle.com> 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@...nel.org>
> > Signed-off-by: Jann Horn <jannh@...gle.com>
> > ---
> > 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@google.com
> >
> > 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@google.com
> > ---
> >  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.