Re: [RFC PATCH v1 6/8] perf inject: Extend perf inject to support bid_offset conversion

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Integer overflow in `u16 max_size` calculation bypasses bounds check, leading to a massive heap buffer overflow.
- [High] Missing `inject_bid_offset_tool__new` instantiation and fatal invalid pointer free on error path.
- [High] Unused attribute rewriting functions lead to complete data corruption in output `perf.data`.
- [High] Raw event copying incorrectly checks `branch_sample_type` flags against the `sample_type` bitmask, aliasing with other flags.
- [High] Incorrect event boundary calculations allow the tool to read out-of-bounds memory up to 16 bytes past the event buffer.
- [High] Missing curly braces around `PERF_SAMPLE_READ` format condition checks leads to unconditional array execution, corrupting the sample payload.
- [Medium] Missing explicit inclusion of POSIX standard libc headers `<errno.h>` and `<string.h>` breaks musl compatibility.
--

--- Patch [6]: [RFC PATCH v1 6/8] perf inject: Extend perf inject to support bid_offset conversion ---
commit 00dac7cb64934c318662c48215a38d4a8f9ec625
Author: Ian Rogers <irogers@google.com>

perf inject: Extend perf inject to support bid_offset conversion

This patch adds a --sample-buildids option to perf inject. It drops MMAP
events and rewrites samples to use build IDs and offsets instead of virtual
addresses.

> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index 6aa9e3eea4383..8f8d5267a37e2 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
[ ... ]
> @@ -2880,8 +2889,11 @@ int cmd_inject(int argc, const char **argv)
>  
>  	if (IS_ERR(inject.session)) {
>  		ret = PTR_ERR(inject.session);
> +
>  		if (inject.aslr)
>  			aslr_tool__delete(tool);
> +		if (inject.build_id_style == BID_RWS__SAMPLE_BUILDID)
> +			inject_bid_offset_tool__delete(tool);

[Severity: High]
Does this code call inject_bid_offset_tool__delete() on an invalid pointer
if __perf_session__new() fails? It appears inject_bid_offset_tool__new() is
never called to instantiate the tool during setup, so when running with
--sample-buildids, this error path will use container_of() and free() on an
uninitialized or incorrect pointer, crashing the tool.

>  		goto out_close_output;
>  	}
>  
> diff --git a/tools/perf/util/inject_bid_offset.c b/tools/perf/util/inject_bid_offset.c
> new file mode 100644
> index 0000000000000..f7b5fb05fac79
> --- /dev/null
> +++ b/tools/perf/util/inject_bid_offset.c
> @@ -0,0 +1,504 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include "inject_bid_offset.h"
> +
> +#include <stdlib.h>
> +
> +#include <linux/compiler.h>
> +#include <linux/string.h>

[Severity: Medium]
Will this file fail to build against musl libc? According to the POSIX
standard, files using POSIX constants like -E2BIG, -EFAULT and functions
like memcpy() need to explicitly include <errno.h> and <string.h>.

