Received-SPF: pass (google.com: domain of linux-kernel+bounces-87596-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240229070757.796244-1-irogers@google.com>
In-Reply-To: <20240229070757.796244-1-irogers@google.com>
From: Namhyung Kim <namhyung@kernel.org>
Date: Thu, 29 Feb 2024 13:45:56 -0800
Message-ID: <CAM9d7chNekmdJyPbOOzZVhR+cBWajscSg3pVFKN4A4evKwFXjg@mail.gmail.com>
Subject: Re: [PATCH v1] libperf evlist: Avoid out-of-bounds access
To: Ian Rogers <irogers@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>, 
	Arnaldo Carvalho de Melo <acme@kernel.org>, Mark Rutland <mark.rutland@arm.com>, 
	Alexander Shishkin <alexander.shishkin@linux.intel.com>, Jiri Olsa <jolsa@kernel.org>, 
	Adrian Hunter <adrian.hunter@intel.com>, Yang Jihong <yangjihong1@huawei.com>, 
	linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Feb 28, 2024 at 11:08=E2=80=AFPM Ian Rogers <irogers@google.com> wr=
ote:
>
> 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
>
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>
> =3D=3D484408=3D=3DERROR: AddressSanitizer: SEGV on unknown address 0x0000=
00000010
>
> =3D=3D484408=3D=3DThe signal is caused by a WRITE memory access.
>
> =3D=3D484408=3D=3DHint: 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:3=
15
>     #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 *ev=
list)
>
>  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 =3D SID(evsel, cpu, thread);
> +       struct perf_sample_id *sid =3D SID(evsel, cpu_map_idx, thread);
>
>         sid->id =3D id;
>         sid->evsel =3D 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++] =3D 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] =3D { 0, };
>         int id_idx =3D 1; /* The first entry is the counter value */
>         u64 id;
>         int ret;
>
> +       if (!SID(evsel, cpu_map_idx, thread))
> +               return -1;
> +
>         ret =3D 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 =3D 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/in=
clude/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 *ev=
list);
>
>  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
>