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 <[email protected]>
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/[email protected]?part=6
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.