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] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAEf4BzacBUdttSi9d0Ecud7XEgdMrzsbZa0wmpFceLRwjQ-=dg@mail.gmail.com>
Date: Tue, 10 Dec 2024 09:52:21 -0800
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Jann Horn <jannh@...gle.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 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

pw-bot: cr


>         old_run_ctx = bpf_set_run_ctx(&run_ctx.run_ctx);
>         item = &array->items[0];
>         while ((prog = READ_ONCE(item->prog))) {
> @@ -2227,9 +2223,8 @@ bpf_prog_run_array_uprobe(const struct bpf_prog_array __rcu *array_rcu,
>                         rcu_read_unlock();
>         }
>         bpf_reset_run_ctx(old_run_ctx);
> -out:
> +
>         migrate_enable();
> -       rcu_read_unlock_trace();
>         return ret;
>  }
>
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index fed382b7881b82ee3c334ea77860cce77581a74d..4875e7f5de3db249af34c539c079fbedd38f4107 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -1402,9 +1402,13 @@ static void __uprobe_perf_func(struct trace_uprobe *tu,
>
>  #ifdef CONFIG_BPF_EVENTS
>         if (bpf_prog_array_valid(call)) {

bpf_prog_array_valid() explicitly calls out that it's just an
opportunistic check and bpf_prog_run_array*() should double check for
NULL

> +               const struct bpf_prog_array *array;
>                 u32 ret;
>
> -               ret = bpf_prog_run_array_uprobe(call->prog_array, regs, bpf_prog_run);
> +               rcu_read_lock_trace();
> +               array = rcu_dereference_check(call->prog_array, rcu_read_lock_trace_held());
> +               ret = bpf_prog_run_array_uprobe(array, regs, bpf_prog_run);
> +               rcu_read_unlock_trace();
>                 if (!ret)
>                         return;
>         }
>
> ---
> base-commit: 509df676c2d79c985ec2eaa3e3a3bbe557645861
> change-id: 20241206-bpf-fix-uprobe-uaf-53d928bab3d0
>
> --
> Jann Horn <jannh@...gle.com>
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