perf_evsel sample_id is an xyarray which can cause a segfault when
accessed beyond its size. e.g.

 # perf record -e intel_pt// -C 1 sleep 1
 Segmentation fault (core dumped)

That is happening because a dummy event is opened to capture text poke
events accoss all CPUs, however the mmap logic is allocating according
to the number of user_requested_cpus.

In general, perf sometimes uses the evsel cpus to open events, and
sometimes the evlist user_requested_cpus. However, it is not necessary
to determine which case is which because the opened event file
descriptors are also in an xyarray, the size of whch can be used
to correctly allocate the size of the sample_id xyarray, because there
is one ID per file descriptor. Note, in the affected code path,
perf_evsel fd array is subsequently used to get the file descriptor for
the mmap, so it makes sense for the xyarrays to be the same size there.

Fixes: 246eba8e9041c4 ("perf tools: Add support for PERF_RECORD_TEXT_POKE")
Fixes: d1a177595b3a82 ("libperf: Adopt perf_evlist__mmap()/munmap() from tools/perf")
Cc: stable@vger.kernel.org # 5.5+
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/lib/perf/evlist.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/lib/perf/evlist.c b/tools/lib/perf/evlist.c
index 1b15ba13c477..a09315538a30 100644
--- a/tools/lib/perf/evlist.c
+++ b/tools/lib/perf/evlist.c
@@ -577,7 +577,6 @@ int perf_evlist__mmap_ops(struct perf_evlist *evlist,
 {
 	struct perf_evsel *evsel;
 	const struct perf_cpu_map *cpus = evlist->user_requested_cpus;
-	const struct perf_thread_map *threads = evlist->threads;
 
 	if (!ops || !ops->get || !ops->mmap)
 		return -EINVAL;
@@ -589,7 +588,7 @@ int perf_evlist__mmap_ops(struct perf_evlist *evlist,
 	perf_evlist__for_each_entry(evlist, evsel) {
 		if ((evsel->attr.read_format & PERF_FORMAT_ID) &&
 		    evsel->sample_id == NULL &&
-		    perf_evsel__alloc_id(evsel, perf_cpu_map__nr(cpus), threads->nr) < 0)
+		    perf_evsel__alloc_id(evsel, evsel->fd->max_x, evsel->fd->max_y) < 0)
 			return -ENOMEM;
 	}
 
-- 
2.25.1

On Wed, Apr 13, 2022 at 4:42 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> perf_evsel sample_id is an xyarray which can cause a segfault when
> accessed beyond its size. e.g.
>
>  # perf record -e intel_pt// -C 1 sleep 1
>  Segmentation fault (core dumped)
>
> That is happening because a dummy event is opened to capture text poke
> events accoss all CPUs, however the mmap logic is allocating according
> to the number of user_requested_cpus.

Nit: typo on 'accoss'

> In general, perf sometimes uses the evsel cpus to open events, and
> sometimes the evlist user_requested_cpus. However, it is not necessary
> to determine which case is which because the opened event file
> descriptors are also in an xyarray, the size of whch can be used
> to correctly allocate the size of the sample_id xyarray, because there
> is one ID per file descriptor. Note, in the affected code path,
> perf_evsel fd array is subsequently used to get the file descriptor for
> the mmap, so it makes sense for the xyarrays to be the same size there.
>
> Fixes: 246eba8e9041c4 ("perf tools: Add support for PERF_RECORD_TEXT_POKE")
> Fixes: d1a177595b3a82 ("libperf: Adopt perf_evlist__mmap()/munmap() from tools/perf")
> Cc: stable@vger.kernel.org # 5.5+
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/lib/perf/evlist.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/tools/lib/perf/evlist.c b/tools/lib/perf/evlist.c
> index 1b15ba13c477..a09315538a30 100644
> --- a/tools/lib/perf/evlist.c
> +++ b/tools/lib/perf/evlist.c
> @@ -577,7 +577,6 @@ int perf_evlist__mmap_ops(struct perf_evlist *evlist,
>  {
>         struct perf_evsel *evsel;
>         const struct perf_cpu_map *cpus = evlist->user_requested_cpus;
> -       const struct perf_thread_map *threads = evlist->threads;
>
>         if (!ops || !ops->get || !ops->mmap)
>                 return -EINVAL;
> @@ -589,7 +588,7 @@ int perf_evlist__mmap_ops(struct perf_evlist *evlist,
>         perf_evlist__for_each_entry(evlist, evsel) {
>                 if ((evsel->attr.read_format & PERF_FORMAT_ID) &&
>                     evsel->sample_id == NULL &&
> -                   perf_evsel__alloc_id(evsel, perf_cpu_map__nr(cpus), threads->nr) < 0)
> +                   perf_evsel__alloc_id(evsel, evsel->fd->max_x, evsel->fd->max_y) < 0)
>                         return -ENOMEM;
>         }
>
> --
> 2.25.1
>

Em Wed, Apr 13, 2022 at 02:11:41PM -0700, Ian Rogers escreveu:
> On Wed, Apr 13, 2022 at 4:42 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
> >
> > perf_evsel sample_id is an xyarray which can cause a segfault when
> > accessed beyond its size. e.g.
> >
> >  # perf record -e intel_pt// -C 1 sleep 1
> >  Segmentation fault (core dumped)
> >
> > That is happening because a dummy event is opened to capture text poke
> > events accoss all CPUs, however the mmap logic is allocating according
> > to the number of user_requested_cpus.
> 
> Nit: typo on 'accoss'

Fixed
 
> > In general, perf sometimes uses the evsel cpus to open events, and
> > sometimes the evlist user_requested_cpus. However, it is not necessary
> > to determine which case is which because the opened event file
> > descriptors are also in an xyarray, the size of whch can be used
> > to correctly allocate the size of the sample_id xyarray, because there
> > is one ID per file descriptor. Note, in the affected code path,
> > perf_evsel fd array is subsequently used to get the file descriptor for
> > the mmap, so it makes sense for the xyarrays to be the same size there.
> >
> > Fixes: 246eba8e9041c4 ("perf tools: Add support for PERF_RECORD_TEXT_POKE")
> > Fixes: d1a177595b3a82 ("libperf: Adopt perf_evlist__mmap()/munmap() from tools/perf")
> > Cc: stable@vger.kernel.org # 5.5+
> > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> 
> Acked-by: Ian Rogers <irogers@google.com>

Thanks, applied.

- Arnaldo

 
> Thanks,
> Ian
> 
> > ---
> >  tools/lib/perf/evlist.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/tools/lib/perf/evlist.c b/tools/lib/perf/evlist.c
> > index 1b15ba13c477..a09315538a30 100644
> > --- a/tools/lib/perf/evlist.c
> > +++ b/tools/lib/perf/evlist.c
> > @@ -577,7 +577,6 @@ int perf_evlist__mmap_ops(struct perf_evlist *evlist,
> >  {
> >         struct perf_evsel *evsel;
> >         const struct perf_cpu_map *cpus = evlist->user_requested_cpus;
> > -       const struct perf_thread_map *threads = evlist->threads;
> >
> >         if (!ops || !ops->get || !ops->mmap)
> >                 return -EINVAL;
> > @@ -589,7 +588,7 @@ int perf_evlist__mmap_ops(struct perf_evlist *evlist,
> >         perf_evlist__for_each_entry(evlist, evsel) {
> >                 if ((evsel->attr.read_format & PERF_FORMAT_ID) &&
> >                     evsel->sample_id == NULL &&
> > -                   perf_evsel__alloc_id(evsel, perf_cpu_map__nr(cpus), threads->nr) < 0)
> > +                   perf_evsel__alloc_id(evsel, evsel->fd->max_x, evsel->fd->max_y) < 0)
> >                         return -ENOMEM;
> >         }
> >
> > --
> > 2.25.1
> >

-- 

- Arnaldo