Re: [PATCH 17/29] perf tools: Bounds check perf_event_attr fields against attr.size before printing

[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 fallback logic in `syscall_arg__scnprintf_augmented_perf_event_attr` blindly copies 64 bytes from the payload, causing an out-of-bounds read if the actual payload is smaller.

Pre-existing issues:
- [High] `perf_event_attr__fprintf()` trusts the user-controlled `attr->size` field for bounds checking without validating it against the actual buffer limits in the `perf trace` path.
--

commit 45122ffcfc4c42d20283944fff0ce0ea20c80919
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf tools: Bounds check perf_event_attr fields against attr.size before printing

perf_event_attr__fprintf() accessed all struct fields unconditionally,
but attrs from older perf.data files or BPF-captured syscall payloads
may have a smaller size than the current struct.  Fields beyond the
recorded size contain uninitialized or zero-filled data.

> diff --git a/tools/perf/trace/beauty/perf_event_open.c b/tools/perf/trace/beauty/perf_event_open.c
> index c1c7445dcff99..6315b46bcdf02 100644
> --- a/tools/perf/trace/beauty/perf_event_open.c
> +++ b/tools/perf/trace/beauty/perf_event_open.c
[ ... ]
> @@ -80,7 +81,27 @@ 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)
>  {
> -	return perf_event_attr___scnprintf((void *)arg->augmented.args->value, bf, size,
> +	struct perf_event_attr *attr = (void *)arg->augmented.args->value;
> +	struct perf_event_attr local_attr;
> +
> +	/*
> +	 * augmented_raw_syscalls.bpf.c (shipped with perf) copies
> +	 * PERF_ATTR_SIZE_VER0 bytes when the tracee passes size=0,
> +	 * but leaves the size field as 0.  The payload size is
> +	 * guaranteed by perf's own BPF program, not externally
> +	 * controllable.  Copy to a local so we can fix up size
> +	 * without writing to the potentially read-only augmented
> +	 * args buffer.
> +	 */
> +	if (!attr->size) {
> +		memcpy(&local_attr, attr, PERF_ATTR_SIZE_VER0);

[Severity: High]
Can this memcpy() read out of bounds if the actual payload is smaller than
PERF_ATTR_SIZE_VER0?

If a tracee uses a TOCTOU race to pass a small payload size (like 8 bytes)
to the first read, but modifies attr->size to 0 before the second read, the
user-space payload might only be 8 bytes long.

Since the BPF program sys_enter_perf_event_open dynamically sizes its output
based on the tracee's attr->size using bpf_perf_event_output(), could blindly
copying 64 bytes here read past the end of the perf ring buffer and cause a
crash?

> +		memset((void *)&local_attr + PERF_ATTR_SIZE_VER0, 0,
> +		       sizeof(local_attr) - PERF_ATTR_SIZE_VER0);
> +		local_attr.size = PERF_ATTR_SIZE_VER0;
> +		attr = &local_attr;
> +	}
> +
> +	return perf_event_attr___scnprintf(attr, bf, size,
>  					   trace__show_zeros(arg->trace));
>  }
[ ... ]
> diff --git a/tools/perf/util/perf_event_attr_fprintf.c b/tools/perf/util/perf_event_attr_fprintf.c
> index 741c3d657a8b6..3933639d76c54 100644
> --- a/tools/perf/util/perf_event_attr_fprintf.c
> +++ b/tools/perf/util/perf_event_attr_fprintf.c
[ ... ]
> @@ -275,24 +275,56 @@ static void __p_config_id(struct perf_pmu *pmu, char *buf, size_t size, u32 type
[ ... ]
>  int perf_event_attr__fprintf(FILE *fp, struct perf_event_attr *attr,
>  			     attr__fprintf_f attr__fprintf, void *priv)
>  {
>  	struct perf_pmu *pmu = perf_pmus__find_by_type(attr->type);
> +	/*
> +	 * size == 0 means ABI0 — the producer didn't set attr.size.
> +	 * perf_event__fprintf_attr() may pass the raw mmap'd event
> +	 * before the local copy, so default to PERF_ATTR_SIZE_VER0
> +	 * (the ABI0 footprint) to avoid reading past the attr into
> +	 * the ID array that follows it in HEADER_ATTR events.
> +	 */
> +	u32 attr_size = attr->size ?: PERF_ATTR_SIZE_VER0;
>  	char buf[BUF_SIZE];
>  	int ret = 0;
>  
> -	if (!pmu && (attr->type == PERF_TYPE_HARDWARE || attr->type == PERF_TYPE_HW_CACHE)) {
> +	/*
> +	 * Cap to what we understand: all callers store the attr in a
> +	 * buffer of sizeof(*attr) bytes (perf.data read path copies
> +	 * min(attr.size, sizeof), BPF augmented path copies into a
> +	 * fixed-size value[] array).  A spoofed attr->size larger
> +	 * than sizeof would cause PRINT_ATTRn to read past the
> +	 * actual buffer.
> +	 */
> +	if (attr_size > sizeof(*attr))
> +		attr_size = sizeof(*attr);

[Severity: High]
This is a pre-existing issue, but does capping attr_size to sizeof(*attr)
fully prevent out-of-bounds reads in the perf trace path?

The comment mentions the BPF augmented path copies into a fixed-size value[]
array, but augmented__output actually trims the emitted ring buffer event to
exactly the copied size (len + size).

If a tracee uses a TOCTOU race to supply a small payload while spoofing
attr->size to 136, this function will use 136 as the size. Will the PRINT_ATTRn
macros then read up to 136 bytes from a much smaller payload, resulting in
an out-of-bounds read?

> +
> +	if (!pmu && attr_size >= offsetof(struct perf_event_attr, config) + sizeof(attr->config) &&
> +	    (attr->type == PERF_TYPE_HARDWARE || attr->type == PERF_TYPE_HW_CACHE)) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260524032709.1080771-1-acme@kernel.org?part=17