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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Kernel ASLR base is leaked through unmodified mmap `pgoff` fields.
- [Medium] The 'targeted tail-word clearing' logic for STACK_USER and AUX frames is dead code because the entire sample is unconditionally dropped immediately after.
- [Medium] The `skipn()` pipe helper lacks retry logic for `-EINTR`, risking spurious aborts.
--

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

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

This commit introduces an '--aslr' option for perf inject to remap virtual
addresses in perf.data files. This prevents leaking virtual addresses and
weakening ASLR on the source machine. Events with unknown or unhandled
addresses are dropped to remain conservative.

> diff --git a/tools/perf/util/aslr.c b/tools/perf/util/aslr.c
> new file mode 100644
> index 000000000000..27b642c91eb9
> --- /dev/null
> +++ b/tools/perf/util/aslr.c
> @@ -0,0 +1,1161 @@

[ ... ]

> +static int skipn(int fd, u64 n)
> +{
> +	char buf[4096];
> +	ssize_t ret;
> +
> +	while (n > 0) {
> +		ret = read(fd, buf, (n < (u64)sizeof(buf) ? n : (u64)sizeof(buf)));
> +		if (ret <= 0)
> +			return ret;

Will this cause spurious aborts if read() is interrupted by a signal?

Since this is reading from a pipe, if read() returns -1 with errno == EINTR,
skipn() will return the error immediately rather than retrying.

> +		n -= ret;
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static int aslr_tool__process_mmap(const struct perf_tool *tool,
> +				union perf_event *event,
> +				struct perf_sample *sample,
> +				struct machine *machine)
> +{
[ ... ]
> +	memcpy(&new_event->mmap, &event->mmap, event->mmap.header.size);
> +	/* Remaps the mmap.start. */
> +	new_event->mmap.start = aslr_tool__remap_mapping(aslr, thread, cpumode,
> +							   event->mmap.start,
> +							   event->mmap.len,
> +							   event->mmap.pgoff);

Does this leak the kernel ASLR base?

For the main kernel mapping, the perf tool synthesizes mmap events where the
mmap.pgoff field is set to the original kernel start address. Since pgoff is
left unmodified here, the unobfuscated kernel base address will be copied
directly into the output stream.

> +	err = delegate->mmap(delegate, new_event, sample, machine);
> +	thread__put(thread);
> +	return err;
> +}

[ ... ]

> +static int aslr_tool__process_mmap2(const struct perf_tool *tool,
> +				union perf_event *event,
> +				struct perf_sample *sample,
> +				struct machine *machine)
> +{
[ ... ]
> +	memcpy(&new_event->mmap2, &event->mmap2, event->mmap2.header.size);
> +	/* Remaps the mmap.start. */
> +	new_event->mmap2.start = aslr_tool__remap_mapping(aslr, thread, cpumode,
> +							   event->mmap2.start,
> +							   event->mmap2.len,
> +							   event->mmap2.pgoff);

Similarly, does this leave the unobfuscated kernel base address in pgoff
for mmap2 events?

> +	err = delegate->mmap2(delegate, new_event, sample, machine);
> +	thread__put(thread);
> +	return err;
> +}

[ ... ]

> +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_STACK_USER) {
> +		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);
> +			}

Is this tail-word clearing dead code?

> +			i += u64_words;
> +			j += u64_words;
> +
> +			COPY_U64(); /* dyn_size */
> +		}
> +		/* TODO: can this be less conservative? */
> +		pr_debug("Dropping stack user sample as possible ASLR leak\n");
> +		ret = 0;
> +		goto out_put;

Immediately after clearing the trailing heap padding in the output array, the
code unconditionally jumps to out_put with ret = 0, skipping the sample
delegate call. This drops the entire sample from the output stream.

> +	}

[ ... ]

> +	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;
> +	}

Does this code also unnecessarily calculate and clear the tail padding for
AUX frames, given that the sample is unconditionally dropped?

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