Re: [PATCH v2 perf-tools-next 6/6] perf trace beauty: Validate payload size in augmented perf_event_open beautifier

[sashiko-bot@kernel.org]

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

New issues:
- [High] The newly added bounds check relies on the `augmented_arg->size` field, but the BPF program for `sys_enter_perf_event_open` leaves this field uninitialized, randomly breaking `perf trace` augmentation.

Pre-existing issues:
- [High] The patch fails to validate that the embedded `attr->size` does not exceed the actual augmented payload size, allowing out-of-bounds reads if the tracee mutates the field (TOCTOU) or if a malformed `perf.data` file is parsed.
--

commit f9180137a1c578e9c897496a107baa521c327ae0
Author: Aaron Tomlin <atomlin@atomlin.com>

perf trace beauty: Validate payload size in augmented perf_event_open beautifier

This commit adds bounds checking to the augmented payload of perf_event_open
in perf trace. It attempts to verify that the augmented argument size is large
enough to contain at least PERF_ATTR_SIZE_VER0 bytes before accessing the
perf_event_attr payload.

> diff --git a/tools/perf/trace/beauty/perf_event_open.c b/tools/perf/trace/beauty/perf_event_open.c
> index 6315b46bcdf02..846738225abd7 100644
> --- a/tools/perf/trace/beauty/perf_event_open.c
> +++ b/tools/perf/trace/beauty/perf_event_open.c
> @@ -81,9 +81,18 @@ static size_t perf_event_attr___scnprintf(struct perf_event_attr *attr, char *bf
>  
>  static size_t syscall_arg__scnprintf_augmented_perf_event_attr(struct syscall_arg *arg, char *bf, size_t size)
>  {
> -	struct perf_event_attr *attr = (void *)arg->augmented.args->value;
> +	struct augmented_arg *augmented_arg = arg->augmented.args;
> +	struct perf_event_attr *attr;
>  	struct perf_event_attr local_attr;
>  
> +	if (arg->augmented.size < (int)sizeof(*augmented_arg))
> +		return 0;
> +
> +	if (augmented_arg->size < (int)PERF_ATTR_SIZE_VER0 ||
> +	    augmented_arg->size > arg->augmented.size - (int)sizeof(*augmented_arg))
> +		return 0;

[Severity: High]
Does this newly added bounds check read an uninitialized field?

Looking at sys_enter_perf_event_open() in
tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c, the BPF program reads
the payload and outputs it:

    if (bpf_probe_read_user(&augmented_args->arg.value, size, attr) < 0)
        goto failure;

    return augmented__output(args, augmented_args, len + size);

However, it does not appear to populate augmented_args->arg.size. Since
augmented_args is allocated from a reused per-CPU map, will size contain
junk data from previous syscalls (like an openat length), causing this
check to randomly fail and break the trace augmentation?

> +
> +	attr = (void *)augmented_arg->value;

[Severity: High]
This is a pre-existing issue, but does the newly added bounds check completely
prevent out-of-bounds reads if the tracee mutates the embedded attr->size
field?

While this patch validates that augmented_arg->size fits within the buffer,
attr is eventually passed down to perf_event_attr__fprintf() which trusts
attr->size directly:

    u32 attr_size = attr->size ?: PERF_ATTR_SIZE_VER0;
    ...
    if (attr_size > sizeof(*attr))
        attr_size = sizeof(*attr);

If a malformed perf.data record or a TOCTOU mutation provides an attr->size
that is larger than the actual augmented payload (e.g., 136 bytes while the
buffer only contains PERF_ATTR_SIZE_VER0 bytes), won't the formatter read
past the end of the available buffer and leak adjacent memory into the
trace?

[ ... ]

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