Re: [PATCH v8 4/4] perf aslr: Strip sample registers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [High] Restoring unstripped `evsel` attributes before delegating the stripped sample event corrupts downstream processors.
- [High] In pipe mode, synthesized headers bypass `aslr_tool__process_attr`, causing the output stream to start with unstripped attributes.
- [High] The manual attribute stripping loop in `__cmd_inject` misses `bp_addr` and unsupported sample types, leaking ASLR data in the file header.
- [High] Stripping `sample_type` attributes during pipe mode inline attribute processing corrupts subsequent input sample parsing.
- [Medium] Silently ignoring allocation failures when caching original attributes leads to trace parsing desynchronization and potential out-of-bounds reads.

Pre-existing issues:
- [Critical] Stack buffer overflow when copying a variable-length `perf_event` into a statically sized `union perf_event` local variable.
--

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

perf aslr: Strip sample registers

When the ASLR tracking tool encounters sample events containing user
or interrupt register dumps, it previously dropped the entire sample event
conservatively to prevent absolute virtual memory pointers leakage.
Refactor the ASLR tool to strip out only the register dump payload by
masking out the relevant perf_event_attr fields when the delegated
tool is handling the data.

> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index f42b315199b3d..a34902ff0e77b 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
> @@ -239,6 +239,8 @@ static int perf_event__repipe_attr(const struct perf_tool *tool,
>  
>  		memcpy(&stripped_event, event, event->header.size);

[Severity: Critical]
This is a pre-existing issue, but stripped_event is a statically sized union
on the stack. 

Since PERF_RECORD_HEADER_ATTR events can be almost 64KB due to the
variable-length ID array, could copying event->header.size bytes cause a
massive stack buffer overflow?

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

[ ... ]

> @@ -2470,7 +2472,16 @@ static int __cmd_inject(struct perf_inject *inject)
>  			}
>  		}
>  
> +		if (inject->aslr) {
> +			struct evsel *evsel;
>  
> +			evlist__for_each_entry(session->evlist, evsel) {
> +				evsel__reset_sample_bit(evsel, REGS_USER);
> +				evsel__reset_sample_bit(evsel, REGS_INTR);
> +				evsel->core.attr.sample_regs_user = 0;
> +				evsel->core.attr.sample_regs_intr = 0;
> +			}
> +		}
>  
>  		session->header.data_offset = output_data_offset;
>  		session->header.data_size = inject->bytes_written;

[Severity: High]
This loop strips REGS_USER and REGS_INTR before the file header is
serialized, but it doesn't clear bp_addr for breakpoints or apply the
ASLR_SUPPORTED_SAMPLE_TYPE mask.

Since those are only cleared at the end of cmd_inject() after the header
is written, does this leak breakpoint virtual addresses and defeat ASLR
for those events?

[ ... ]

> @@ -2730,6 +2741,17 @@ int cmd_inject(int argc, const char **argv)
>  	if (zstd_init(&(inject.session->zstd_data), 0) < 0)
>  		pr_warning("Decompression initialization failed.\n");
>  
> +	if (inject.aslr) {
> +		struct evsel *evsel;
> +
> +		evlist__for_each_entry(inject.session->evlist, evsel) {
> +			ret = aslr_tool__cache_orig_attrs(tool, evsel);
> +			if (ret) {
> +				pr_err("Failed to cache original attributes: %d\n", ret);
> +				goto out_delete;
> +			}
> +		}
> +	}

[Severity: High]
When processing pipe output, perf_event__synthesize_for_pipe() uses
perf_event__repipe directly rather than the tool's configured attr callback.

Will the output pipe stream begin with unstripped ATTR headers claiming
registers are present? Does this cause trace consumers to fail when parsing
the stripped SAMPLE records?

[ ... ]

> diff --git a/tools/perf/util/aslr.c b/tools/perf/util/aslr.c
> index 901b563048fa2..457813812a34a 100644
> --- a/tools/perf/util/aslr.c
> +++ b/tools/perf/util/aslr.c

[ ... ]

