Re: [PATCH 11/12] perf jitdump: Validate sym string NUL-termination in code load

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

Pre-existing issues:
- [Critical] Heap Buffer Overflow / Out-of-bounds Write in JIT event synthesis due to hardcoded sample ID struct.
- [High] Out-of-bounds Read / Information Leak in `jit_repipe_code_move` due to fixed 16-byte sample ID allocation.
--

--- Patch [11]: [PATCH 11/12] perf jitdump: Validate sym string NUL-termination in code load ---
commit 6b4b582de49c92ce6528fb28a944ec4bfb31b834
Author: Arnaldo Carvalho de Melo <[email protected]>

perf jitdump: Validate sym string NUL-termination in code load

jit_repipe_code_load() computes sym = (void *)jr + sizeof(jr->load) and
passes it to jit_emit_elf() which calls strlen(sym) via jit_write_elf().
If code_size equals total_size - sizeof(jr->load), the sym pointer
aliases the code blob with no NUL terminator, and strlen() scans past
the buffer into adjacent heap memory.

> diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> index 5a3ea2681fb37..5898a7d8eb962 100644
> --- a/tools/perf/util/jitdump.c
> +++ b/tools/perf/util/jitdump.c
> @@ -487,6 +487,13 @@ static int jit_repipe_code_load(struct jit_buf_desc *jd, union jr_entry *jr)
>  
>  	sym   = (void *)((unsigned long)jr + sizeof(jr->load));
>  	code  = (unsigned long)jr + jr->load.p.total_size - csize;
> +
> +	/* sym string lives between the load header and the code blob */
> +	if (!memchr(sym, '\0', code - (unsigned long)sym)) {
> +		pr_warning("jitdump: unterminated symbol name in code_load record\n");
> +		return -1;
> +	}
> +
>  	count = jr->load.code_index;
>  	idr_size = jd->machine->id_hdr_size;
>  

[Severity: Critical]
This isn't a bug introduced by this patch, but I noticed a potential
out-of-bounds write later in jit_repipe_code_load() and
jit_repipe_code_move() when setting up the sample ID:

tools/perf/util/jitdump.c:jit_repipe_code_load() {
    ...
    id = (void *)((unsigned long)event + event->mmap.header.size - idr_size);
    if (jd->sample_type & PERF_SAMPLE_TID) {
        id->pid  = pid;
        id->tid  = tid;
    }
    if (jd->sample_type & PERF_SAMPLE_TIME)
        id->time = convert_timestamp(jd, jr->load.p.timestamp);
    ...
}

The code casts the end of the event payload to a hardcoded struct layout.
If the session is recorded with PERF_SAMPLE_TIME but without PERF_SAMPLE_TID,
idr_size is 8 bytes.

The id pointer is placed 8 bytes before the end of the payload size, but
writing to id->time (at offset 8) will write exactly 8 bytes past the end of
the buffer.

Could this corrupt heap memory if the original filename is long enough to
maximize the event buffer?

[Severity: High]
This is also a pre-existing issue, but could this calculation in
jit_repipe_code_move() lead to an out-of-bounds read?

tools/perf/util/jitdump.c:jit_repipe_code_move() {
    ...
    event = calloc(1, sizeof(*event) + 16);
    ...
    event->mmap2.header.size = (sizeof(event->mmap2) -
            (sizeof(event->mmap2.filename) - size) + idr_size);
    ...
}

The allocation hardcodes 16 bytes for the sample ID payload, but header.size
incorporates idr_size.

If the session was recorded with multiple sample attributes, idr_size can
exceed 16 bytes.

If the calculated header.size exceeds the allocated heap size, will
perf_data__write() read past the end of the buffer?

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