Re: [PATCH v18 3/5] perf inject/aslr: Implement sample address remapping

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incorrect byte-swapping logic corrupts cross-endian ASLR injection output and map lookups.
--

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

perf inject/aslr: Implement sample address remapping

Add the sample address remapping logic to the ASLR tool. This patch
implements aslr_tool__process_sample, which parses sample events,
remaps IPs, ADDRs, callchains, and branch stacks using the mappings
collected from metadata events.

> diff --git a/tools/perf/util/aslr.c b/tools/perf/util/aslr.c
> index e45f68c604937..03944677ddfcc 100644
> --- a/tools/perf/util/aslr.c
> +++ b/tools/perf/util/aslr.c

[ ... ]

> @@ -602,13 +657,413 @@ static int aslr_tool__process_sample(const struct perf_tool *tool,

[ ... ]

> +#define REMAP_U64(addr_field) \
> +	do { \
> +		u64 remapped; \
> +		if (CHECK_BOUNDS(1, 1)) { \
> +			ret = -EFAULT; \
> +			goto out_put; \
> +		} \
> +		remapped = aslr_tool__remap_address(aslr, thread, cpumode, addr_field); \
> +		if (orig_needs_swap) \
> +			remapped = bswap_64(remapped); \
> +		out_array[j++] = remapped; \
> +		i++; \
> +	} while (0)

[Severity: High]
Since perf_session__process_event() applies perf_event__all64_swap() to
PERF_RECORD_SAMPLE payloads, converting all 64-bit words in
event->sample.array to native endianness before the tool processes them,
will applying bswap_64() here write a corrupted foreign-endian address into
the output buffer?

The output perf.data file is emitted with a native header, so downstream
readers wouldn't know to swap these values back.

[ ... ]

> +		for (u64 cntr = 0; cntr < nr; cntr++) {
> +			if (CHECK_BOUNDS(1, 1)) {
> +				ret = -EFAULT;
> +				goto out_put;
> +			}
> +			addr = in_array[i++];
> +			if (addr >= PERF_CONTEXT_MAX) {
> +				out_array[j++] = orig_needs_swap ? bswap_64(addr) : addr;

[Severity: High]
Could this logic result in callchain context markers being incorrectly swapped
to foreign endianness before they are written to the output event buffer?

[ ... ]

> +	if (sample_type & PERF_SAMPLE_BRANCH_STACK) {
> +		u64 nr;
> +
> +		if (CHECK_BOUNDS(1, 1)) {
> +			ret = -EFAULT;
> +			goto out_put;
> +		}
> +		nr = in_array[i];
> +		COPY_U64();
> +
> +		if (evsel->core.attr.branch_sample_type & PERF_SAMPLE_BRANCH_HW_INDEX)
> +			COPY_U64(); /* hw_idx */
> +
> +		if (nr > (ULLONG_MAX / 3)) {
> +			ret = -EFAULT;
> +			goto out_put;
> +		}
> +		if (nr * 3 > max_i - i || nr * 3 > max_j - j) {
> +			ret = -EFAULT;
> +			goto out_put;
> +		}
> +		for (u64 cntr = 0; cntr < nr; cntr++) {
> +			u64 from = in_array[i++];
> +			u64 to = in_array[i++];
> +
> +			if (orig_needs_swap) {
> +				from = bswap_64(from);
> +				to = bswap_64(to);
> +			}
> +
> +			from = aslr_tool__remap_address(aslr, thread, sample->cpumode, from);
> +			to = aslr_tool__remap_address(aslr, thread, sample->cpumode, to);

[Severity: High]
If the from and to addresses are swapped to foreign endianness before calling
aslr_tool__remap_address(), wouldn't this pass invalid addresses to the thread
map lookup, causing it to silently fail and bypass ASLR remapping for branch
stacks?

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