linux-kernel - Re: [PATCH v2] perf parse-events: Prevent null pointer dereference in __add

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAP-5=fUWP46kdXY4xaA6QzqG+Lj+b4ZGQmjrMKzXZpwSg60idQ@mail.gmail.com>
Date: Mon, 18 Aug 2025 14:28:17 -0700
From: Ian Rogers <irogers@...gle.com>
To: zhaoguohan@...inos.cn
Cc: peterz@...radead.org, mingo@...hat.com, acme@...nel.org, 
	namhyung@...nel.org, mark.rutland@....com, alexander.shishkin@...ux.intel.com, 
	jolsa@...nel.org, adrian.hunter@...el.com, kan.liang@...ux.intel.com, 
	thomas.falcon@...el.com, 
	"open list:PERFORMANCE EVENTS SUBSYSTEM" <linux-perf-users@...r.kernel.org>, 
	"open list:PERFORMANCE EVENTS SUBSYSTEM" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] perf parse-events: Prevent null pointer dereference in __add_event()

On Fri, Aug 15, 2025 at 12:20 AM <zhaoguohan@...inos.cn> wrote:
>
> From: GuoHan Zhao <zhaoguohan@...inos.cn>
>
> In the error handling path of __add_event(), if evsel__new_idx() fails
> and returns NULL, the subsequent calls to zfree(&evsel->name) and
> zfree(&evsel->metric_id) will cause null pointer dereference.
>
> Extend the goto chain to properly handle the case where evsel allocation
> fails, avoiding unnecessary cleanup operations on a NULL pointer.
>
> Fixes: cd63c2216825 ("perf parse-events: Minor __add_event refactoring")
> Signed-off-by: GuoHan Zhao <zhaoguohan@...inos.cn>

Reviewed-by: Ian Rogers <irogers@...gle.com>

> Changes in V2:
> - Extended the goto chain with separate error handling labels instead of using null pointer check
> - Reordered jump targets to avoid accessing NULL evsel members
> - Added Fixes tag
> - Updated commit subject to use "Prevent" instead of "Fix"
> ---
>  tools/perf/util/parse-events.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 8282ddf68b98..8a1fc5d024bf 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -277,18 +277,18 @@ __add_event(struct list_head *list, int *idx,
>
>         evsel = evsel__new_idx(attr, *idx);
>         if (!evsel)
> -               goto out_err;
> +               goto out_free_cpus;

nit: can we call this out_put_cpus rather than free?

Thanks,
Ian

>
>         if (name) {
>                 evsel->name = strdup(name);
>                 if (!evsel->name)
> -                       goto out_err;
> +                       goto out_free_evsel;
>         }
>
>         if (metric_id) {
>                 evsel->metric_id = strdup(metric_id);
>                 if (!evsel->metric_id)
> -                       goto out_err;
> +                       goto out_free_evsel;
>         }
>
>         (*idx)++;
> @@ -310,12 +310,15 @@ __add_event(struct list_head *list, int *idx,
>                 evsel__warn_user_requested_cpus(evsel, user_cpus);
>
>         return evsel;
> -out_err:
> -       perf_cpu_map__put(cpus);
> -       perf_cpu_map__put(pmu_cpus);
> +
> +out_free_evsel:
>         zfree(&evsel->name);
>         zfree(&evsel->metric_id);
>         free(evsel);
> +out_free_cpus:
> +       perf_cpu_map__put(cpus);
> +       perf_cpu_map__put(pmu_cpus);
> +
>         return NULL;
>  }
>
> --
> 2.43.0
>