Re: [PATCH v8 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 2 potential issue(s) to consider:
- [Critical] A malicious perf.data file can trigger a stack buffer overflow in `perf_event__repipe_attr` by providing an event with a maximized size.
- [High] Prematurely masking `sample_type` in `aslr_tool__process_attr()` corrupts the parsing of subsequent sample payloads.
--

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

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

    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/builtin-inject.c b/tools/perf/builtin-inject.c
> index a2493f1097df9..f42b315199b3d 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
> @@ -232,6 +234,14 @@ static int perf_event__repipe_attr(const struct perf_tool *tool,
>  	if (!inject->output.is_pipe)
>  		return 0;
>  
> +	if (inject->aslr) {
> +		union perf_event stripped_event;
> +
> +		memcpy(&stripped_event, event, event->header.size);

[Severity: Critical]
Is it possible for event->header.size to exceed sizeof(union perf_event)?

perf_session__process_events() reads events from the perf.data file and
dynamically reallocates its heap buffer if an event's header.size exceeds
sizeof(union perf_event). The format allows a maximum size of 65535 bytes.

If a malicious or unusually large event is processed, could this memcpy
overflow the local stripped_event buffer on the stack, potentially leading
to a return address overwrite?

> +		stripped_event.attr.attr.sample_type &= ASLR_SUPPORTED_SAMPLE_TYPE;
> +		return perf_event__repipe_synth(tool, &stripped_event);
> +	}
> +
>  	return perf_event__repipe_synth(tool, event);
>  }

[ ... ]

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

[ ... ]

> +static int aslr_tool__process_attr(const struct perf_tool *tool,
> +				   union perf_event *event,
> +				   struct evlist **pevlist)
> +{
> +	struct delegate_tool *del_tool;
> +	struct aslr_tool *aslr;
> +	struct perf_tool *delegate;
> +	union perf_event *new_event;
> +
> +	del_tool = container_of(tool, struct delegate_tool, tool);
> +	aslr = container_of(del_tool, struct aslr_tool, tool);
> +	delegate = aslr->tool.delegate;
> +	new_event = (union perf_event *)aslr->event_copy;
> +
> +	memcpy(&new_event->attr, &event->attr, event->attr.header.size);
> +	if (new_event->attr.attr.type == PERF_TYPE_BREAKPOINT)
> +		new_event->attr.attr.bp_addr = 0;  /* Conservatively remove addresses. */
> +
> +	new_event->attr.attr.sample_type &= ASLR_SUPPORTED_SAMPLE_TYPE;

[Severity: High]
Does prematurely masking sample_type here corrupt the parsing of subsequent
sample payloads?

If evsel is created in the core session lacking the masked bits (e.g.,
PERF_SAMPLE_REGS_USER), evlist__parse_sample() will rely on this masked
sample_type when a raw PERF_RECORD_SAMPLE event is later read from the file.

However, the raw event on disk still contains the data bytes for the masked
fields. Could the parser fail to skip them and instead misalign, reading
these raw bytes as subsequent fields? This might cause evsel__parse_sample
to fail with -EFAULT or corrupt the parsed data.

> +
> +	return delegate->attr(delegate, new_event, pevlist);
> +}

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