| 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
| ||
|
Message-ID: <CAP-5=fVWC+MeeUcNmofkQi766se4P0ERGz_+L+7uTFUXocwVvQ@mail.gmail.com>
Date: Thu, 29 Feb 2024 17:06:04 -0800
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>, Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>,
Adrian Hunter <adrian.hunter@...el.com>, Yang Jihong <yangjihong1@...wei.com>,
linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] libperf evlist: Avoid out-of-bounds access
On Thu, Feb 29, 2024 at 1:46 PM Namhyung Kim <namhyung@...nel.org> wrote:
>
> On Wed, Feb 28, 2024 at 11:08 PM Ian Rogers <irogers@...gle.com> wrote:
> >
> > Parallel testing appears to show a race between allocating and setting
> > evsel ids. As there is a bounds check on the xyarray it yields a segv
> > like:
> >
> > ```
> > AddressSanitizer:DEADLYSIGNAL
> >
> > =================================================================
> >
> > ==484408==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000010
> >
> > ==484408==The signal is caused by a WRITE memory access.
> >
> > ==484408==Hint: address points to the zero page.
> >
> > #0 0x55cef5d4eff4 in perf_evlist__id_hash tools/lib/perf/evlist.c:256
> > #1 0x55cef5d4f132 in perf_evlist__id_add tools/lib/perf/evlist.c:274
> > #2 0x55cef5d4f545 in perf_evlist__id_add_fd tools/lib/perf/evlist.c:315
> > #3 0x55cef5a1923f in store_evsel_ids util/evsel.c:3130
> > #4 0x55cef5a19400 in evsel__store_ids util/evsel.c:3147
> > #5 0x55cef5888204 in __run_perf_stat tools/perf/builtin-stat.c:832
> > #6 0x55cef5888c06 in run_perf_stat tools/perf/builtin-stat.c:960
> > #7 0x55cef58932db in cmd_stat tools/perf/builtin-stat.c:2878
> > ...
> > ```
> >
> > Avoid this crash by early exiting the perf_evlist__id_add_fd and
> > perf_evlist__id_add is the access is out-of-bounds.
> >
> > Signed-off-by: Ian Rogers <irogers@...gle.com>
>
> While I'm ok with this change, the real fix would be changing
> evsel_store__ids() to use xyarray__max_{x,y} for fd instead
> of cpu and thread map numbers.
So I'm not sure on how to code that fix. Could you take over looking
at this? It reproduces for me with "perf test -v -p" when built with
"-fsanitize=address".
Thanks,
Ian
> Thanks,
> Namhyung
>
> > ---
> > tools/lib/perf/evlist.c | 18 ++++++++++++------
> > tools/lib/perf/include/internal/evlist.h | 4 ++--
> > 2 files changed, 14 insertions(+), 8 deletions(-)
> >
> > diff --git a/tools/lib/perf/evlist.c b/tools/lib/perf/evlist.c
> > index 058e3ff10f9b..c6d67fc9e57e 100644
> > --- a/tools/lib/perf/evlist.c
> > +++ b/tools/lib/perf/evlist.c
> > @@ -248,10 +248,10 @@ u64 perf_evlist__read_format(struct perf_evlist *evlist)
> >
> > static void perf_evlist__id_hash(struct perf_evlist *evlist,
> > struct perf_evsel *evsel,
> > - int cpu, int thread, u64 id)
> > + int cpu_map_idx, int thread, u64 id)
> > {
> > int hash;
> > - struct perf_sample_id *sid = SID(evsel, cpu, thread);
> > + struct perf_sample_id *sid = SID(evsel, cpu_map_idx, thread);
> >
> > sid->id = id;
> > sid->evsel = evsel;
> > @@ -269,21 +269,27 @@ void perf_evlist__reset_id_hash(struct perf_evlist *evlist)
> >
> > void perf_evlist__id_add(struct perf_evlist *evlist,
> > struct perf_evsel *evsel,
> > - int cpu, int thread, u64 id)
> > + int cpu_map_idx, int thread, u64 id)
> > {
> > - perf_evlist__id_hash(evlist, evsel, cpu, thread, id);
> > + if (!SID(evsel, cpu_map_idx, thread))
> > + return;
> > +
> > + perf_evlist__id_hash(evlist, evsel, cpu_map_idx, thread, id);
> > evsel->id[evsel->ids++] = id;
> > }
> >
> > int perf_evlist__id_add_fd(struct perf_evlist *evlist,
> > struct perf_evsel *evsel,
> > - int cpu, int thread, int fd)
> > + int cpu_map_idx, int thread, int fd)
> > {
> > u64 read_data[4] = { 0, };
> > int id_idx = 1; /* The first entry is the counter value */
> > u64 id;
> > int ret;
> >
> > + if (!SID(evsel, cpu_map_idx, thread))
> > + return -1;
> > +
> > ret = ioctl(fd, PERF_EVENT_IOC_ID, &id);
> > if (!ret)
> > goto add;
> > @@ -312,7 +318,7 @@ int perf_evlist__id_add_fd(struct perf_evlist *evlist,
> > id = read_data[id_idx];
> >
> > add:
> > - perf_evlist__id_add(evlist, evsel, cpu, thread, id);
> > + perf_evlist__id_add(evlist, evsel, cpu_map_idx, thread, id);
> > return 0;
> > }
> >
> > diff --git a/tools/lib/perf/include/internal/evlist.h b/tools/lib/perf/include/internal/evlist.h
> > index d86ffe8ed483..f43bdb9b6227 100644
> > --- a/tools/lib/perf/include/internal/evlist.h
> > +++ b/tools/lib/perf/include/internal/evlist.h
> > @@ -126,11 +126,11 @@ u64 perf_evlist__read_format(struct perf_evlist *evlist);
> >
> > void perf_evlist__id_add(struct perf_evlist *evlist,
> > struct perf_evsel *evsel,
> > - int cpu, int thread, u64 id);
> > + int cpu_map_idx, int thread, u64 id);
> >
> > int perf_evlist__id_add_fd(struct perf_evlist *evlist,
> > struct perf_evsel *evsel,
> > - int cpu, int thread, int fd);
> > + int cpu_map_idx, int thread, int fd);
> >
> > void perf_evlist__reset_id_hash(struct perf_evlist *evlist);
> >
> > --
> > 2.44.0.278.ge034bb2e1d-goog
> >
>
Powered by blists - more mailing lists