[PATCH 1/2] perf: do not call zfree() on uninitialized pointer

[PATCH 1/2] perf: do not call zfree() on uninitialized pointer

[Jiri Slaby (SUSE)]

In add_drm_pmu(), the check for type > PERF_PMU_TYPE_DRM_END happens
before "drm" is allocated by zalloc(). The error path nevertheless
called zfree(&drm), i.e. free() on an uninitialized pointer, which
can crash. Remove this bogus zfree() -- nothing has been allocated
yet at this point, so there is nothing to free.

Fixes this compiler warning:
In function ‘__zfree’,
    inlined from ‘add_drm_pmu’ at util/drm_pmu.c:110:3,
    inlined from ‘read_drm_pmus_cb’ at util/drm_pmu.c:174:10:
../lib/zalloc.c:13:9: warning: ‘drm’ may be used uninitialized [-Wmaybe-uninitialized]
   13 |         free(*ptr);
      |         ^

Assisted-by: LLM
Fixes: 28917cb17f9d ("perf drm_pmu: Add a tool like PMU to expose DRM information")
Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Cc: Ian Rogers <irogers@google.com>
---
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: James Clark <james.clark@linaro.org>
Cc: linux-perf-users@vger.kernel.org
---
 tools/perf/util/drm_pmu.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/perf/util/drm_pmu.c b/tools/perf/util/drm_pmu.c
