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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Missing `PERF_SAMPLE_MAX_SIZE` bounds check in auxtrace injection helpers causes Heap Overflow.
- [High] Failure to strip size field on empty AUX samples breaks downstream parsing.
- [High] Early error return in `perf_event__repipe_sample` leaves a dangling pointer and corrupted shared state.
- [High] Integer underflow and out-of-bounds read in `perf_event__repipe_attr`.
- [Medium] Missing `attr.size` update in file mode branch stack injection.
--

commit 5881c330c2ca4300a88cc8c8e4d578c17a6ebef5
Author: Ian Rogers <irogers@google.com>

perf inject: Fix itrace branch stack synthesis

When using "perf inject --itrace=L" to synthesize branch stacks from
AUX data, several issues caused failures with the generated file.

> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index 0c51cb4250d1e..fe03cab7e9907 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
> @@ -231,7 +242,24 @@ 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;
> +	n_ids = event->header.size - sizeof(event->header) - event->attr.attr.size;
> +	n_ids /= sizeof(u64);
> +	ids = perf_record_header_attr_id(event);

Could event->header.size underflow n_ids here if it is smaller than the
combined size of the header and attribute?

If an event is crafted with a maliciously small size, the direct struct
assignment might trigger an out-of-bounds read, and the underflowed n_ids
could lead to a large out-of-bounds memcpy() in perf_event__synthesize_attr().

