Re: [PATCH v16 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] The tool modifies the shared `evsel->needs_swap` flag without locking and bypasses necessary 32-bit field unswapping, corrupting cross-endian sample parsing.
--

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

perf inject/aslr: Implement sample address remapping

This commit adds the sample address remapping logic to the ASLR tool.
It parses sample events, remaps addresses and IPs, and filters out 
potentially leaking raw samples.

> diff --git a/tools/perf/util/aslr.c b/tools/perf/util/aslr.c
> index 084158014dc73..7afa5a0dac2f3 100644
> --- a/tools/perf/util/aslr.c
> +++ b/tools/perf/util/aslr.c
[ ... ]
> @@ -597,13 +651,405 @@ static int aslr_tool__process_sample(const struct perf_tool *tool,
[ ... ]
> +#define COPY_U64() \
> +	do { \
> +		if (CHECK_BOUNDS(1, 1)) { \
> +			ret = -EFAULT; \
> +			goto out_put; \
> +		} \
> +		out_array[j++] = in_array[i++]; \
> +	} while (0)
[ ... ]
> +	new_event->sample.header.size = sizeof(struct perf_event_header) + j * sizeof(u64);
> +
> +	orig_needs_swap = evsel->needs_swap;
>  
> -	return delegate->sample(delegate, event, sample, machine);
> +	evsel->needs_swap = false;
> +	perf_sample__init(&new_sample, /*all=*/ true);
> +	ret = evsel__parse_sample(evsel, new_event, &new_sample);
> +	evsel->needs_swap = orig_needs_swap;

[Severity: High]
Does temporarily setting evsel->needs_swap to false here break
cross-endian parsing and introduce a data race?

Since new_event is built using COPY_U64(), it directly copies the 64-bit
swapped words from the original event. If we force needs_swap to false
when parsing new_event, won't that bypass the necessary unswapping of
packed 32-bit fields like pid and tid, resulting in corrupted data for
cross-endian files?

Also, mutating the shared evsel->needs_swap flag without locking could
cause data races with other threads processing samples. Since this patch
introduces __evsel__parse_sample() specifically to take a needs_swap
parameter, should we be using that helper here instead of mutating the
evsel? And should we be passing the original orig_needs_swap value to it
so that the packed fields are unswapped correctly?

> +
> +	if (ret) {
> +		perf_sample__exit(&new_sample);
> +		goto out_put;
> +	}

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