Re: [PATCH v20 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] Addresses that cannot be remapped are destructively overwritten with 0.
- [Medium] Missing union-based repacking for PERF_SAMPLE_CPU causes cross-endian event synthesis failure.
--

commit 4363deb515e79cb7e3488a5d19b247858fb88edd
Author: Gabriel Marin <gmx@google.com>

Add the sample address remapping logic to the ASLR tool.

This patch implements sample event processing for the ASLR tool, parsing
sample events and remapping addresses like IPs, callchains, and branch stacks.
It also adds the aslr_tool__remap_address helper function.

> diff --git a/tools/perf/util/aslr.c b/tools/perf/util/aslr.c
> --- a/tools/perf/util/aslr.c
> +++ b/tools/perf/util/aslr.c
> @@ -112,6 +113,60 @@ static u64 round_up_to_page_size(u64 addr)
>  	return (addr + page_size - 1) & ~((u64)page_size - 1);
>  }
>  
> +static u64 aslr_tool__remap_address(struct aslr_tool *aslr,
> +				    struct thread *aslr_thread,
> +				    u8 cpumode,
> +				    u64 addr)
> +{
> +	struct addr_location al;
> +	struct remap_addresses_key key;
> +	u64 *remapped_invariant_ptr = NULL;
> +	u64 remap_addr = 0;

[ ... ]

> +		if (!thread__find_map(aslr_thread, effective_cpumode, addr, &al)) {
> +			addr_location__exit(&al);
> +			return 0; /* No mmap. */

[Severity: High]
Does returning 0 here destructively overwrite valid non-randomized addresses?

If an address cannot be mapped (such as JIT code, vsyscall, or missing map
regions), returning 0 unconditionally overwrites the sample's IP, ADDR,
callchains, and branch stacks. Should this return the original addr instead
to avoid clustering profiler trace data at NULL?

> +		}
> +	}
> +
> +	key.machine = maps__machine(thread__maps(aslr_thread));
> +	key.dso = map__dso(al.map);
> +	key.invariant = map__start(al.map) - map__pgoff(al.map);
> +	key.pid = (effective_cpumode == PERF_RECORD_MISC_KERNEL ||
> +		   effective_cpumode == PERF_RECORD_MISC_GUEST_KERNEL) ?
> +		  kernel_pid : thread__pid(aslr_thread);
> +
> +	if (hashmap__find(&aslr->remap_addresses, &key, &remapped_invariant_ptr)) {
> +		remap_addr = *remapped_invariant_ptr + map__pgoff(al.map) +
> +			     (addr - map__start(al.map));
> +	} else {
> +		pr_debug("Cannot find a remapped entry for address %lx in mapping %lx(%lx) for pid=%d\n",
> +			 addr, map__start(al.map), map__size(al.map), key.pid);
> +	}
> +
> +	addr_location__exit(&al);
> +	return remap_addr;

[Severity: High]
Similarly, when the mapping is not in the remap_addresses hashmap, this will
return remap_addr which was initialized to 0. Would it be safer to default
remap_addr to the original addr instead of 0?

> +}

[ ... ]

> +	if (sample_type & PERF_SAMPLE_ID)
> +		COPY_U64(); /* id */
> +	if (sample_type & PERF_SAMPLE_STREAM_ID)
> +		COPY_U64(); /* stream_id */
> +	if (sample_type & PERF_SAMPLE_CPU)
> +		COPY_U64(); /* cpu, res */

[Severity: Medium]
The commit message states that both PERF_SAMPLE_TID and PERF_SAMPLE_CPU must
be unpacked and repacked using unions to ensure correct 32-bit layout
alignment for cross-endian hosts. However, it looks like PERF_SAMPLE_CPU
blindly uses COPY_U64() here instead of a union.

Since the generic perf event parsing layer byte-swaps the 64-bit payload
array, this reverses the internal order of the two 32-bit fields (cpu and
res). Does copying this improperly swapped 64-bit word verbatim write the
reversed fields directly to the output, corrupting them when reparsed with
needs_swap=false?

> +	if (sample_type & PERF_SAMPLE_PERIOD)
> +		COPY_U64(); /* period */

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