Re: [PATCH 10/12] perf jitdump: Validate debug entries on native (non-swap) path

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

Pre-existing issues:
- [High] Missing NUL-termination validation for the symbol string in JIT_CODE_LOAD records causes out-of-bounds reads via strlen().
- [High] In `jit_repipe_unwinding_info()`, missing validation of `unwinding_size` and `eh_frame_hdr_size` causes an integer underflow and massive out-of-bounds read.
- [Medium] Dereferencing fields of `struct debug_entry` causes undefined behavior and SIGBUS on strict-alignment architectures.
- [Medium] Failing to allocate memory in `jit_repipe_debug_info` causes the next `JIT_CODE_LOAD` to inherit stale debug info.
- [Medium] In `jit_repipe_code_move()`, the synthetic MMAP2 event allocation hardcodes a +16 byte hack for the sample ID header, causing an out-of-bounds heap read.
--

--- Patch [10]: [PATCH 10/12] perf jitdump: Validate debug entries on native (non-swap) path ---
commit 6e2ff5c2d1b7c024e8a5cacbadffafaf75094372
Author: Arnaldo Carvalho de Melo <[email protected]>
Subject: perf jitdump: Validate debug entries on native (non-swap) path

The bounds-checking and nr_entry clamping added for the byte-swap path
only runs when jd->needs_bswap is true.  On native-endian files, nr_entry
passes through unvalidated to jit_repipe_debug_info(), which stores it
as jd->nr_debug_entries.  Downstream, jit_process_debug_info() in
genelf_debug.c iterates nr_debug_entries times via debug_entry_next(),
which calls strlen() on each entry's name field — a crafted nr_entry
causes OOB reads and writes.

Add bounds-checked iteration in jit_repipe_debug_info() that validates
each debug_entry fits in the payload and its name is NUL-terminated
before calling debug_entry_next().  Clamp nr_debug_entries to the count
of valid entries.

> diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> index d3de307532d55..5a3ea2681fb37 100644
> --- a/tools/perf/util/jitdump.c
> +++ b/tools/perf/util/jitdump.c
> @@ -694,8 +694,10 @@ static int jit_repipe_code_move(struct jit_buf_desc *jd, union jr_entry *jr)

[Severity: Medium]
This is a pre-existing issue, but in jit_repipe_code_move(), the synthetic
MMAP2 event allocation hardcodes a +16 byte hack for the sample ID header:

tools/perf/util/jitdump.c:jit_repipe_code_move() {
    ...
    event = calloc(1, sizeof(*event) + 16);
    ...
}

The sample ID header size (idr_size) can be up to 48 or 64 bytes depending
on the perf record configuration.

If the JIT path length is very long, the clamped size pushes
event->mmap2.header.size to exceed the allocated buffer size.

When jit_inject_event() writes the event out:

tools/perf/util/jitdump.c:jit_repipe_code_move() {
    ...
    ret = jit_inject_event(jd, event);
    ...
}

Could this read past the allocated heap block and leak uninitialized heap memory
into perf.data?

>  
>  static int jit_repipe_debug_info(struct jit_buf_desc *jd, union jr_entry *jr)
>  {
> -	void *data;
> -	size_t sz;
> +	struct debug_entry *ent;
> +	void *data, *end;
> +	size_t sz, valid;
> +	uint64_t i;
>  
>  	if (!(jd && jr))
>  		return -1;

[Severity: Medium]
This is a pre-existing issue, but if malloc() fails in jit_repipe_debug_info():

tools/perf/util/jitdump.c:jit_repipe_debug_info() {
    ...
    data = malloc(sz);
    if (!data)
        return -1;
    ...
}

The function returns early without clearing the previously allocated
jd->debug_data.

Since the caller jit_process_dump() ignores this error, will the next
JIT_CODE_LOAD erroneously use the leftover stale debug_data and attach
it to the new code?

> @@ -715,10 +717,25 @@ static int jit_repipe_debug_info(struct jit_buf_desc *jd, union jr_entry *jr)
>  	jd->debug_data       = data;
>  
>  	/*
> -	 * we must use nr_entry instead of size here because
> -	 * we cannot distinguish actual entry from padding otherwise
> +	 * Clamp nr_debug_entries to entries that actually fit in the
> +	 * payload.  The byte-swap path already does this for cross-endian
> +	 * files; validate on the native path too, since downstream
> +	 * jit_process_debug_info() iterates via debug_entry_next() which
> +	 * calls strlen() on each entry's name field.
>  	 */
> -	jd->nr_debug_entries = jr->info.nr_entry;
> +	end = data + sz;
> +	ent = data;
> +	valid = 0;
> +	for (i = 0; i < jr->info.nr_entry; i++) {
> +		if ((void *)ent + sizeof(*ent) > end)
> +			break;
> +		/* name must be NUL-terminated within the payload */
> +		if (!memchr(ent->name, '\0', (char *)end - ent->name))
> +			break;
> +		ent = debug_entry_next(ent);

[Severity: Medium]
This is a pre-existing issue, but debug_entry_next() advances the pointer
without guaranteeing 8-byte alignment.

Later, when unaligned ent pointers are dereferenced to read the uint64_t addr
field, either in the byte-swap path:

tools/perf/util/jitdump.c:jit_get_next_entry() {
    ...
    ent->addr = bswap_64(ent->addr);
    ...
}

or downstream during iteration:

tools/perf/util/genelf_debug.c:jit_process_debug_info() {
    ...
    ent = debug_entry_next(ent);
    ...
}

Will this cause undefined behavior and a SIGBUS crash on architectures that
enforce strict alignment, like SPARC?

> +		valid++;
> +	}
> +	jd->nr_debug_entries = valid;
>  
>  	return 0;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but while processing JIT_CODE_LOAD
records, jit_repipe_code_load() extracts the sym pointer from the payload:

tools/perf/util/jitdump.c:jit_repipe_code_load() {
    ...
    sym = (void *)((unsigned long)jr + sizeof(jr->load));
    ...
}

This extraction happens without verifying that it is NUL-terminated within the
bounds of the available buffer size.

Later, when jit_write_elf() computes the symbol length:

tools/perf/util/genelf.c:jit_write_elf() {
    ...
    symlen = 2 + strlen(sym);
    ...
}

Might this lead to an out-of-bounds read via strlen() if a malformed record
omits the NUL byte?

[Severity: High]
This is a pre-existing issue, but in jit_repipe_unwinding_info(), untrusted
size fields are copied without validation:

tools/perf/util/jitdump.c:jit_repipe_unwinding_info() {
    ...
    jd->eh_frame_hdr_size = jr->unwinding.eh_frame_hdr_size;
    jd->unwinding_size = jr->unwinding.unwinding_size;
    ...
}

If unwinding_header_size is greater than unwinding_size, will the calculation
in jit_add_eh_frame_info() underflow?

tools/perf/util/genelf.c:jit_add_eh_frame_info() {
    ...
    uint64_t unwinding_table_size = unwinding_size - unwinding_header_size;
    ...
}

Can this underflow result in a massive out-of-bounds read when the huge size
is subsequently passed to libelf for the section buffer size?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10