linux-kernel - Re: [syzbot] [perf?] KASAN: slab-use-after-free Read in __uprobe

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: <CAEf4Bza8Ptd4eLfhqci2OVgGQZYrFC-bn-250ErFPcsKzQoRXA@mail.gmail.com>
Date: Sun, 11 Aug 2024 21:20:55 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Oleg Nesterov <oleg@...hat.com>
Cc: syzbot <syzbot+f7a1c2c2711e4a780f19@...kaller.appspotmail.com>, 
	Andrii Nakryiko <andrii@...nel.org>, jolsa@...nel.org, acme@...nel.org, 
	adrian.hunter@...el.com, alexander.shishkin@...ux.intel.com, 
	irogers@...gle.com, kan.liang@...ux.intel.com, linux-kernel@...r.kernel.org, 
	linux-perf-users@...r.kernel.org, linux-trace-kernel@...r.kernel.org, 
	mark.rutland@....com, mhiramat@...nel.org, mingo@...hat.com, 
	namhyung@...nel.org, peterz@...radead.org, syzkaller-bugs@...glegroups.com
Subject: Re: [syzbot] [perf?] KASAN: slab-use-after-free Read in __uprobe_unregister

On Sun, Aug 11, 2024 at 5:35 AM Oleg Nesterov <oleg@...hat.com> wrote:
>
> On 08/11, Oleg Nesterov wrote:
> >
> > Hmm, bpf_uprobe_multi_link_attach() looks obviously wrong.
> >
> > bpf_link_prime() is called after the
> >
> >       for (i = 0; i < cnt; i++) {
> >               uprobe_register(...);
> >               ...
> >       }
> >
> > loop. If bpf_link_prime() fails, bpf_uprobe_multi_link_attach() just do
> > kvfree(uprobes) without _unregister(). In particular, this leaks the freed
> > bpf_uprobe->consumer in the uprobe->consumers list.
> >
> > After that another _unregister() on the same uprobe can hit the problem.
> >
> > I guess we need a simple patch for -stable...
>
> Something like below on top of perf/core. But I don't like the usage of
> "i" in the +error_unregister path...
>

Wouldn't the below be cleaner?

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index cd098846e251..3ca65454f888 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -3491,8 +3491,10 @@ int bpf_uprobe_multi_link_attach(const union
bpf_attr *attr, struct bpf_prog *pr
        }

        err = bpf_link_prime(&link->link, &link_primer);
-       if (err)
+       if (err) {
+               bpf_uprobe_unregister(&path, uprobes, cnt);
                goto error_free;
+       }

        return bpf_link_settle(&link_primer);


We should probably route this through the bpf tree, I don't think it
will conflict with your changes, right?


> Oleg.
>
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -3486,17 +3486,19 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>                                                     &uprobes[i].consumer);
>                 if (IS_ERR(uprobes[i].uprobe)) {
>                         err = PTR_ERR(uprobes[i].uprobe);
> -                       bpf_uprobe_unregister(uprobes, i);
> -                       goto error_free;
> +                       goto error_unregister;
>                 }
>         }
>
>         err = bpf_link_prime(&link->link, &link_primer);
>         if (err)
> -               goto error_free;
> +               goto error_unregister;
>
>         return bpf_link_settle(&link_primer);
>
> +error_unregister:
> +       bpf_uprobe_unregister(uprobes, i);
> +
>  error_free:
>         kvfree(uprobes);
>         kfree(link);
>