Re: [PATCH v2 2/2] perf inject: Fix itrace branch stack synthesis

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Modifying `event->attr.attr` in-place in `perf_event__repipe_attr` causes SIGSEGV and completely breaks sample parsing.
- [High] The size check in `perf_event__repipe_attr` incorrectly rejects valid older files and causes out-of-bounds writes.
- [High] Flawed `sample_size` recalculation in `perf_event__convert_sample_callchain` causes heap memory leaks and malformed output.
--

commit 64e2d17135bd2355121cf62d57be79321e910c46
Author: Ian Rogers <irogers@google.com>

perf inject: Fix itrace branch stack synthesis

This commit addresses several issues with synthesizing branch stacks from
AUX data when using "perf inject --itrace=L". It modifies sample types early
to handle missing branch stack flags and fixes sample synthesis logic.

> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index 0c51cb4250d1..13fb1f05ceb2 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
> @@ -223,6 +223,19 @@ static int perf_event__repipe_attr(const struct perf_tool *tool,
>  						  tool);
>  	int ret;
>  
> +	if (event->header.size < sizeof(struct perf_record_header_attr)) {

Does this size check incorrectly reject valid older files?

Older perf.data files have a smaller attr.size (e.g., 64 bytes). Valid
events from older kernels may have a total size of 72 bytes, triggering
this check and causing perf inject to wrongly reject them.

If an older event has enough trailing IDs to pass the size check, the struct
ends at offset 64 and the id[] array begins at offset 72. Modifying
event->attr.attr.branch_sample_type writes to offset 80, which overlaps with
the id[] array and can corrupt the event ID data.

> +		pr_err("Attribute event size %u is too small\n", event->header.size);
> +		return -EINVAL;
> +	}
> +
> +	if (inject->itrace_synth_opts.set)
> +		event->attr.attr.sample_type &= ~PERF_SAMPLE_AUX;
> +
> +	if (inject->itrace_synth_opts.add_last_branch) {
> +		event->attr.attr.sample_type |= PERF_SAMPLE_BRANCH_STACK;
> +		event->attr.attr.branch_sample_type |= PERF_SAMPLE_BRANCH_HW_INDEX;
> +	}

Could modifying event->attr.attr in-place here cause a segmentation fault?

When reading from a standard perf.data file, perf_session__process_events()
mmaps the file into memory using PROT_READ. The event pointer passed to
perf_event__repipe_attr() points directly into this read-only buffer, so
writing to event->attr.attr.sample_type could immediately trigger a SIGSEGV.

Even when reading from a pipe where the buffer might be writable, applying
these modifications before calling perf_event__process_attr() alters the
flags received by the internal evsel. The sample records arriving next
in the stream still physically contain the original layout, which causes
evsel__parse_sample() to misinterpret payload offsets and abort the session.

>  	ret = perf_event__process_attr(tool, event, pevlist);

[ ... ]

> @@ -463,7 +531,7 @@ static int perf_event__convert_sample_callchain(const struct perf_tool *tool,
>  	/* remove sample_type {STACK,REGS}_USER for synthesize */
>  	sample_type &= ~(PERF_SAMPLE_STACK_USER | PERF_SAMPLE_REGS_USER);
>  
> -	ret = perf_event__synthesize_sample(event_copy, evsel->core.attr.sample_type,
> +	ret = perf_event__synthesize_sample(event_copy, sample_type,

This wasn't introduced by this patch, but does the sample_size recalculation
just above this cause heap memory leaks and malformed output?

When adjusting the size of the synthesized sample, the function adds the
size of the newly injected callchain but fails to subtract the size of the
original callchain present in the event. Additionally, when removing
STACK_USER, it only subtracts the payload size, failing to remove the 16 bytes
of length prefix and suffix metadata.

This heavily inflates the calculated sample_size. While
perf_event__synthesize_sample() correctly writes the smaller, actual data to
the heap buffer, perf_event__repipe_synth() writes the inflated sample_size
to the output pipe, leaking uninitialized heap memory into the file and
resulting in an oversized, malformed record.

>  					    evsel->core.attr.read_format,
>  					    evsel->core.attr.branch_sample_type, sample);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260429181136.2712655-1-irogers@google.com?part=2