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

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

Pre-existing issues:
- [High] The JVMTI agent and debug_entry array iterates over tightly packed, variable-length structures without padding, causing unaligned memory accesses when reading or writing 64-bit and 32-bit fields.
- [High] The functions `jit_get_next_entry()` and `jit_repipe_code_move()` lack bounds checking to ensure that the parsed `JIT_CODE_MOVE` record is at least `sizeof(struct jr_code_move)` bytes in length.
- [High] The arguments `unwinding_header_size` and `unwinding_size` are silently truncated from 64-bit to 32-bit before being passed to `jit_write_elf()`.
--

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

perf jitdump: Validate debug entries on native (non-swap) path

This commit fixes an out-of-bounds read and write on native-endian files
by adding bounds checking and NUL-termination validation for debug
entries in jit_repipe_debug_info(). It clamps nr_debug_entries to the
count of valid entries that fit in the payload.

> diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> index 02840dbf8a1fc..87612ef3e2325 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: High]
This isn't a bug introduced by this patch, but does jit_repipe_code_move()
lack bounds checking for the JIT_CODE_MOVE record size?

If a crafted JIT dump provides a JIT_CODE_MOVE record with a total_size
smaller than sizeof(struct jr_code_move), jit_get_next_entry() might only
read the provided smaller size into jd->buf.

Could extracting fields like jr->move.new_code_addr in jit_repipe_code_move()
then read out-of-bounds heap memory and leak stale data into the generated
perf.data file?

>  
>  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;

[ ... ]

> @@ -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: High]
This is a pre-existing issue, but does advancing the pointer this way lead
to unaligned memory accesses?

Looking at tools/perf/util/jitdump.h, debug_entry_next() computes the next
pointer without enforcing 8-byte alignment:

static inline struct debug_entry *
debug_entry_next(struct debug_entry *ent)
{
	void *a = ent + 1;
	size_t l = strlen(ent->name) + 1;
	return a + l;
}

Since the variable-length name string is not padded, the returned pointer
might not be 64-bit aligned. 

When jit_process_debug_info() or jit_get_next_entry() subsequently accesses
ent->addr (a 64-bit integer), could this trigger a SIGBUS crash on
architectures that enforce strict alignment?

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

[Severity: High]
This is also a pre-existing issue, but does jit_emit_elf() silently truncate
64-bit size values?

In tools/perf/util/jitdump.c, jit_repipe_code_load() passes the 64-bit fields
jd->eh_frame_hdr_size and jd->unwinding_size to jit_emit_elf():

ret = jit_emit_elf(jd, filename, sym, addr, (const void *)uaddr, csize,
                   jd->debug_data, jd->nr_debug_entries,
                   jd->unwinding_data, jd->eh_frame_hdr_size,
                   jd->unwinding_size);

However, jit_emit_elf() defines these parameters as 32-bit integers:

void *unwinding,
uint32_t unwinding_header_size,
uint32_t unwinding_size)

If a crafted JIT dump provides 64-bit sizes that are truncated when passed
to jit_emit_elf(), could this result in an unwinding_size smaller than
unwinding_header_size?

When jit_write_elf() widens these back to 64-bit and subtracts them, could
this underflow the unwinding_table_size and cause massive out-of-bounds
operations during ELF generation?

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