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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Size mismatch in synthesized attributes causes downstream parser out-of-bounds reads and data corruption.
- [Medium] Missing implementation of intel-pt.c bug fixes claimed in commit message.
--

commit 9f4d44a4d4fc78e9089af666b1106f4abf4ea03a
Author: Ian Rogers <irogers@google.com>

perf inject: Fix itrace branch stack synthesis

This commit addresses multiple issues in the generation of synthesized
branch stacks from AUX data. It adjusts sample types, fixes size
calculation bugs, prevents event truncation, and introduces safe handling
of read-only maps in perf_event__repipe_attr.

>     1. The synthesized samples were delivered without the
>        PERF_SAMPLE_BRANCH_STACK flag if it was not in the original event's
>        sample_type. Fixed by using sample_type | evsel->synth_sample_type
>        in intel_pt_do_synth_pebs_sample.

[ ... ]

>     6. NULL Pointer Dereference in intel-pt.c: Addressed by updating the
>        condition in intel_pt_do_synth_pebs_sample to fill sample.
>        branch_stack if it warrants synthesis, even if not in the original
>        sample_type.

The commit message explicitly mentions fixes in
intel_pt_do_synth_pebs_sample() within intel-pt.c, but the patch does not
contain any modifications to that file.

Were these changes accidentally omitted from this patch?

> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index 5f06ee1c14a08..2c17b652308f6 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c

[ ... ]

> @@ -231,7 +242,28 @@ static int perf_event__repipe_attr(const struct perf_tool *tool,
>  	if (!inject->output.is_pipe)
>  		return 0;
>  
> -	return perf_event__repipe_synth(tool, event);
> +	if (!inject->itrace_synth_opts.set)
> +		return perf_event__repipe_synth(tool, event);
> +
> +	attr = event->attr.attr;
> +	if (event->header.size < sizeof(event->header) + event->attr.attr.size) {
> +		pr_err("Attribute event size %u is too small\n", event->header.size);
> +		return -EINVAL;
> +	}
> +	n_ids = event->header.size - sizeof(event->header) - event->attr.attr.size;
> +	n_ids /= sizeof(u64);
> +	ids = perf_record_header_attr_id(event);
> +
> +	attr.sample_type &= ~PERF_SAMPLE_AUX;
> +
> +	if (inject->itrace_synth_opts.add_last_branch) {
> +		attr.sample_type |= PERF_SAMPLE_BRANCH_STACK;
> +		if (attr.size < PERF_ATTR_SIZE_VER2)
> +			attr.size = PERF_ATTR_SIZE_VER2;
> +		attr.branch_sample_type |= PERF_SAMPLE_BRANCH_HW_INDEX;
> +	}
> +	return perf_event__synthesize_attr(tool, &attr, (u32)n_ids, ids,
> +					   perf_event__repipe_synth_cb);
>  }

Can this cause size mismatch issues downstream?

By performing a fixed-size struct copy attr = event->attr.attr, if the
incoming event's attribute is smaller (e.g., PERF_ATTR_SIZE_VER0), this will
copy the trailing id array into fields like branch_sample_type.

Then, when passing it to perf_event__synthesize_attr(), it hardcodes the new
event's header.size using sizeof(struct perf_event_attr) but leaves
attr.size as PERF_ATTR_SIZE_VER2.

Could downstream parsers calculating the number of IDs:

n_ids = (event->header.size - sizeof(header) - attr.size) / sizeof(u64)

end up hallucinating extra IDs because header.size accounts for 136 bytes
while attr.size is 80?

If the original attr.size was larger than 136, could this calculation
underflow to a negative value, causing the parser to read out of bounds?

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