Re: [PATCH v5 4/5] 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] Cross-VM Isolation Failure in ASLR Remapping
- [Medium] Callchain Parsing Omission for `PERF_CONTEXT_USER_DEFERRED`
- [Low] Fragile Alignment of `event_copy` Buffer
--

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

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

This patch adds an aslr option for perf inject that remaps all virtual
addresses to prevent leaking host ASLR information. It uses a struct
machines instance to isolate host mappings from KVM guest address spaces.

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

[ ... ]

> +struct remap_addresses_key {
> +	struct dso *dso;
> +	u64 invariant;
> +	pid_t pid;
> +};

Is there a missing machine identifier in this struct? If host processes and
guest processes share the same PID and map anonymous memory at the same
offset, it appears their keys will collide, causing the guest to reuse the
host's remapped address.

[ ... ]

> +struct aslr_tool {
> +	/** @tool: The tool implemented here and a pointer to a delegate to process the data. */
> +	struct delegate_tool tool;
> +	/** @machines: The machines with the input, not remapped, virtual address layout. */
> +	struct machines machines;
> +	/** @event_copy: Buffer used to create an event to pass to the delegate. */
> +	char event_copy[PERF_SAMPLE_MAX_SIZE];

This isn't a bug, but since event_copy is declared as a char array, it only
guarantees a 1-byte alignment. While it currently happens to be 8-byte
aligned because it follows struct machines, could future additions to
struct aslr_tool break this alignment and cause crashes on strict-alignment
architectures when it is cast to union perf_event *?

> +	/** @remap_addresses: mapping from remap_addresses_key to remapped address. */
> +	struct hashmap remap_addresses;
> +	/** @top_addresses: mapping from process to max remapped address. */
> +	struct hashmap top_addresses;

Since top_addresses tracks allocations by pid alone, will host and guest
processes with the same PID share the same allocation pool and intertwine
their memory layouts?

[ ... ]

> +static u64 aslr_tool__remap_ksymbol(struct aslr_tool *aslr,
> +				    struct thread *aslr_thread,
> +				    u64 addr, u32 len)
> +{

[ ... ]

> +	hashmap__for_each_entry(&aslr->remap_addresses, cur, bkt) {
> +		k = (struct remap_addresses_key *)cur->pkey;
> +		if (k->pid == kernel_pid && k->invariant == key.invariant) {
> +			v = (u64 *)cur->pvalue;
> +
> +			if (al.map)
> +				remap_addr = *v + map__pgoff(al.map) +
> +					     (addr - map__start(al.map));
> +			else
> +				remap_addr = *v;
> +			addr_location__exit(&al);
> +			return remap_addr;
> +		}
> +	}

Because both host and guest use kernel_pid (-1), will this loop match the
first entry regardless of dso identity and cause guest kernel symbols to
collide with host kernel symbols, leaking host kernel layouts to the guest?

[ ... ]

> +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)
> +{

[ ... ]

> +				case PERF_CONTEXT_GUEST_USER:
> +					cpumode = PERF_RECORD_MISC_GUEST_USER;
> +					break;
> +				case PERF_CONTEXT_USER_DEFERRED:
> +					/*
> +					 * Immediately followed by a 64-bit
> +					 * stitching cookie. Skip/Copy it!
> +					 */
> +					CHECK_BOUNDS(1, 1);
> +					out_array[j++] = in_array[i++];
> +					cntr++;
> +					break;

Should this case also update cpumode to PERF_RECORD_MISC_USER?

Without it, subsequent user-space addresses will be processed using the
preceding CPU mode (usually kernel). If a user address overlaps with a valid
kernel map, it looks like it could be erroneously remapped as a kernel
address.

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