Re: [PATCH v17 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 3 potential issue(s) to consider:
- [High] Modifying memory-mapped read-only event buffers in place causes a segmentation fault.
- [High] Inconsistent handling of `(start - map__start(al.map))` for user space mappings leads to corrupted virtual address calculations.
- [High] KSYMBOL unregister events are emitted with an incorrect, newly allocated address instead of the original remapped address.
--

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

perf inject/aslr: Add ASLR tool infrastructure and MMAP tracking

If perf.data files are taken from one machine to another they may
leak virtual addresses and so weaken ASLR on the machine they are
coming from. Add an aslr option for perf inject that remaps all
virtual addresses, or drops data/events, so that the virtual address
information isn't leaked.

> diff --git a/tools/perf/util/aslr.c b/tools/perf/util/aslr.c
> new file mode 100644
> index 0000000000000..2c5fafbe5d846
> --- /dev/null
> +++ b/tools/perf/util/aslr.c

[ ... ]

> +static u64 aslr_tool__findnew_mapping(struct aslr_tool *aslr,
> +				      struct machine *session_machine,
> +				      struct thread *aslr_thread,
> +				      u8 cpumode, u64 start,
> +				      u64 len, u64 pgoff)
> +{

[ ... ]

> +		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 {
> +				/*
> +				 * Subtract pgoff from the base virtual address so that
> +				 * when the lookup path adds pgoff back, it perfectly
> +				 * cancels out and returns remap_addr.
> +				 */
> +				*new_remap_val = remap_addr - pgoff;
> +			}
> +		} else {
> +			*new_remap_val = remap_addr - (al.map ? map__pgoff(al.map) : pgoff);

[Severity: High]
Is there a missing subtraction of (start - map__start(al.map)) for the user
space mapping here? 

The kernel space path just above subtracts it. If perf merges adjacent VMAs, 
start might not equal map__start(al.map), which could result in returning an 
address inflated by that difference on subsequent lookups and corrupting 
the virtual address calculations.

> +		}
> +		err = hashmap__add(&aslr->remap_addresses, new_remap_key, new_remap_val);

[ ... ]

> +static int aslr_tool__process_ksymbol(const struct perf_tool *tool,
> +				      union perf_event *event,
> +				      struct perf_sample *sample,
> +				      struct machine *machine)
> +{

[ ... ]

> +	err = perf_event__process_ksymbol(tool, event, sample, aslr_machine);
> +	if (err)
> +		return err;
> +
> +	thread = machine__findnew_thread(aslr_machine, kernel_pid, 0);
> +	if (!thread)
> +		return -ENOMEM;
> +	memcpy(&new_event->ksymbol, &event->ksymbol, event->ksymbol.header.size);
> +	/* Remaps the ksymbol.start */
> +	new_event->ksymbol.addr = aslr_tool__findnew_mapping(aslr, machine, thread,

[Severity: High]
For KSYMBOL unregister events, does calling perf_event__process_ksymbol() 
earlier in this function delete the map from aslr_machine->kmaps before we can
look it up here?

If the map is deleted beforehand, aslr_tool__findnew_mapping() might fail to 
find the original map, fall back to generating a new unmapped key, and emit 
a mismatched remapped address instead of the original one.

> +							     PERF_RECORD_MISC_KERNEL,
> +							     event->ksymbol.addr,
> +							     event->ksymbol.len,
> +							     /*pgoff=*/0);

[ ... ]

> +void aslr_tool__strip_attr_event(union perf_event *event, struct evlist **pevlist)
> +{

[ ... ]

> +	if (event->header.size >= (offsetof(struct perf_record_header_attr,
> +					    attr.sample_type) + sizeof(u64))) {
> +		u64 st = event->attr.attr.sample_type;
> +
> +		if (needs_swap)
> +			st = bswap_64(st);
> +
> +		st &= ASLR_SUPPORTED_SAMPLE_TYPE;
> +
> +		if (needs_swap)
> +			st = bswap_64(st);
> +
> +		event->attr.attr.sample_type = st;

[Severity: High]
Does this code modify the event payload in place?

When processing a native-endian perf.data file, perf memory-maps the file's
data section with PROT_READ. If a PERF_RECORD_HEADER_ATTR event is located
in this read-only mapped data section, could writing directly to 
event->attr.attr.sample_type trigger a segmentation fault?

> +	}

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