> +#include <linux/zalloc.h>
[ ... ]
> +int perf_event__rewrite_attr_for_build_id_offset(struct perf_event_attr *attr)
> +{

[Severity: High]
Is this attribute rewriting function dead code? It seems to be defined but
never called. Since inject_bid_offset_tool__sample() strips PERF_SAMPLE_IP
and appends PERF_SAMPLE_BUILD_ID_OFFSET, leaving the event attributes
unmodified will cause downstream tools like perf report to parse the
rewritten build-id payloads using the old schema, corrupting the data.

> +	if (attr->sample_type & (PERF_SAMPLE_BUILD_ID_OFFSET |
> +				 PERF_SAMPLE_CALLCHAIN_BUILD_ID_OFFSET)) {
[ ... ]
> +static int inject_bid_offset_tool__sample(const struct perf_tool *tool,
> +					  union perf_event *event,
> +					  struct perf_sample *sample,
> +					  struct machine *machine)
> +{
[ ... ]
> +	__u64 i = 0, j = 0;
> +	__u64 *in_array, *out_array;
> +	__u64 sample_type = evsel->core.attr.sample_type;
> +	const __u64 max_i = event->header.size / sizeof(__u64);
> +	struct thread *thread;
> +	u16 max_size = event->header.size;

[Severity: Critical]
Can this variable overflow when adding the callchain size below? Because
max_size is typed as u16, when processing an event with a large
sample->callchain->nr, adding the size could cause it to wrap around.

> +
> +	if ((sample_type & (PERF_SAMPLE_IP | PERF_SAMPLE_CALLCHAIN)) == 0)
[ ... ]
> +	if (sample_type & PERF_SAMPLE_CALLCHAIN) {
> +		max_size +=
> +			sample->callchain->nr * (sizeof(struct perf_build_id) +
> +						 sizeof(u64) - sizeof(u64));
> +	}
> +
> +	if (max_size > PERF_SAMPLE_MAX_SIZE) {

[Severity: Critical]
If max_size wraps around, will this bounds check (against 65536) bypass the
protection, leading to a massive out-of-bounds write later in the loop when
writing the unvalidated payload?

> +		pr_debug("Insufficient space to copy event\n");
> +		thread__put(thread);
> +		return -E2BIG;
> +	}
[ ... ]
> +	in_array = &event->sample.array[0];
> +	out_array = &ev->sample.array[0];
> +
> +	if (sample_type & PERF_SAMPLE_IDENTIFIER) {
> +		if (i > max_i)
> +			goto err;

[Severity: High]
Does this boundary calculation allow reading past the event buffer? max_i
represents the total number of 64-bit words including the 8-byte header,
but in_array points to the array skipping the header. Checking against
max_i loosely allows index i to read out of bounds.

> +		out_array[j++] = in_array[i++];
> +	}
[ ... ]
> +	if (sample_type & PERF_SAMPLE_READ) {
> +		if ((evsel->core.attr.read_format & PERF_FORMAT_GROUP) == 0) {
> +			if (i > max_i)
> +				goto err;
> +			out_array[j++] = in_array[i++];
> +			if (evsel->core.attr.read_format &
> +			    PERF_FORMAT_TOTAL_TIME_ENABLED)
> +				if (i > max_i)
> +					goto err;
> +			out_array[j++] = in_array[i++];

[Severity: High]
Will this execute unconditionally? Because of the missing curly braces
around the condition check, it looks like only the if (i > max_i) goto
err; check is conditional on PERF_FORMAT_TOTAL_TIME_ENABLED, causing the
out_array assignment to always execute and misalign all subsequent fields
if the flag is absent.

> +			if (evsel->core.attr.read_format &
> +			    PERF_FORMAT_TOTAL_TIME_RUNNING)
> +				if (i > max_i)
> +					goto err;
> +			out_array[j++] = in_array[i++];
[ ... ]
> +		} else {
> +			u64 nr;
[ ... ]
> +			for (u64 cntr = 0; cntr < nr; cntr++) {
> +				if (i > max_i)
> +					goto err;
> +				out_array[j++] = in_array[i++];
> +				if (evsel->core.attr.read_format &
> +				    PERF_FORMAT_ID)
> +					if (i > max_i)
> +						goto err;
> +				out_array[j++] = in_array[i++];

[Severity: High]
Are there missing curly braces here as well? The array copy appears to
execute unconditionally, potentially corrupting the event if
PERF_FORMAT_ID is absent.

[ ... ]
> +	if (sample_type & PERF_SAMPLE_BRANCH_STACK) {
> +		if (i > max_i)
> +			goto err;
> +		out_array[j++] = in_array[i++];
> +		if (sample_type & PERF_SAMPLE_BRANCH_HW_INDEX) {

[Severity: High]
Does this code check the wrong bitmask? PERF_SAMPLE_BRANCH_HW_INDEX belongs
to branch_sample_type, not sample_type. Checking it against sample_type
means it aliases with PERF_SAMPLE_TRANSACTION, which might incorrectly
consume fields and corrupt the payload for unrelated events.

> +			if (i > max_i)
> +				goto err;
> +			out_array[j++] = in_array[i++];
> +		}
> +		if (i + (sample->branch_stack->nr * 3) > max_i)
> +			goto err;
> +		memcpy(&out_array[j], &in_array[i],
> +		       sample->branch_stack->nr * 3 * sizeof(u64));
> +		i += sample->branch_stack->nr * 3;
> +		j += sample->branch_stack->nr * 3;
> +		if (sample_type & PERF_SAMPLE_BRANCH_COUNTERS) {

[Severity: High]
Similarly, does this alias with PERF_SAMPLE_PHYS_ADDR since
PERF_SAMPLE_BRANCH_COUNTERS is a branch_sample_type flag, rather than a
sample_type flag?

> +			if (i + sample->branch_stack->nr > max_i)
> +				goto err;
[ ... ]

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