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
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@xxxxxxxxxx>
>
