Re: [PATCH v18 4/5] perf aslr: Strip sample registers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] aslr_tool__strip_attr_event corrupts the trailing ID array for events with smaller ABIs by improperly verifying bounds via event->header.size instead of event->attr.attr.size.
--

commit e1654455a52059659d22e581a3b84d7f361f3820
Author: Ian Rogers <irogers@google.com>
Subject: perf aslr: Strip sample registers

Extend the ASLR tool stripping helpers to drop register dump payloads
by masking out the relevant perf_event_attr fields (sample_regs_user,
sample_regs_intr) when the delegated tool is handling the data.

> diff --git a/tools/perf/util/aslr.c b/tools/perf/util/aslr.c
> index 03944677ddfcc..912efd111bb3f 100644
> --- a/tools/perf/util/aslr.c
> +++ b/tools/perf/util/aslr.c
[ ... ]
> @@ -1108,43 +1188,30 @@ static int aslr_tool__process_auxtrace_error(const struct perf_tool *tool __mayb
>  	return 0;
>  }
>  
> -
> -void aslr_tool__strip_attr_event(union perf_event *event, struct evlist **pevlist)
> +void aslr_tool__strip_attr_event(union perf_event *event, struct evlist *evlist)
>  {
> -	struct evsel *evsel;
> -	bool needs_swap = false;
> -
> -	if (pevlist && *pevlist) {
> -		evsel = evlist__last(*pevlist);
> -		if (evsel)
> -			needs_swap = evsel->needs_swap;
> -	}
> +	if (!evlist)
> +		return;
>  
>  	if (event->header.size >= (offsetof(struct perf_record_header_attr,
>  					    attr.sample_type) + sizeof(u64))) {
> -		u64 st = event->attr.attr.sample_type;
> -
> -		if (needs_swap)
> -			st = bswap_64(st);
> -
> -		st &= ASLR_SUPPORTED_SAMPLE_TYPE;
> -
> -		if (needs_swap)
> -			st = bswap_64(st);
> -
> -		event->attr.attr.sample_type = st;
> +		event->attr.attr.sample_type &= ASLR_SUPPORTED_SAMPLE_TYPE;
> +
> +		if (event->header.size >=
> +		    (offsetof(struct perf_record_header_attr, attr.sample_regs_user) + sizeof(u64)))
> +			event->attr.attr.sample_regs_user = 0;
> +		if (event->header.size >=
> +		    (offsetof(struct perf_record_header_attr, attr.sample_regs_intr) + sizeof(u64)))
> +			event->attr.attr.sample_regs_intr = 0;

[Severity: High]
Can this corrupt the trailing ID array for events with smaller ABIs?

Since event->header.size represents the total size of the record, it includes
both the attr payload and the dynamically appended array of u64 IDs.

If an older perf.data file uses an ABI where attr.size is smaller than the
offset of sample_regs_user (such as ABI 0) but contains multiple appended IDs,
event->header.size will still easily exceed this offset check.

In this scenario, writing 0 to event->attr.attr.sample_regs_user would write
past the end of the actual attr struct and silently overwrite the trailing
ID array, causing downstream tools to fail to match events to their IDs.

Should this bounds verification check against event->attr.attr.size (falling
back to PERF_ATTR_SIZE_VER0 if zero) instead of event->header.size?

>  	}
>  
>  	if (event->header.size >= (offsetof(struct perf_record_header_attr,
>  					    attr.type) + sizeof(u32))) {
>  		u32 type = event->attr.attr.type;
>  
> -		if (needs_swap)
> -			type = bswap_32(type);
> -
>  		if (type == PERF_TYPE_BREAKPOINT &&

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