* [PATCH v1] libperf evlist: Avoid out-of-bounds access
@ 2024-02-29 7:07 Ian Rogers
2024-02-29 21:45 ` Namhyung Kim
2024-03-01 19:30 ` Namhyung Kim
0 siblings, 2 replies; 5+ messages in thread
From: Ian Rogers @ 2024-02-29 7:07 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, Yang Jihong, linux-perf-users,
linux-kernel
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@google.com>
---
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v1] libperf evlist: Avoid out-of-bounds access
2024-02-29 7:07 [PATCH v1] libperf evlist: Avoid out-of-bounds access Ian Rogers
@ 2024-02-29 21:45 ` Namhyung Kim
2024-03-01 1:06 ` Ian Rogers
2024-03-01 19:30 ` Namhyung Kim
1 sibling, 1 reply; 5+ messages in thread
From: Namhyung Kim @ 2024-02-29 21:45 UTC (permalink / raw)
To: Ian Rogers
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Adrian Hunter,
Yang Jihong, linux-perf-users, linux-kernel
On Wed, Feb 28, 2024 at 11:08 PM Ian Rogers <irogers@google.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@google.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.
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v1] libperf evlist: Avoid out-of-bounds access
2024-02-29 21:45 ` Namhyung Kim
@ 2024-03-01 1:06 ` Ian Rogers
2024-03-01 1:58 ` Namhyung Kim
0 siblings, 1 reply; 5+ messages in thread
From: Ian Rogers @ 2024-03-01 1:06 UTC (permalink / raw)
To: Namhyung Kim
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Adrian Hunter,
Yang Jihong, linux-perf-users, linux-kernel
On Thu, Feb 29, 2024 at 1:46 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Wed, Feb 28, 2024 at 11:08 PM Ian Rogers <irogers@google.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@google.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
> >
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v1] libperf evlist: Avoid out-of-bounds access
2024-03-01 1:06 ` Ian Rogers
@ 2024-03-01 1:58 ` Namhyung Kim
0 siblings, 0 replies; 5+ messages in thread
From: Namhyung Kim @ 2024-03-01 1:58 UTC (permalink / raw)
To: Ian Rogers
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Adrian Hunter,
Yang Jihong, linux-perf-users, linux-kernel
On Thu, Feb 29, 2024 at 5:06 PM Ian Rogers <irogers@google.com> wrote:
>
> On Thu, Feb 29, 2024 at 1:46 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Wed, Feb 28, 2024 at 11:08 PM Ian Rogers <irogers@google.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@google.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".
Ok, but now I think that the fd array should have the same dimension
with the id array. I'm not sure where it can change but I'll take a look
later. Let's apply this one first.
Thanks,
Namhyung
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1] libperf evlist: Avoid out-of-bounds access
2024-02-29 7:07 [PATCH v1] libperf evlist: Avoid out-of-bounds access Ian Rogers
2024-02-29 21:45 ` Namhyung Kim
@ 2024-03-01 19:30 ` Namhyung Kim
1 sibling, 0 replies; 5+ messages in thread
From: Namhyung Kim @ 2024-03-01 19:30 UTC (permalink / raw)
To: Ian Rogers, Peter Zijlstra, Alexander Shishkin, Mark Rutland,
linux-kernel, Jiri Olsa, Adrian Hunter, Arnaldo Carvalho de Melo,
linux-perf-users, Yang Jihong, Ingo Molnar
On Wed, 28 Feb 2024 23:07:57 -0800, Ian Rogers 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
>
> [...]
Applied to perf-tools-next, thanks!
Best regards,
--
Namhyung Kim <namhyung@kernel.org>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-03-01 19:30 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-29 7:07 [PATCH v1] libperf evlist: Avoid out-of-bounds access Ian Rogers
2024-02-29 21:45 ` Namhyung Kim
2024-03-01 1:06 ` Ian Rogers
2024-03-01 1:58 ` Namhyung Kim
2024-03-01 19:30 ` Namhyung Kim
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®