Re: [PATCH v17 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 stripping of breakpoint and kprobe/uprobe fields is bypassed, and sample_type is corrupted, when processing cross-endian traces due to incorrect bswap logic.
--

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

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.

struct aslr_evsel_priv maintains the original perf_event_attr values
and is looked up via the evsel_orig_attrs hashmap so that sample sizes
can be properly parsed even when bits are stripped from the pipeline.

> diff --git a/tools/perf/util/aslr.c b/tools/perf/util/aslr.c
> index 8980599b0158f..bf6bb27153573 100644
> --- a/tools/perf/util/aslr.c
> +++ b/tools/perf/util/aslr.c
[ ... ]
> @@ -1120,43 +1200,44 @@ 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 (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) {
> +			u64 st = bswap_64(event->attr.attr.sample_type);
>  
> -		if (needs_swap)
> -			st = bswap_64(st);
> +			st &= ASLR_SUPPORTED_SAMPLE_TYPE;
> +			event->attr.attr.sample_type = bswap_64(st);

[Severity: High]
Does this code inadvertently corrupt the sample_type bitmask?

Since perf_session__process_user_event() calls event_swap(), which delegates
to perf_event__hdr_attr_swap(), the event->attr.attr structure is already
byte-swapped in-place to native endianness before the tool->attr() callback
is invoked.

Swapping it again here when needs_swap is true would corrupt the value before
applying the ASLR_SUPPORTED_SAMPLE_TYPE mask.

> +		} else {
> +			event->attr.attr.sample_type &= ASLR_SUPPORTED_SAMPLE_TYPE;
> +		}
>  
> -		event->attr.attr.sample_type = st;
> +		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;
>  	}
>  
>  	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);
> +		u32 type = needs_swap ? bswap_32(event->attr.attr.type) : event->attr.attr.type;

[Severity: High]
Is it possible this logic bypasses ASLR scrubbing for cross-endian traces?

Because event->attr.attr.type was already swapped to native endianness prior to
this tool callback, calling bswap_32() here would cause the type ==
PERF_TYPE_BREAKPOINT check below to evaluate to false (e.g., native 5
becomes 0x05000000).

This would bypass the scrubbing of bp_addr, config1, and config2, potentially
leaking kernel ASLR addresses.

>  
>  		if (type == PERF_TYPE_BREAKPOINT &&
>  		    event->header.size >= (offsetof(struct perf_record_header_attr,
> -						    attr.bp_addr) + sizeof(u64))) {
> +							attr.bp_addr) + sizeof(u64))) {
>  			event->attr.attr.bp_addr = 0;
>  		} else if (type >= PERF_TYPE_MAX) {
>  			struct perf_pmu *pmu;

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