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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect double byte-swapping of already natively-swapped 64-bit array fields corrupts cross-endian parsing and address remapping.
- [High] The entire sample event is dropped if it includes branch stack counters, silently breaking profiles collected with LBR counters.
--

commit 99425ccaa2ec1a33c2b9f564c32b12db661696fe
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 2c5fafbe5d846..8980599b0158f 100644
> --- a/tools/perf/util/aslr.c
> +++ b/tools/perf/util/aslr.c

[ ... ]

> @@ -599,13 +654,428 @@ 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); \

[Severity: High]
Does this code incorrectly double byte-swap the remapped addresses?

The perf core framework, perf_event__all64_swap(), already byte-swaps the
entire PERF_RECORD_SAMPLE array payload to host endianness before passing
it to the tool callbacks.

By calling bswap_64() again in aslr_tool__process_sample when orig_needs_swap
is true, won't this mix host and file endianness in the out_array, corrupting
the output perf.data layout and breaking cross-endian parsing downstream?

> +		out_array[j++] = remapped; \
> +		i++; \
> +	} while (0)

[ ... ]

> +	if (sample_type & PERF_SAMPLE_CALLCHAIN) {
> +		u64 nr;
> +
> +		if (CHECK_BOUNDS(1, 1)) {
> +			ret = -EFAULT;
> +			goto out_put;
> +		}
> +		nr = in_array[i];
> +		if (orig_needs_swap)
> +			nr = bswap_64(nr);

[Severity: High]
Can this double-swapping of the nr counter cause parsing loops to fail?

Since in_array[i] is already in host endianness, wouldn't swapping it back
into file endianness produce a massive integer value for nr?

> +		out_array[j++] = in_array[i++];

[ ... ]

> +	if (sample_type & PERF_SAMPLE_BRANCH_STACK) {
> +		u64 nr;
> +
> +		if (CHECK_BOUNDS(1, 1)) {
> +			ret = -EFAULT;
> +			goto out_put;
> +		}
> +		nr = in_array[i];
> +		if (orig_needs_swap)
> +			nr = bswap_64(nr);

[Severity: High]
Similarly, could swapping nr here cause the branch stack iteration bounds
checks to unconditionally return -EFAULT and drop the event?

> +		out_array[j++] = in_array[i++];

[ ... ]

> +		if (evsel->core.attr.branch_sample_type & PERF_SAMPLE_BRANCH_COUNTERS) {
> +			if (nr > max_i - i || nr > max_j - j) {
> +				ret = -EFAULT;
> +				goto out_put;
> +			}
> +			memcpy(&out_array[j], &in_array[i], nr * sizeof(u64));
> +			i += nr;
> +			j += nr;
> +			/* TODO: confirm branch counters don't leak ASLR information. */
> +			pr_debug("Dropping sample branch counters as possible ASLR leak\n");
> +			ret = 0;
> +			goto out_put;

[Severity: High]
Does this logic silently discard the entire sample event when branch
counters are present?

By executing ret = 0 and jumping to out_put in aslr_tool__process_sample,
the delegate->sample invocation is bypassed.

Won't this break profiles collected with LBR counters (e.g. perf record -b)
by completely dropping all branch stack counter samples instead of just
omitting the specific fields?

> +		}
> +	}

[ ... ]

> +	if (sample_type & PERF_SAMPLE_STACK_USER) {
> +		u64 size;
>  
> -	return delegate->sample(delegate, event, sample, machine);
> +		if (CHECK_BOUNDS(1, 1)) {
> +			ret = -EFAULT;
> +			goto out_put;
> +		}
> +		size = in_array[i];
> +		if (orig_needs_swap)
> +			size = bswap_64(size);

[Severity: High]
Could double-swapping the PERF_SAMPLE_STACK_USER size corrupt the payload
boundaries?

Like the nr values above, swapping the already host-endian size might trigger
a bounds failure in the subsequent u64_words bounds check.

> +		out_array[j++] = in_array[i++];
> +		if (size > 0) {

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