Re: [PATCH v16 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] The attribute size check incorrectly uses `event->header.size` instead of `attr.size`, potentially overwriting the `id` array.
- [Medium] The ASLR tool's isolated `machines` collection lacks a `perf_env` pointer, forcing kernel space mapping to silently fall back to the host's compilation architecture bitness.
--

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

This patch introduces the core ASLR remapping tool infrastructure and
implements remapping/tracking for metadata events. It adds an aslr option
for perf inject that remaps all virtual addresses to prevent virtual address
information leakage when sharing perf.data files.

> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
[ ... ]
> @@ -242,8 +245,43 @@ static int perf_event__repipe_attr(const struct perf_tool *tool,
[ ... ]
> +			if (stripped_event->attr.attr.type == PERF_TYPE_BREAKPOINT &&
> +			    event->header.size >= (offsetof(struct perf_record_header_attr,
> +							    attr.bp_addr) + sizeof(u64))) {
> +				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"))) {
> +					if (event->header.size >=
> +					    (offsetof(struct perf_record_header_attr,
> +						      attr.config1) + sizeof(u64)))
> +						stripped_event->attr.attr.config1 = 0;
> +					if (event->header.size >=
> +					    (offsetof(struct perf_record_header_attr,
> +						      attr.config2) + sizeof(u64)))
> +						stripped_event->attr.attr.config2 = 0;
> +				}
> +			}

[Severity: High]
Does this incorrectly check event->header.size instead of the actual
attr.size?

If an older perf.data file is processed with a shorter ABI struct (such as the
64-byte PERF_ATTR_SIZE_VER0) and contains appended IDs, this boundary check
would pass because the total event size exceeds the offset.

Could this cause the assignment to config1 or config2 to overwrite the leading
elements of the appended id[] array with zeroes, permanently corrupting the
event ID stream?

> diff --git a/tools/perf/util/aslr.c b/tools/perf/util/aslr.c
> --- /dev/null
> +++ b/tools/perf/util/aslr.c
[ ... ]
> +static void aslr_tool__init(struct aslr_tool *aslr, struct perf_tool *delegate)
> +{
> +	delegate_tool__init(&aslr->tool, delegate);
> +	aslr->tool.tool.ordered_events = true;
> +
> +	machines__init(&aslr->machines);
[ ... ]
> +static u64 aslr_tool__findnew_mapping(struct aslr_tool *aslr,
[ ... ]
> +	} else {
> +		/* First address of the process, allocate key and first top address. */
> +		struct top_addresses_key *tk;
> +		struct process_top_address *top_val;
> +		struct machine *machine = remap_key.machine;
> +		struct perf_env *env = machine ? machine->env : NULL;
> +		bool is_64 = env ? perf_env__kernel_is_64_bit(env) : (sizeof(void *) == 8);

[Severity: Medium]
Because machines__init() creates an isolated collection of machines without
linking its host environment to the primary session's environment, will env
always resolve to NULL here?

If env is NULL, this falls back to the executing host's pointer size
(sizeof(void *) == 8) rather than the recorded target architecture.

Will this break cross-platform analysis when analyzing a 32-bit perf.data file
on a 64-bit host?

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