Re: [PATCH v5 02/25] perf sample: Make sure perf_sample__init/exit are used

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

New issues:
- [Medium] The return value of `perf_evsel__parse_id_sample` is silently ignored for non-`PERF_RECORD_SAMPLE` events.

Pre-existing issues:
- [High] In the Python bindings, if `evlist__event2evsel` fails during event parsing, an uninitialized `struct perf_sample` is passed to `perf_sample__exit()`, resulting in a free of garbage pointers.
- [High] Missing check for `deferred_callchain` allows a malformed event to crash the tool by dereferencing a NULL callchain.
- [High] Missing bounds check on `event->callchain_deferred.nr` leads to massive out-of-bounds read and potential heap overflow.
--

--- Patch [2]: [PATCH v5 02/25] perf sample: Make sure perf_sample__init/exit are used ---
commit 24e922c277122af1d6e4cb75082ba12f77262a00
Author: Ian Rogers <[email protected]>

perf sample: Make sure perf_sample__init/exit are used
    
The deferred stack trace code wasn't using perf_sample__init/exit. Add
the deferred stack trace clean up to perf_sample__exit which requires
proper NULL initialization in perf_sample__init. Make the
perf_sample__exit robust to being called more than once by using
zfree.

> diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> index f879b84f8ff99..f031cbbeeba8b 100644
> --- a/tools/perf/util/callchain.c
> +++ b/tools/perf/util/callchain.c
> @@ -1919,6 +1922,7 @@ int sample__merge_deferred_callchain(struct perf_sample *sample_orig,
>  	memcpy(&callchain->ips[nr_orig], sample_callchain->callchain->ips,
>  	       nr_deferred * sizeof(u64));

[Severity: High]
This is a pre-existing issue, but does this lack a bounds check on
event->callchain_deferred.nr before copying from the event buffer?

In tools/perf/util/evsel.c:evsel__parse_sample(), data->callchain->nr is
checked against UINT64_MAX / sizeof(u64) rather than being bounded by the
actual event->header.size.

When sample__merge_deferred_callchain() is called, nr_deferred * sizeof(u64)
can integer-overflow on 32-bit systems (causing a small heap allocation and a
subsequent buffer overwrite) or cause a massive out-of-bounds read on 64-bit
systems.

Could we validate the nr field against the actual event size?

> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index c702741a91738..f46e1d40bad73 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -1632,8 +1632,11 @@ int evlist__parse_sample(struct evlist *evlist, union perf_event *event, struct
>  	struct evsel *evsel = evlist__event2evsel(evlist, event);
>  	int ret;
>  
> -	if (!evsel)
> +	if (!evsel) {
> +		/* Ensure the sample is okay for perf_sample__exit. */
> +		perf_sample__init(sample, /*all=*/false);
>  		return -EFAULT;
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but does the Python binding
code have a similar issue where an uninitialized sample is cleaned up on
error?

In tools/perf/util/python.c:pyrf_event__new(), the pyrf_event is allocated
via PyObject_New without zeroing the memory, meaning the sample member is
uninitialized. 

If evlist__event2evsel() fails during event parsing in
pyrf_evlist__read_on_cpu(), Py_DECREF triggers the destructor
pyrf_sample_event__delete(), which calls perf_sample__exit(&pevent->sample).
This passes uninitialized garbage pointers to zfree(), which can corrupt
memory.

> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 5a294595a6776..4d3615d7b8859 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
[ ... ]
> @@ -3236,25 +3238,26 @@ int evsel__parse_sample(struct evsel *evsel, union perf_event *event,
[ ... ]
>  	if (event->header.type != PERF_RECORD_SAMPLE) {
> -		if (!evsel->core.attr.sample_id_all)
> -			return 0;
> -		return perf_evsel__parse_id_sample(evsel, event, data);
> +		if (evsel->core.attr.sample_id_all)
> +			perf_evsel__parse_id_sample(evsel, event, data);
> +		return 0;
>  	}

[Severity: Medium]
Is it intentional that the return value from perf_evsel__parse_id_sample()
is now silently ignored for non-PERF_RECORD_SAMPLE events?

If perf_evsel__parse_id_sample() fails on a malformed or truncated event,
returning 0 instead of propagating the error or taking the out_efault path
might leave perf_sample fields uninitialized or zeroed, potentially leading
to incorrect downstream analysis.

> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 09de5288f9e15..6fb481fda4926 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -1367,14 +1367,18 @@ static int evlist__deliver_deferred_callchain(struct evlist *evlist,
[ ... ]
> -		if (sample->tid != orig_sample.tid)
> +		if (sample->tid != orig_sample.tid) {
> +			perf_sample__exit(&orig_sample);
>  			continue;
> +		}
>  
>  		if (event->callchain_deferred.cookie == orig_sample.deferred_cookie)
>  			sample__merge_deferred_callchain(&orig_sample, sample);

[Severity: High]
This is a pre-existing issue, but could this comparison succeed improperly
and cause a crash?

Since orig_sample is zero-initialized by perf_sample__init(), if it has no
callchain, its cookie is 0 and its callchain pointer is NULL. 

If the incoming event also has a cookie of 0, this equality check passes and
we call sample__merge_deferred_callchain(), which then dereferences
sample_orig->callchain->nr without a NULL check.

Should this verify that orig_sample.deferred_callchain is actually true?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2