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@kernel.org>
Signed-off-by: Jann Horn <jannh@google.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;
 	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)) {
+		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@google.com>

On Tue, Dec 10, 2024 at 7:34 AM Jann Horn <jannh@google.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@kernel.org>
> Signed-off-by: Jann Horn <jannh@google.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@google.com>
>

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