Re: [PATCH v6 6/6] perf aslr: Strip sample registers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Stripping register feature bits from event attributes while retaining the ABI word in the payload permanently misaligns downstream parsers.
- [High] In pipe mode, dynamically clearing register bits from ATTR events causes `perf inject` to misinterpret incoming raw SAMPLE events, leading to out-of-bounds reads and silent sample drops.
--

commit 2f6701e115da3f7f7833ab4467a9c51f40012620
Author: Ian Rogers <irogers@google.com>
perf aslr: Strip sample registers

This commit modifies the perf ASLR tool to strip register dump payload words
from sample events instead of dropping the entire sample. It also strips the
corresponding sample bit flags from the event attributes to keep downstream
parsers synchronized.

> diff --git a/tools/perf/util/aslr.c b/tools/perf/util/aslr.c
> index 09b7f2f8fb850..e5369589a7338 100644
> --- a/tools/perf/util/aslr.c
> +++ b/tools/perf/util/aslr.c
> @@ -751,18 +751,13 @@ static int aslr_tool__process_sample(const struct perf_tool *tool, union perf_ev
>  		if (abi != PERF_SAMPLE_REGS_ABI_NONE) {
>  			u64 nr = hweight64(evsel->core.attr.sample_regs_user);
>  
> -			if (nr > max_i - i || nr > max_j - j) {
> +			if (nr > max_i - i) {
>  				ret = -EFAULT;
>  				goto out_put;
>  			}
> -			memcpy(&out_array[j], &in_array[i], nr * sizeof(u64));
>  			i += nr;
> -			j += nr;
> +			out_array[j-1] = PERF_SAMPLE_REGS_ABI_NONE;

Since the PERF_SAMPLE_REGS_USER bit is completely cleared from the event's
sample_type bitmask, downstream parsers will not expect an ABI word and will
not advance their read pointer.

Does leaving the 8-byte ABI word in out_array permanently misalign downstream
parsers? It seems they might misinterpret this leftover word as the data for
the next field present in the bitmask sequence, shifting all subsequent sample
fields.

>  		}

[ ... ]

> @@ -907,6 +897,15 @@ 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;
> +	}
> +

In pipe mode, ATTR events arrive dynamically before SAMPLE events. If we
intercept these ATTR events and strip the register bits from the session's
internal evsel, will aslr_tool__process_sample() still be able to parse
incoming raw SAMPLE events correctly?

Because the bits would be missing from evsel->core.attr.sample_type, it
looks like the parser will skip the register parsing logic and fail to
advance its read pointer over the ABI word and register dump that are actually
present in the raw input payload. This could cause it to read subsequent
fields from the middle of the register dump.

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

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