perf header: Validate feature section size and add read path bounds checking

Harden feature section parsing against crafted perf.data files:

1. perf_header__process_sections() reads the feature section table
   and passes each section's offset and size directly to the
   processing callbacks without validating them against the actual
   file size.  A crafted section size would make all downstream
   bounds checks against ff->size ineffective since they compare
   against the untrusted, inflated bound.  Add an fstat() check
   with S_ISREG() guard and verify that each section's offset +
   size does not extend past EOF.

2. __do_read_buf() validates reads against ff->size (section size),
   but __do_read_fd() had no such check, so a malformed perf.data
   with an understated section size could cause reads past the end
   of the current section into the next section's data.  Add the
   bounds check in __do_read(), the common caller of both helpers,
   so it is enforced uniformly for both the fd and buf paths.
   Track the section-relative offset in __do_read_fd() so the
   check works for the fd path.  Reject negative sizes which on
   32-bit can occur when a u32 >= 0x80000000 is passed as ssize_t.

3. do_read_string() relied on file data being null-padded.  Add
   explicit null-termination (buf[len-1] = '\0') after reading
   and validate length (>= 1, fits within section) before
   allocating, so callers like process_cpu_topology() never
   receive an unterminated string.

4. Initialize feat_fd.offset to 0 (section-relative) instead of
   section->offset (file-absolute) so the bounds tracking is
   consistent with __do_read()'s section-relative comparison.
   Adjust process_build_id() to use lseek() for its file-absolute
   offset needs since it cannot rely on ff->offset for that.

5. Propagate ff->size to perf_file_section__fprintf_info() so its
   reads are also bounded.

Reported-by: sashiko-bot@kernel.org # Running on a local machine
Reviewed-by: Ian Rogers <irogers@google.com>
Cc: David Carrillo-Cisneros <davidcc@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Assisted-by: Claude:claude-opus-4.6-1m
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
This commit is contained in:
Arnaldo Carvalho de Melo 2026-05-02 14:32:58 -03:00
parent 7685cc0f2e
commit b8deecd753

View File

@ -233,23 +233,32 @@ static int __do_read_fd(struct feat_fd *ff, void *addr, ssize_t size)
if (ret != size)
return ret < 0 ? (int)ret : -1;
ff->offset += size;
return 0;
}
static int __do_read_buf(struct feat_fd *ff, void *addr, ssize_t size)
{
if (size > (ssize_t)ff->size - ff->offset)
return -1;
memcpy(addr, ff->buf + ff->offset, size);
ff->offset += size;
return 0;
}
static int __do_read(struct feat_fd *ff, void *addr, ssize_t size)
{
/*
* Reject negative sizes, which on 32-bit can occur when a
* u32 >= 0x80000000 is passed as ssize_t. The cast to
* ssize_t is safe because perf_header__process_sections()
* validates that each section fits within the file size
* before any feature callback reaches here, and only
* feature sections (metadata like build IDs, topology, etc.)
* use this path these cannot legitimately approach 2GB.
*/
if (size < 0 || size > (ssize_t)ff->size - ff->offset)
return -1;
if (!ff->buf)
return __do_read_fd(ff, addr, size);
return __do_read_buf(ff, addr, size);
@ -289,16 +298,25 @@ static char *do_read_string(struct feat_fd *ff)
if (do_read_u32(ff, &len))
return NULL;
/* At least the null terminator. */
if (len < 1 || len > ff->size - ff->offset) {
pr_debug("do_read_string: invalid length %u (remaining %zu)\n",
len, (size_t)(ff->size - ff->offset));
return NULL;
}
buf = malloc(len);
if (!buf)
return NULL;
if (!__do_read(ff, buf, len)) {
/*
* strings are padded by zeroes
* thus the actual strlen of buf
* may be less than len
* do_write_string() writes len including the null
* terminator, padded to NAME_ALIGN. Ensure the
* string is always null-terminated even if the file
* data has been tampered with.
*/
buf[len - 1] = '\0';
return buf;
}
@ -2775,7 +2793,13 @@ static int process_tracing_data(struct feat_fd *ff __maybe_unused, void *data __
static int process_build_id(struct feat_fd *ff, void *data __maybe_unused)
{
if (perf_header__read_build_ids(ff->ph, ff->fd, ff->offset, ff->size))
/* lseek fails in pipe mode — fall back to ff->offset */
off_t offset = lseek(ff->fd, 0, SEEK_CUR);
if (offset == (off_t)-1)
offset = ff->offset;
if (perf_header__read_build_ids(ff->ph, ff->fd, offset, ff->size))
pr_debug("Failed to read buildids, continuing...\n");
return 0;
}
@ -4152,6 +4176,7 @@ static int perf_file_section__fprintf_info(struct perf_file_section *section,
ff = (struct feat_fd) {
.fd = fd,
.ph = ph,
.size = section->size,
};
if (!feat_ops[feat].full_only || hd->full)
@ -4512,6 +4537,7 @@ int perf_header__process_sections(struct perf_header *header, int fd,
int sec_size;
int feat;
int err;
struct stat st;
nr_sections = bitmap_weight(header->adds_features, HEADER_FEAT_BITS);
if (!nr_sections)
@ -4529,7 +4555,29 @@ int perf_header__process_sections(struct perf_header *header, int fd,
if (err < 0)
goto out_free;
if (fstat(fd, &st) < 0) {
pr_err("Failed to stat the perf data file\n");
err = -1;
goto out_free;
}
for_each_set_bit(feat, header->adds_features, header->last_feat) {
/*
* FIXME: block devices have st_size == 0, so we skip
* bounds checking entirely. Historically perf never
* prevented using a block device as input, but it
* probably should there's no valid use case for it
* and it bypasses all file-size validation.
*/
if (S_ISREG(st.st_mode) &&
(sec->offset > (u64)st.st_size ||
sec->size > (u64)st.st_size - sec->offset)) {
pr_err("Feature %s (%d) section extends past EOF (offset=%" PRIu64 ", size=%" PRIu64 ", file=%" PRIu64 ")\n",
header_feat__name(feat), feat,
sec->offset, sec->size, (u64)st.st_size);
err = -1;
goto out_free;
}
err = process(sec++, header, feat, fd, data);
if (err < 0)
goto out_free;
@ -4756,7 +4804,7 @@ static int perf_file_section__process(struct perf_file_section *section,
.fd = fd,
.ph = ph,
.size = section->size,
.offset = section->offset,
.offset = 0,
};
if (lseek(fd, section->offset, SEEK_SET) == (off_t)-1) {