Re: [PATCH v7 2/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] Modifying `evsel->core.attr.sample_type` upfront corrupts the input event parser, causing misaligned reads of raw payload data.
- [High] Ineffective deadlock mitigation due to pre-loading kernel maps into the wrong `machines` struct and premature guest evaluation.
- [High] `aslr_tool__process_ksymbol` truncates ksymbol addresses to the base address of their containing map.
--

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

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

This commit introduces an ASLR tool for perf inject that remaps virtual
addresses to prevent leaking address space layouts when sharing perf.data
files. It also attempts to eliminate lazy-loading deadlocks by preloading
kernel and module maps before processing events.

> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index a2493f1097df9..8fe924e730a16 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
[ ... ]
> @@ -2267,6 +2269,56 @@ static int output_fd(struct perf_inject *inject)
[ ... ]
> +/*
> + * Scans and force-loads all registered host and guest machine kernel and
> + * module maps up front before event processing starts.
> + */
> +static int perf_inject__load_kernel_maps(struct perf_inject *inject)
> +{
> +	struct machine *machine;
> +	struct rb_node *nd;
> +	int err = 0;
> +
> +	/* Load host kernel maps up front */
> +	if (machine__load_kernel_maps(&inject->session->machines.host) < 0)
> +		err = -1;
> +
> +	/* Load all guest machines kernel maps up front */
> +	for (nd = rb_first_cached(&inject->session->machines.guests); nd; nd = rb_next(nd)) {
> +		machine = rb_entry(nd, struct machine, rb_node);
> +		if (machine__load_kernel_maps(machine) < 0)
> +			err = -1;
> +	}
> +
> +	return err;
> +}

Does this preload the kernel maps into the correct machines collection?

It looks like this loads into inject->session->machines, but the ASLR tool
uses its own isolated aslr->machines collection which starts empty. Because
the preloaded maps are not available to the ASLR tool, will kernel maps still
be lazy-loaded dynamically during runtime event processing?

Also, since this is called before perf_session__process_events(), are there
any guest machines discovered from the event stream yet to iterate over?

[ ... ]
> @@ -2689,18 +2758,38 @@ int cmd_inject(int argc, const char **argv)
[ ... ]
> +	if (inject.aslr) {
> +		struct evsel *evsel;
> +
> +		evlist__for_each_entry(inject.session->evlist, evsel) {
> +			evsel->core.attr.sample_type &= ASLR_SUPPORTED_SAMPLE_TYPE;
> +
> +			if (evsel->core.attr.type == PERF_TYPE_BREAKPOINT)
> +				evsel->core.attr.bp_addr = 0;
> +		}
> +	}

Does modifying the sample_type upfront cause the input event parser to
misread the physical perf.data file?

The input file was recorded with the original sample_type, so its payloads
still contain the stripped fields. In aslr_tool__process_sample(), when it
checks the masked sample_type:

    sample_type = evsel->core.attr.sample_type;
    ...
    if (sample_type & PERF_SAMPLE_REGS_USER) { ... }

Won't it fail to advance the input array index over the register payload,
causing subsequent checks to mistakenly read the leftover register payload
data as stack sizes or lengths?

> diff --git a/tools/perf/util/aslr.c b/tools/perf/util/aslr.c
> new file mode 100644
> index 0000000000000..d0b1b33377fd2
> --- /dev/null
> +++ b/tools/perf/util/aslr.c
[ ... ]
> +static int aslr_tool__process_ksymbol(const struct perf_tool *tool,
> +				      union perf_event *event,
> +				      struct perf_sample *sample,
> +				      struct machine *machine)
> +{
[ ... ]
> +	/* Remaps the ksymbol.start */
> +	new_event->ksymbol.addr = aslr_tool__findnew_mapping(aslr, thread,
> +							     PERF_RECORD_MISC_KERNEL,
> +							     event->ksymbol.addr,
> +							     event->ksymbol.len,
> +							     /*pgoff=*/0);
> +
> +	err = delegate->ksymbol(delegate, new_event, sample, machine);
> +	thread__put(thread);
> +	return err;
> +}

Could this truncate ksymbol addresses?

The aslr_tool__findnew_mapping() function appears to return the remapped base
address of the entire map when an address falls into an existing map, but it
does not seem to add the symbol's internal offset.

Will this cause ksymbols inside an existing kernel map to be clustered at the
exact start address of the mapping in the output trace?

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