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] [day] [month] [year] [list]

Message-ID: <CAEf4BzbW9frgUDE+9mbrAyMj0eshPOoqgLs11ynb91spWkdAzw@mail.gmail.com>
Date: Mon, 12 Aug 2024 13:00:00 -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, 
	bpf <bpf@...r.kernel.org>
Subject: Re: [syzbot] [perf?] KASAN: slab-use-after-free Read in __uprobe_unregister

On Mon, Aug 12, 2024 at 12:25 PM Oleg Nesterov <oleg@...hat.com> wrote:
>
> On 08/12, Andrii Nakryiko wrote:
> >
> > adding bpf ML, given it's bpf's code base
>
> Thanks,
>
> > On Mon, Aug 12, 2024 at 3:00 AM Oleg Nesterov <oleg@...hat.com> wrote:
> > >
> > > > --- 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);
> > >
> > > I disagree. This code already uses the "goto error_xxx" pattern, why
> >
> > Well, if you have strong preferences,
>
> Well, YES and NO ;) please see below.
>
> > so be it (it's too trivial code
> > to argue about).
>
> Agreed. On a closer look both the code and the problem look very trivial.
>
> But note that nobody noticed this trivial problem before. Including me who
> had to change this trivial code to adapt to the recent API changes.

Yep, error handling problems tend to go unnoticed frequently. The good
thing is that this is quite unlikely in practice for bpf_link_prime()
to fail, which is why it was a syzbot that found this.

>
> May be this means that we should keep the error handling in this function
> more consistent ;)
>
> > We do have quite a lot of "hybrid" error handling
>
> And YES, I don't like this kind of error handling.
>
> But, at the same time: NO, I never-never argue with the maintainers when it
> comes to "cosmetic" issues.
>
> My main point was (and you seem to agree) that this simpler patch above won't
> simplify the routing. I too thought about the change above initially.
>

Agreed. Just stick to your code, it's fine. Thanks!

> -------------------------------------------------------------------------------
> > Yep, absolutely, given the bpf_uprobe_unregister() change, I don't see
> > any problem for it to go together with your refactorings.
> >
> > For the fix:
> >
> > Acked-by: Andrii Nakryiko <andrii@...nel.org>
>
> Thanks! I'll write the changelog and send this patch with your ack included
> tomorrow.
>
> Oleg.
>