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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Incorrect size bounds check for config1 and config2 overwrites the trailing id[] array in older (ABI0/VER0) events.
- [High] Failure to mask unsupported sample_type bits leads to parsing failures on newer perf.data formats.

Pre-existing issues:
- [High] Cross-endian `perf.data` files are corrupted during ASLR injection due to mixed endianness in the synthesized `new_event` payload.
--

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

perf aslr: Strip sample registers

This patch refactors the ASLR tool to strip out only the register dump
payload by masking out the relevant perf_event_attr fields when the
delegated tool is handling the data. It maintains the original
perf_event_attr values via an aslr_evsel_priv struct looked up in the
evsel_orig_attrs hashmap.

> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index 00a54d1c7e41f..c852ade3c4e37 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
> @@ -254,6 +254,12 @@ static int perf_event__repipe_attr(const struct perf_tool *tool,
>  				return -ENOMEM;
>  			memcpy(stripped_event, event, event->header.size);
>  			stripped_event->attr.attr.sample_type &= ASLR_SUPPORTED_SAMPLE_TYPE;
> +			if (stripped_event->attr.attr.size >=
> +			    (offsetof(struct perf_event_attr, sample_regs_user) + sizeof(u64)))
> +				stripped_event->attr.attr.sample_regs_user = 0;
> +			if (stripped_event->attr.attr.size >=
> +			    (offsetof(struct perf_event_attr, sample_regs_intr) + sizeof(u64)))
> +				stripped_event->attr.attr.sample_regs_intr = 0;
>  
>  			if (stripped_event->attr.attr.type == PERF_TYPE_BREAKPOINT &&
>  			    event->header.size >= (offsetof(struct perf_record_header_attr,

[Severity: High]
Does this bounds check for config1 and config2 overwrite the trailing id[] array
in older (ABI0/VER0) events?

In perf_event__repipe_attr(), when clearing config1 and config2 for
kprobe/uprobe events, the size check validates against event->header.size rather
than stripped_event->attr.attr.size.

In older perf records, config2 is not part of the perf_event_attr payload, and
the id[] array begins exactly at offset 64. However, event->header.size includes
the size of the appended id[] array.

If an event has a 64-byte attr.size and an appended id[] array, the validation
will incorrectly pass and the code will write 0 to config2 (offset 72),
overwriting id[0]. Can this corrupt the unique event ID and break event
demuxing for downstream readers?

[ ... ]
> diff --git a/tools/perf/util/aslr.c b/tools/perf/util/aslr.c
> index 7afa5a0dac2f3..bac41bff1b2a3 100644
> --- a/tools/perf/util/aslr.c
> +++ b/tools/perf/util/aslr.c
[ ... ]
> @@ -668,6 +693,10 @@ static int aslr_tool__process_sample(const struct perf_tool *tool,
>  	u64 addr;
>  	size_t i;
>  	size_t j;
> +	struct aslr_evsel_priv *priv = NULL;
> +	u64 orig_sample_type;
> +	u64 orig_regs_user;
> +	u64 orig_regs_intr;
>  	bool orig_needs_swap;
>  
>  	del_tool = container_of(tool, struct delegate_tool, tool);
> @@ -678,7 +707,23 @@ static int aslr_tool__process_sample(const struct perf_tool *tool,
>  		return delegate->sample(delegate, event, sample, machine);
>  
>  	ret = -EFAULT;
> -	sample_type = evsel->core.attr.sample_type;
> +
> +	if (hashmap__find(&aslr->evsel_orig_attrs, evsel, &priv)) {
> +		orig_sample_type = priv->orig_sample_type;
> +		orig_regs_user = priv->orig_sample_regs_user;
> +		orig_regs_intr = priv->orig_sample_regs_intr;
> +	} else {
> +		orig_sample_type = evsel->core.attr.sample_type;
> +		orig_regs_user = evsel->core.attr.sample_regs_user;
> +		orig_regs_intr = evsel->core.attr.sample_regs_intr;
> +	}
> +
> +	orig_sample_size = evsel->sample_size;
> +
> +	sample_type = orig_sample_type;
> +	sample_type &= ~PERF_SAMPLE_REGS_USER;
> +	sample_type &= ~PERF_SAMPLE_REGS_INTR;
> +

[Severity: High]
Does failing to mask out unsupported sample_type bits lead to parsing failures
on newer perf.data formats?

In aslr_tool__process_sample(), the sample_type used to synthesize the new
event is derived from orig_sample_type by masking out only
PERF_SAMPLE_REGS_USER and PERF_SAMPLE_REGS_INTR.

Because this fails to mask out unsupported bits (unlike the pre-patch logic
which used &= ASLR_SUPPORTED_SAMPLE_TYPE), and the extraction loop (COPY_U64)
lacks handlers for unknown/unsupported bits, they are dropped from the
new_event payload.

However, since evsel->core.attr.sample_type = sample_type advertises these
bits as present, the subsequent call to evsel__parse_sample() will expect to
find them in the array. Can this mismatch cause the parser to read misaligned or
out-of-bounds data when processing traces from newer perf tools?

>  	max_i = (event->header.size - sizeof(struct perf_event_header)) / sizeof(__u64);
>  	max_j = (PERF_SAMPLE_MAX_SIZE - sizeof(struct perf_event_header)) / sizeof(__u64);
>  	new_event = (union perf_event *)aslr->event_copy;
> @@ -727,25 +772,25 @@ static int aslr_tool__process_sample(const struct perf_tool *tool,
>  		i++; \
>  	} while (0)
>  
> -	if (sample_type & PERF_SAMPLE_IDENTIFIER)
> +	if (orig_sample_type & PERF_SAMPLE_IDENTIFIER)
>  		COPY_U64(); /* id */

[Severity: High]
This isn't a bug introduced by this patch, but can cross-endian perf.data files
be corrupted during ASLR injection due to mixed endianness in the synthesized
new_event payload?

In aslr_tool__process_sample(), new_event is synthesized using a combination
of COPY_U64() and REMAP_U64(). COPY_U64() copies directly from the original
event->sample.array, which is in the original file's endianness (perf core
does not swap sample payloads in-place).

However, REMAP_U64() computes remapped addresses and writes them in host
endianness. For cross-endian perf.data files, this results in a corrupted,
mixed-endian payload.

Furthermore, evsel->needs_swap = false forces the subsequent parser and the
delegated tool to treat the entire payload as host-endian, which permanently
corrupts fields like TID, TIME, and IDENTIFIER in the injected output file.

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