Re: [PATCH v6 4/6] perf inject/aslr: Add aslr tool to remap/obfuscate virtual addresses

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The sample repacking logic silently drops unknown fields without clearing their corresponding UAPI presence flags, corrupting downstream parsing.
- [High] Missing PERF_RECORD_MISC_GUEST_KERNEL checks in mapping and remapping logic break ASLR obfuscation for guest kernel samples.
--

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

perf inject/aslr: Add aslr tool to remap/obfuscate virtual addresses

This commit introduces a new ASLR obfuscation tool to perf inject that
remaps or drops virtual addresses to prevent ASLR leaks when sharing
perf.data files. It uses a delegate tool architecture to intercept and
sanitize events and samples.

> diff --git a/tools/perf/util/aslr.c b/tools/perf/util/aslr.c
> new file mode 100644
> index 0000000000000..09b7f2f8fb850
> --- /dev/null
> +++ b/tools/perf/util/aslr.c
[ ... ]
> +static u64 aslr_tool__remap_address(struct aslr_tool *aslr,
> +				    struct thread *aslr_thread,
> +				    u8 cpumode,
> +				    u64 addr)
> +{
[ ... ]
> +	key.machine = maps__machine(aslr_thread->maps);
> +	key.dso = map__dso(al.map);
> +	key.invariant = map__start(al.map) - map__pgoff(al.map);
> +	key.pid = effective_cpumode == PERF_RECORD_MISC_KERNEL ? kernel_pid : aslr_thread->pid_;

Does this also need to check for PERF_RECORD_MISC_GUEST_KERNEL? If a guest
kernel sample interrupts a user process, missing this check might cause
key.pid to be assigned aslr_thread->pid_ instead of kernel_pid (-1). Could
this mismatch prevent hashmap__find() from resolving the mapping?

[ ... ]
> +static u64 aslr_tool__findnew_mapping(struct aslr_tool *aslr,
> +				       struct thread *aslr_thread,
> +				       u8 cpumode, u64 start,
> +				       u64 len, u64 pgoff)
> +{
[ ... ]
> +	/* The key to look up an incoming address to the outgoing value. */
> +	addr_location__init(&al);
> +	remap_key.machine = maps__machine(aslr_thread->maps);
> +	remap_key.pid = (cpumode == PERF_RECORD_MISC_KERNEL) ? kernel_pid : aslr_thread->pid_;

Similarly here, if cpumode is PERF_RECORD_MISC_GUEST_KERNEL, shouldn't
remap_key.pid also be set to kernel_pid?

[ ... ]
> +	/* No mmap, create an entry from the top address. */
> +	if (hashmap__find(&aslr->top_addresses, &top_addr_key, &pmax)) {
[ ... ]
> +	} else {
> +		/* First address of the process, allocate key and first top address. */
> +		struct top_addresses_key *tk;
> +
> +		remap_addr = (cpumode == PERF_RECORD_MISC_KERNEL) ?
> +			     kernel_space_start : user_space_start;

Would guest kernel maps erroneously be allocated in the user_space_start
range here instead of kernel_space_start due to the missing check for
PERF_RECORD_MISC_GUEST_KERNEL?

[ ... ]
> +static int aslr_tool__process_sample(const struct perf_tool *tool, union perf_event *event,
> +				struct perf_sample *sample,
> +				struct evsel *evsel, struct machine *machine)
> +{
[ ... ]
> +	if (sample_type & PERF_SAMPLE_AUX) {
> +		u64 size;
> +
> +		CHECK_BOUNDS(1, 1);
> +		size = out_array[j++] = in_array[i++];
> +		if (size > 0) {
> +			size_t u64_words = size / 8 + (size % 8 ? 1 : 0);
> +
> +			if (u64_words > max_i - i || u64_words > max_j - j) {
> +				ret = -EFAULT;
> +				goto out_put;
> +			}
> +			memcpy(&out_array[j], &in_array[i], size);
> +			if (size % 8) {
> +				size_t pad = 8 - (size % 8);
> +
> +				memset(((char *)&out_array[j]) + size, 0, pad);
> +			}
> +			i += u64_words;
> +			j += u64_words;
> +		}
> +		/* TODO: can this be less conservative? */
> +		pr_debug("Dropping aux sample as possible ASLR leak\n");
> +		ret = 0;
> +		goto out_put;
> +	}
> +
> +	if (evsel__is_offcpu_event(evsel)) {
> +		/* TODO: can this be less conservative? */
> +		pr_debug("Dropping off-CPU sample as possible ASLR leak\n");
> +		ret = 0;
> +		goto out_put;
> +	}
> +
> +	new_event->sample.header.size = sizeof(struct perf_event_header) + j * sizeof(u64);

If the sample contains unknown UAPI fields (for instance, when processing a
perf.data file generated by a newer kernel), they appear to be silently
dropped from the payload during this repacking loop.

However, since unrecognized bits aren't cleared from evsel->core.attr,
will downstream parsers encounter missing fields while expecting them to
be present? Could this lead to reading misaligned data or triggering
-EFAULT parsing failures when analyzing the resulting trace?

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