[ ... ]
> @@ -375,7 +402,63 @@ static int perf_event__repipe_sample(const struct perf_tool *tool,
>  
>  	build_id__mark_dso_hit(tool, event, sample, evsel, machine);
>  
> -	if (inject->itrace_synth_opts.set && sample->aux_sample.size) {
> +	if (inject->itrace_synth_opts.set &&
> +	    (inject->itrace_synth_opts.last_branch ||
> +	     inject->itrace_synth_opts.add_last_branch)) {
> +		union perf_event *event_copy = (void *)inject->event_copy;
> +		struct branch_stack dummy_bs = { .nr = 0, .hw_idx = 0 };
> +		int err;
> +		size_t sz;
> +		u64 orig_type = evsel->core.attr.sample_type;
> +		u64 orig_branch_type = evsel->core.attr.branch_sample_type;
> +
> +		struct branch_stack *orig_bs = sample->branch_stack;
> +
> +		if (event_copy == NULL) {
> +			inject->event_copy = malloc(PERF_SAMPLE_MAX_SIZE);
> +			if (!inject->event_copy)
> +				return -ENOMEM;
> +
> +			event_copy = (void *)inject->event_copy;
> +		}
> +
> +		if (!sample->branch_stack)
> +			sample->branch_stack = &dummy_bs;
> +
> +		if (inject->itrace_synth_opts.add_last_branch) {
> +			/* Temporarily add in type bits for synthesis. */
> +			evsel->core.attr.sample_type |= PERF_SAMPLE_BRANCH_STACK;
> +			evsel->core.attr.branch_sample_type |= PERF_SAMPLE_BRANCH_HW_INDEX;
> +		}
> +		evsel->core.attr.sample_type &= ~PERF_SAMPLE_AUX;
> +
> +		sz = perf_event__sample_event_size(sample, evsel->core.attr.sample_type,
> +						   evsel->core.attr.read_format,
> +						   evsel->core.attr.branch_sample_type);
> +
> +		if (sz >= PERF_SAMPLE_MAX_SIZE) {
> +			pr_err("Sample size %zu exceeds max size %d\n", sz, PERF_SAMPLE_MAX_SIZE);
> +			return -EFAULT;
> +		}

Does this early return leave a dangling pointer in sample->branch_stack and
corrupt the shared evsel state?

Since the cleanup logic that restores evsel->core.attr.sample_type,
evsel->core.attr.branch_sample_type, and sample->branch_stack is bypassed,
sample->branch_stack may be left pointing to the out-of-scope dummy_bs
variable, which could cause a use-after-free if the sample is processed
further.

[ ... ]
> +		evsel->core.attr.sample_type = orig_type;
> +		evsel->core.attr.branch_sample_type = orig_branch_type;
> +		sample->branch_stack = orig_bs;
> +
> +		if (err) {
> +			pr_err("Failed to synthesize sample\n");
> +			return err;
> +		}
> +		event = event_copy;
> +	} else if (inject->itrace_synth_opts.set && sample->aux_sample.size) {
>  		event = perf_inject__cut_auxtrace_sample(inject, event, sample);
>  		if (IS_ERR(event))
>  			return PTR_ERR(event);

If the original event had PERF_SAMPLE_AUX set but sample->aux_sample.size is 0,
will bypassing perf_inject__cut_auxtrace_sample() leave an 8-byte size field
in the payload?

Because PERF_SAMPLE_AUX is globally stripped from evsel->core.attr.sample_type,
downstream parsers won't expect this 8-byte field, which might misalign
subsequent fields and cause parsing of the generated file to fail.

[ ... ]
> @@ -2440,12 +2526,25 @@ static int __cmd_inject(struct perf_inject *inject)
>  		 * synthesized hardware events, so clear the feature flag.
>  		 */
>  		if (inject->itrace_synth_opts.set) {
> +			struct evsel *evsel;
> +
>  			perf_header__clear_feat(&session->header,
>  						HEADER_AUXTRACE);
> -			if (inject->itrace_synth_opts.last_branch ||
> -			    inject->itrace_synth_opts.add_last_branch)
> +
> +			evlist__for_each_entry(session->evlist, evsel) {
> +				evsel->core.attr.sample_type &= ~PERF_SAMPLE_AUX;
> +			}
> +
> +			if (inject->itrace_synth_opts.add_last_branch) {
>  				perf_header__set_feat(&session->header,
>  						      HEADER_BRANCH_STACK);
> +
> +				evlist__for_each_entry(session->evlist, evsel) {
> +					evsel->core.attr.sample_type |= PERF_SAMPLE_BRANCH_STACK;
> +					evsel->core.attr.branch_sample_type |=
> +						PERF_SAMPLE_BRANCH_HW_INDEX;
> +				}
> +			}
>  		}

If the input file was recorded with an older, smaller attribute size, should
evsel->core.attr.size be expanded to PERF_ATTR_SIZE_VER2 here as well?

Unlike the pipe mode logic in perf_event__repipe_attr(), attr.size isn't
expanded here. The newly set branch_sample_type field might reside beyond
attr.size and be silently omitted when headers are written to the output file.

> diff --git a/tools/perf/util/arm-spe.c b/tools/perf/util/arm-spe.c
> index c4ed9f10e731f..7504b2cb5edd8 100644
> --- a/tools/perf/util/arm-spe.c
> +++ b/tools/perf/util/arm-spe.c
> @@ -482,13 +482,23 @@ static void arm_spe__prep_branch_stack(struct arm_spe_queue *speq)
>  	bstack->hw_idx = -1ULL;
>  }
>  
> -static int arm_spe__inject_event(union perf_event *event, struct perf_sample *sample, u64 type)
> +static int arm_spe__inject_event(struct arm_spe *spe, union perf_event *event,
> +				 struct perf_sample *sample, u64 type)
>  {
> +	struct evsel *evsel = NULL;
> +	u64 branch_sample_type = 0;
> +
> +	if (spe->session && spe->session->evlist)
> +		evsel = evlist__id2evsel(spe->session->evlist, sample->id);
> +
> +	if (evsel)
> +		branch_sample_type = evsel->core.attr.branch_sample_type;
> +
>  	event->header.type = PERF_RECORD_SAMPLE;
>  	event->header.size = perf_event__sample_event_size(sample, type, /*read_format=*/0,
> -							   /*branch_sample_type=*/0);
> +							   branch_sample_type);

Could the computed event->header.size exceed PERF_SAMPLE_MAX_SIZE here?

The sz >= PERF_SAMPLE_MAX_SIZE bounds check was added to
perf_event__repipe_sample() and perf_event__convert_sample_callchain(), but
appears to be missing from the hardware tracing helpers (arm_spe__inject_event,
cs_etm__inject_event, intel_pt_inject_event).

If the computed size exceeds PERF_SAMPLE_MAX_SIZE, it might truncate the value
when assigned to the u16 header.size, while perf_event__synthesize_sample()
writes the full size and overflows the buffer.

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