index b8badae7015c..5e4bdd8e7fa8 100644
--- a/tools/perf/util/drm_pmu.c
+++ b/tools/perf/util/drm_pmu.c
@@ -107,7 +107,6 @@ static struct drm_pmu *add_drm_pmu(struct list_head *pmus, char *line, size_t li
 		type = PERF_PMU_TYPE_DRM_START;
 
 	if (type > PERF_PMU_TYPE_DRM_END) {
-		zfree(&drm);
 		pr_err("Unable to encode DRM PMU type for %s\n", name);
 		return NULL;
 	}
-- 
2.56.0

[PATCH 2/2] perf: do not call list_for_each_entry_safe() on uninitalized list

[Jiri Slaby (SUSE)]

In test__pmu_config_helpers(), when evlist__new() fails, the code
jumped to the err_out label, which calls
parse_events_terms__exit(&terms). At that point
parse_events_terms__init(&terms) has not run yet, so the terms list
head is uninitialized and list_for_each_entry_safe() walks a garbage
pointer.

Add a separate err_out_pmu label after the evlist/terms cleanup and
use it for the early evlist allocation failure, so the uninitialized
terms are never touched.

Fixes this compiler warning:
In function ‘parse_events_terms__exit’,
    inlined from ‘test__pmu_config_helpers’ at tests/pmu.c:296:2:
util/parse-events.c:2849:2: warning: ‘terms.terms.next’ may be used uninitialized [-Wmaybe-uninitialized]
tests/pmu.c: In function ‘test__pmu_config_helpers’:
tests/pmu.c:209:28: note: ‘terms’ declared here

Assisted-by: LLM
Fixes: 8e2ef85c66dc ("perf tests: Test evsel__set_config_if_unset() and config change tracking")
Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Cc: James Clark <james.clark@linaro.org>
---
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Ian Rogers <irogers@google.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: linux-perf-users@vger.kernel.org
---
 tools/perf/tests/pmu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
index 13e8d7fa80af..be984e1385df 100644
--- a/tools/perf/tests/pmu.c
+++ b/tools/perf/tests/pmu.c
@@ -223,7 +223,7 @@ static int test__pmu_config_helpers(struct test_suite *test __maybe_unused,
 	evlist = evlist__new();
 	if (evlist == NULL) {
 		pr_err("Failed allocation");
-		goto err_out;
+		goto err_out_pmu;
 	}
 
 	parse_events_terms__init(&terms);
@@ -295,6 +295,7 @@ static int test__pmu_config_helpers(struct test_suite *test __maybe_unused,
 err_out:
 	parse_events_terms__exit(&terms);
 	evlist__put(evlist);
+err_out_pmu:
 	test_pmu_put(dir, pmu);
 	return ret;
 }
-- 
2.56.0

Re: [PATCH 1/2] perf: do not call zfree() on uninitialized pointer

[sashiko-bot]

> In add_drm_pmu(), the check for type > PERF_PMU_TYPE_DRM_END happens
> before "drm" is allocated by zalloc(). The error path nevertheless
> called zfree(&drm), i.e. free() on an uninitialized pointer, which
> can crash. Remove this bogus zfree() -- nothing has been allocated
> yet at this point, so there is nothing to free.
> 
> Fixes this compiler warning:
> In function ‘__zfree’,
>     inlined from ‘add_drm_pmu’ at util/drm_pmu.c:110:3,
>     inlined from ‘read_drm_pmus_cb’ at util/drm_pmu.c:174:10:
> ../lib/zalloc.c:13:9: warning: ‘drm’ may be used uninitialized [-Wmaybe-uninitialized]
>    13 |         free(*ptr);
>       |         ^
> 
> Assisted-by: LLM
> Fixes: 28917cb17f9d ("perf drm_pmu: Add a tool like PMU to expose DRM information")
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104100802.539266-1-jirislaby@kernel.org?part=1

Re: [PATCH 2/2] perf: do not call list_for_each_entry_safe() on uninitalized list

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf tests: uninitialized err list in test__pmu_config_helpers()
--

commit 779f6d6b7e775ad26417497420edb0cda7bafa6c
Author: Jiri Slaby (SUSE) <jirislaby@kernel.org>

perf: do not call list_for_each_entry_safe() on uninitalized list

This commit adds an err_out_pmu cleanup label in test__pmu_config_helpers()
so that an early evlist__new() allocation failure skips calling
parse_events_terms__exit(&terms) before parse_events_terms__init(&terms) has
run.

> diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
> index 13e8d7fa80af2..be984e1385df2 100644
> --- a/tools/perf/tests/pmu.c
> +++ b/tools/perf/tests/pmu.c
> @@ -223,7 +223,7 @@ static int test__pmu_config_helpers(struct test_suite *test __maybe_unused,
>  	evlist = evlist__new();
>  	if (evlist == NULL) {
>  		pr_err("Failed allocation");
> -		goto err_out;
> +		goto err_out_pmu;
>  	}
>  
>  	parse_events_terms__init(&terms);
> @@ -295,6 +295,7 @@ static int test__pmu_config_helpers(struct test_suite *test __maybe_unused,
>  err_out:
>  	parse_events_terms__exit(&terms);
>  	evlist__put(evlist);
> +err_out_pmu:
>  	test_pmu_put(dir, pmu);
>  	return ret;
>  }

[Severity: Medium]
Should test__pmu_config_helpers() in tools/perf/tests/pmu.c be initializing
and cleaning up err instead of terms?

Looking at test__pmu_config_helpers(), both terms and config_terms appear to
be unused remnants, whereas err is passed to parse_events(),
parse_events_error__print(), and parse_events_error__contains() without
parse_events_error__init(&err) or parse_events_error__exit(&err) ever being
called:

	struct parse_events_terms terms;
	struct parse_events_error err;
	LIST_HEAD(config_terms);
	...
	parse_events_terms__init(&terms);
	ret = parse_events(evlist, event, &err);
	if (ret) {
		pr_debug("failed to parse event '%s', err %d\n", event, ret);
		parse_events_error__print(&err, event);
		if (parse_events_error__contains(&err, "can't access trace events"))
			ret = TEST_SKIP;
		goto err_out;
	}
	...
err_out:
	parse_events_terms__exit(&terms);
	evlist__put(evlist);

When parse_events() fails and records an error via
parse_events_error__handle(), list_add(&entry->list, &err->list) and the
subsequent list_for_each_entry() walks in parse_events_error__print() and
parse_events_error__contains() will dereference the uninitialized err.list
pointers on the stack, and any allocated parse_events_error_entry objects
will be leaked.

Instead of keeping the unused terms variable, should terms and config_terms
be removed and parse_events_terms__init(&terms) /
parse_events_terms__exit(&terms) be replaced with
parse_events_error__init(&err) and parse_events_error__exit(&err)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104100802.539266-1-jirislaby@kernel.org?part=2