Re: [PATCH 18/28] perf header: Validate f_attr.ids section before use in perf_session__read_header()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The per-attribute caps fail to prevent total memory exhaustion, allowing a small crafted file to reliably trigger an Out-Of-Memory (OOM) panic via overlapping ID sections.
--

commit 19eb906f7ac517fa0070cb7144ca1e4c2646e3ff
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf header: Validate f_attr.ids section before use in perf_session__read_header()

This commit adds validation to the perf.data f_attr.ids section to prevent
integer truncation and heap corruption. It caps the number of attributes and
IDs per attribute, ensures size alignment, and checks that ID sections fit
within the file size.

> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c

[ ... ]

> @@ -4968,6 +5004,45 @@ int perf_session__read_header(struct perf_session *session)
>  			perf_event__attr_swap(&f_attr.attr);
>  		}
>  
> +		/*
> +		 * Validate ids section: must be aligned to u64, and
> +		 * the count must fit in an int to avoid truncation in
> +		 * nr_ids and size_t overflow in perf_evsel__alloc_id()
> +		 * on 32-bit architectures.
> +		 */
> +		if (f_attr.ids.size % sizeof(u64)) {
> +			pr_err("Invalid ids section size %" PRIu64 " for attr %d, not aligned to u64\n",
> +			       f_attr.ids.size, i);
> +			err = -EINVAL;
> +			goto out_delete_evlist;
> +		}
> +
> +		/*
> +		 * Cap the ID count to avoid int truncation of nr_ids
> +		 * on 64-bit and size_t overflow in the allocation
> +		 * paths (nr_ids * sizeof(u64), nr_ids *
> +		 * sizeof(struct perf_sample_id)) on 32-bit.
> +		 */
> +		if (f_attr.ids.size / sizeof(u64) > MAX_IDS_PER_ATTR) {
> +			pr_err("Invalid ids section size %" PRIu64 " for attr %d, too many IDs\n",
> +			       f_attr.ids.size, i);
> +			err = -EINVAL;
> +			goto out_delete_evlist;
> +		}
> +
> +		/*
> +		 * FIXME: see perf_header__process_sections() — block
> +		 * devices bypass this check because st_size is 0.
> +		 */
> +		if (S_ISREG(input_stat.st_mode) &&
> +		    (f_attr.ids.offset > (u64)input_stat.st_size ||
> +		     f_attr.ids.size > (u64)input_stat.st_size - f_attr.ids.offset)) {
> +			pr_err("Invalid ids section for attr %d: offset=%" PRIu64 " size=%" PRIu64 " exceeds file size %" PRIu64 "\n",
> +			       i, f_attr.ids.offset, f_attr.ids.size, (u64)input_stat.st_size);
> +			err = -EINVAL;
> +			goto out_delete_evlist;
> +		}
> +

Does this allow a small crafted file to trigger an out-of-memory panic?

The offset and size checks verify that the section falls within the file,
but there doesn't appear to be a check preventing multiple attributes from
pointing to the exact same offset and size.

Because the aggregate number of IDs across all attributes is not capped,
could a crafted file declare MAX_NR_ATTRS (65,536), all pointing to the same
valid 128MB ID section?

This might cause the subsequent loop to repeatedly allocate and populate memory
for each attribute, forcing the physical pages to be faulted in and potentially
exhausting physical memory.

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