Re: [PATCH v15 2/5] perf inject/aslr: Add ASLR tool infrastructure and MMAP tracking

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unvalidated writes to `config1` and `config2` in `perf_event__repipe_attr` cause a heap buffer overflow for older ABI events.
- [High] Asymmetric computation for kernel mappings without an associated DSO causes massive address overflows on lookup.
--

commit 493cc968b1147b3f20e3102e4ae64844207831a1
Author: Ian Rogers <irogers@google.com>
perf inject/aslr: Add ASLR tool infrastructure and MMAP tracking

This patch adds an ASLR tool to perf inject, allowing it to remap virtual
addresses or drop sensitive data to prevent leaking a machine's ASLR layout
when sharing perf.data files. It implements core ASLR remapping and tracking
for metadata events like MMAP, FORK, and KSYMBOL. Sample events are
currently delegated without remapping.

> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index 75ffe31d03fea..6ac6e6fb3b47e 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
[ ... ]
> @@ -242,8 +245,35 @@ static int perf_event__repipe_attr(const struct perf_tool *tool,
>  	if (!inject->output.is_pipe)
>  		return 0;
>  
> -	if (!inject->itrace_synth_opts.set)
> +	if (!inject->itrace_synth_opts.set) {
> +		if (inject->aslr) {
> +			union perf_event *stripped_event = malloc(event->header.size);
> +			int err;
> +
> +			if (!stripped_event)
> +				return -ENOMEM;
> +			memcpy(stripped_event, event, event->header.size);
> +			stripped_event->attr.attr.sample_type &= ASLR_SUPPORTED_SAMPLE_TYPE;
> +
> +			if (stripped_event->attr.attr.type == PERF_TYPE_BREAKPOINT) {
> +				stripped_event->attr.attr.bp_addr = 0;
> +			} else if (stripped_event->attr.attr.type >= PERF_TYPE_MAX) {
> +				struct perf_pmu *pmu;
> +
> +				pmu = perf_pmus__find_by_type(stripped_event->attr.attr.type);
> +				if (pmu && (!strcmp(pmu->name, "kprobe") ||
> +					    !strcmp(pmu->name, "uprobe"))) {
> +					stripped_event->attr.attr.config1 = 0;
> +					stripped_event->attr.attr.config2 = 0;
> +				}
> +			}
> +
> +			err = perf_event__repipe_synth(tool, stripped_event);
> +			free(stripped_event);
> +			return err;
> +		}

[Severity: High]
Does this code risk a heap buffer overflow for older ABI events?

In perf_event__repipe_attr(), the stripped_event buffer is allocated with
event->header.size. For older ABI0 perf.data files, this size can be as small
as 72 bytes.

The code here unconditionally writes to bp_addr (at offset 88), config1
(offset 88), and config2 (offset 96) for relevant events, without verifying if
event->header.size is large enough to contain these fields. Could this write
up to 32 bytes past the end of the stripped_event heap allocation?

[ ... ]
> diff --git a/tools/perf/util/aslr.c b/tools/perf/util/aslr.c
> new file mode 100644
> index 0000000000000..16537e0e1bbb4
> --- /dev/null
> +++ b/tools/perf/util/aslr.c
[ ... ]
> @@ -0,0 +1,718 @@
[ ... ]
> +static u64 aslr_tool__findnew_mapping(struct aslr_tool *aslr,
> +				      struct thread *aslr_thread,
> +				      u8 cpumode, u64 start,
> +				      u64 len, u64 pgoff)
> +{
[ ... ]
> +	if (hashmap__find(&aslr->remap_addresses, &remap_key, &remapped_invariant_ptr)) {
> +		/* Mmap already exists. */
> +		u64 calculated_max;
> +
> +		if (al.map) {
> +			remap_addr = *remapped_invariant_ptr + map__pgoff(al.map) +
> +				     (start - map__start(al.map));
> +		} else {
> +			remap_addr = *remapped_invariant_ptr + pgoff;
> +		}

[Severity: High]
Does this lookup path correctly handle the invariant when al.map is NULL?

If al.map is NULL, remap_addr is computed as *remapped_invariant_ptr + pgoff.

[ ... ]

> +	/* Create rmeapping entry. */
> +	new_remap_key = malloc(sizeof(*new_remap_key));
> +	new_remap_val = malloc(sizeof(u64));
> +	if (!new_remap_key || !new_remap_val) {
> +		err = -ENOMEM;
> +	} else {
> +		*new_remap_key = remap_key;
> +		new_remap_key->dso = dso__get(remap_key.dso);
> +		if (cpumode == PERF_RECORD_MISC_KERNEL ||
> +		    cpumode == PERF_RECORD_MISC_GUEST_KERNEL) {
> +			if (al.map) {
> +				*new_remap_val = remap_addr -
> +						 (start - map__start(al.map)) -
> +						 map__pgoff(al.map);
> +			} else {
> +				*new_remap_val = remap_addr;
> +			}

[Severity: High]
Is it possible this logic creates an asymmetric computation for kernel
mappings without an associated DSO?

When al.map is NULL during insert, this stores remap_addr directly into
*new_remap_val without subtracting pgoff. However, as seen in the lookup path
above, it unconditionally adds pgoff to the cached value when al.map is NULL.

For anonymous kernel maps, the kernel sets pgoff to the original virtual
address in bytes. This asymmetric addition seems like it would cause a massive
overflow by adding a 64-bit kernel address to the already high remap_addr,
drastically corrupting the returned remapped address on lookups.

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