[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240812100028.GA11656@redhat.com>
Date: Mon, 12 Aug 2024 12:00:29 +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
Subject: Re: [syzbot] [perf?] KASAN: slab-use-after-free Read in
__uprobe_unregister
On 08/11, Andrii Nakryiko wrote:
>
> On Sun, Aug 11, 2024 at 5:35 AM Oleg Nesterov <oleg@...hat.com> wrote:
> >
> > 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);
I disagree. This code already uses the "goto error_xxx" pattern, why
duplicate bpf_uprobe_unregister() ? What if another "can fail" code
comes between register and bpf_link_prime() ?
See the patch below, on top of perf/core.
> We should probably route this through the bpf tree, I don't think it
> will conflict with your changes, right?
It will conflict, and in this sense it is even worse than the "#syz test"
patch I sent in https://lore.kernel.org/all/20240811125816.GC30068@redhat.com/
Because with your version above the necessary change
- bpf_uprobe_unregister(&path, uprobes, cnt);
+ bpf_uprobe_unregister(uprobes, cnt);
won't be noticed during the merge, I guess.
So can we route this fix through the perf/core ? I'll add "cc: stable",
in the next merge window the Greg's scripts will report the "FAILED"
status of the -stable patch, I'll send the trivial backport in reply.
No?
Oleg.
---
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 4e391daafa64..90cd30e9723e 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -3484,17 +3484,20 @@ 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;
+ link->cnt = i;
+ 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, link->cnt);
+
error_free:
kvfree(uprobes);
kfree(link);
Powered by blists - more mailing lists