> @@ -983,8 +1013,22 @@ static int aslr_tool__process_sample(const struct perf_tool *tool,
>  
>  	new_event->sample.header.size = sizeof(struct perf_event_header) + j * sizeof(u64);
>  
> +	/* Temporarily override evsel attributes to match the stripped new_event format! */
> +	orig_sample_size = evsel->sample_size;
> +	evsel->sample_size = __evsel__sample_size(sample_type);
> +	evsel->core.attr.sample_type = sample_type;
> +	evsel->core.attr.sample_regs_user = 0;
> +	evsel->core.attr.sample_regs_intr = 0;
> +
>  	perf_sample__init(&new_sample, /*all=*/ true);
>  	ret = evsel__parse_sample(evsel, new_event, &new_sample);
> +
> +	/* Restore original attributes immediately so trace ingestion never desynchronizes! */
> +	evsel->sample_size = orig_sample_size;
> +	evsel->core.attr.sample_type = orig_sample_type;
> +	evsel->core.attr.sample_regs_user = orig_regs_user;
> +	evsel->core.attr.sample_regs_intr = orig_regs_intr;
> +
>  	if (ret) {
>  		perf_sample__exit(&new_sample);
>  		goto out_put;

[Severity: High]
If downstream delegates like perf_event__convert_sample_callchain use
evsel->core.attr.sample_type to parse the event, will they expect register
data that is no longer present in new_event?

Since we restored the original attributes before calling the delegate, could
this read past the end of new_event and cause data corruption?

Should we restore the attributes after the delegate returns?

[ ... ]

> @@ -1010,6 +1054,7 @@ static int aslr_tool__process_attr(const struct perf_tool *tool,
>  	struct aslr_tool *aslr;
>  	struct perf_tool *delegate;
>  	union perf_event *new_event;
> +	int err;
>  
>  	del_tool = container_of(tool, struct delegate_tool, tool);
>  	aslr = container_of(del_tool, struct aslr_tool, tool);
> @@ -1020,9 +1065,32 @@ static int aslr_tool__process_attr(const struct perf_tool *tool,
>  	if (new_event->attr.attr.type == PERF_TYPE_BREAKPOINT)
>  		new_event->attr.attr.bp_addr = 0;  /* Conservatively remove addresses. */
>  
> +	if (new_event->attr.attr.sample_type & PERF_SAMPLE_REGS_USER) {
> +		new_event->attr.attr.sample_type &= ~PERF_SAMPLE_REGS_USER;
> +		new_event->attr.attr.sample_regs_user = 0;
> +	}
> +	if (new_event->attr.attr.sample_type & PERF_SAMPLE_REGS_INTR) {
> +		new_event->attr.attr.sample_type &= ~PERF_SAMPLE_REGS_INTR;
> +		new_event->attr.attr.sample_regs_intr = 0;
> +	}
> +

[Severity: High]
When processing inline attributes in pipe mode, this strips the register flags
before delegating to perf_event__repipe_attr(). 

When subsequent PERF_RECORD_SAMPLE events arrive from the input pipe, will
evsel__parse_sample() use this stripped evsel to parse the unstripped raw
input buffer? Could this cause the parser to misinterpret the unread register
data as subsequent fields and drop the samples?

>  	new_event->attr.attr.sample_type &= ASLR_SUPPORTED_SAMPLE_TYPE;
>  
> -	return delegate->attr(delegate, new_event, pevlist);
> +	err = delegate->attr(delegate, new_event, pevlist);
> +	if (!err && pevlist && *pevlist) {
> +		struct evsel *evsel = evlist__last(*pevlist);
> +		struct aslr_evsel_priv *priv = zalloc(sizeof(*priv));
> +
> +		if (priv) {
> +			priv->orig_sample_type = event->attr.attr.sample_type;
> +			priv->orig_sample_regs_user = event->attr.attr.sample_regs_user;
> +			priv->orig_sample_regs_intr = event->attr.attr.sample_regs_intr;
> +			if (hashmap__add(&aslr->evsel_orig_attrs, evsel, priv) != 0)
> +				free(priv);
> +		}
> +	}
> +
> +	return err;
>  }

[Severity: Medium]
If zalloc() or hashmap__add() fails here, the error is silently ignored.

When aslr_tool__process_sample() later fails the hashmap lookup, it will fall
back to an evsel->core.attr.sample_type that lacks the register flags. Will
parsing the unstripped raw kernel sample array with this stripped mask cause
the manual parser to desynchronize and misinterpret the unread register data?

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