Re: [PATCH v5 2/4] perf tools top: Add --weight option

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Buffer overflow or NULL pointer dereference in for_each_drm_fdinfo_in_dir() due to unchecked reallocarray() failure.
- [High] Use-After-Free and memory leak in add_event() when strndup() fails.
--

commit f1674c1fafe728985095507f0cfd95caa7a7179c
Author: Andi Kleen <ak@linux.intel.com>

perf tools top: Add --weight option
    
Add a -W/--weight option to perf top to collect weights too. Useful with
follow on patches.

> diff --git a/tools/perf/Documentation/perf-top.txt b/tools/perf/Documentation/perf-top.txt
> index af3e4230c72f4..2da2a16bbf260 100644
> --- a/tools/perf/Documentation/perf-top.txt
> +++ b/tools/perf/Documentation/perf-top.txt
> @@ -25,6 +25,12 @@ OPTIONS
>  --count=<count>::
>  	Event period to sample.
>  
> +-W::
> +--weight::
> +Enable weighted sampling. One or more additional weights can be recorded per sample

[ ... ]

While reviewing this, I noticed some pre-existing issues in
tools/perf/util/drm_pmu.c:

tools/perf/util/drm_pmu.c:for_each_drm_fdinfo_in_dir() {
    ...
		if (minors->minors_num == minors->minors_len) {
			unsigned int *tmp = reallocarray(minors->minors, minors->minors_len + 4,
							 sizeof(unsigned int));

			if (tmp) {
				minors->minors = tmp;
				minors->minors_len += 4;
			}
		}
		minors->minors[minors->minors_num++] = minor;
    ...
}

[Severity: High]
This is a pre-existing issue, but does this code write out of bounds or
dereference a NULL pointer?

If reallocarray() fails, tmp is NULL, so minors->minors and minors->minors_len
are not updated.

However, the function proceeds to unconditionally assign minor to
minors->minors[minors->minors_num++].

If the array was initially NULL, this dereferences a NULL pointer. If it was
already allocated, this writes past the end of the un-resized buffer.


tools/perf/util/drm_pmu.c:add_event() {
    ...
	tmp = reallocarray(*events, *num_events + 1, sizeof(struct drm_pmu_event));
	if (!tmp)
		return -ENOMEM;
	tmp[*num_events].unit = unit;
	tmp[*num_events].desc = desc;
	tmp[*num_events].name = strndup(line, colon - line);
	if (!tmp[*num_events].name)
		return -ENOMEM;
	(*num_events)++;
	*events = tmp;
	return 0;
}

[Severity: High]
This is a pre-existing issue, but does this code leave a dangling pointer and
leak memory when strndup() fails?

If reallocarray() succeeds and moves the memory block, the old memory pointed
to by *events is freed.

If strndup() then fails, the function returns -ENOMEM without updating
*events to point to tmp.

This leaks the newly allocated tmp block and leaves *events as a dangling
pointer to the old, freed memory block.

Since the caller read_drm_pmus_cb() ignores the return value of add_event()
and continues parsing, a subsequent match could pass this dangling *events
pointer back into reallocarray(), triggering a use-after-free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830193459.2840519-1-ak@linux.intel.com?part=2