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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Modifying `evsel->core.attr.sample_type` before input event processing corrupts the raw event stream parser.
- [Low] Build failure due to undeclared `ASLR_SUPPORTED_SAMPLE_TYPE` identifier.
- [Low] Commit message discrepancy regarding the handling of ABI words.
- [Low] Error masking in `aslr_tool__cache_orig_attrs` hides out-of-memory errors.
--

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

perf aslr: Strip sample registers

This commit refactors the ASLR tool to strip register dumps from sample
events rather than dropping the entire sample. It scrubs incoming attribute
events and attempts to keep downstream parsers synchronized.

> and output sample ABI words are safely overwritten to
> PERF_SAMPLE_REGS_ABI_NONE.

This isn't a bug, but the commit message states that the ABI words are
overwritten to PERF_SAMPLE_REGS_ABI_NONE. However, the code in
aslr_tool__process_sample() skips writing to the output array entirely,
stripping the ABI words and register payloads rather than overwriting them.

Is this description accurate?

> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index 8fe924e730a16..4bafccf7dae40 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
[ ... ]
> @@ -2783,7 +2794,18 @@ int cmd_inject(int argc, const char **argv)
>  		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;
> +			}
> +
> +			/* Strip the registers and unknown flags natively inside memory! */
>  			evsel->core.attr.sample_type &= ASLR_SUPPORTED_SAMPLE_TYPE;

This macro is removed from tools/perf/util/aslr.c in this patch and doesn't
appear to be relocated to a shared header. Will this cause a build failure
due to an undeclared identifier?

> +			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;

During trace ingestion, the core parser relies on evsel->core.attr.sample_type
to deserialize raw binary samples linearly. 

If the register bits are cleared prematurely from the in-memory evsel here in
cmd_inject() (before __cmd_inject() executes perf_session__process_events()),
the parser will expect no register data and skip extracting those fields.

If the raw sample contains subsequent trailing fields like
PERF_SAMPLE_STACK_USER, will the unskipped raw register bytes physically
present in the stream be misinterpreted as those trailing fields?

Could this result in out-of-bounds memory reads and -EFAULT errors,
effectively aborting trace processing for files recorded with registers?

> diff --git a/tools/perf/util/aslr.c b/tools/perf/util/aslr.c
> index d0b1b33377fd2..ef6ba6fa5ff45 100644
> --- a/tools/perf/util/aslr.c
> +++ b/tools/perf/util/aslr.c
[ ... ]
> @@ -1097,10 +1122,34 @@ void aslr_tool__delete(struct perf_tool *tool)
[ ... ]
> +int aslr_tool__cache_orig_attrs(struct perf_tool *tool, struct evsel *evsel)
> +{
> +	struct delegate_tool *del_tool = container_of(tool, struct delegate_tool, tool);
> +	struct aslr_tool *aslr = container_of(del_tool, struct aslr_tool, tool);
> +	struct aslr_evsel_priv *priv = zalloc(sizeof(*priv));
> +
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->orig_sample_type = evsel->core.attr.sample_type;
> +	priv->orig_sample_regs_user = evsel->core.attr.sample_regs_user;
> +	priv->orig_sample_regs_intr = evsel->core.attr.sample_regs_intr;
> +
> +	if (hashmap__add(&aslr->evsel_orig_attrs, evsel, priv) != 0) {
> +		free(priv);
> +		return -EEXIST;
> +	}
> +	return 0;
> +}

If memory allocation fails for the new bucket or entry inside hashmap__add(),
it will return -ENOMEM.

Since the code assumes any non-zero return indicates the entry already exists
and unconditionally returns -EEXIST, does this unintentionally mask legitimate
memory exhaustion errors during tool initialization?

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