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: <20240812192405.GD11656@redhat.com>
Date: Mon, 12 Aug 2024 21:24:31 +0200
From: Oleg Nesterov <oleg@...hat.com>
To: Andrii Nakryiko <andrii.nakryiko@...il.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 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.

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.

-------------------------------------------------------------------------------
> 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.