Re: [PATCH 09/12] perf jitdump: Use dirname() return value in jit_open()

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

Pre-existing issues:
- [High] Missing bounds checking and validation of JIT_CODE_DEBUG_INFO when architecture endianness matches.
- [High] Missing validation of unwinding record sizes allows heap information leaks into generated ELF files.
- [High] Missing NUL-termination check for symbol name in JIT_CODE_LOAD.
--

--- Patch [9]: [PATCH 09/12] perf jitdump: Use dirname() return value in jit_open() ---
commit b82fdc2ff8228d697093005adb380338825bb2dd
Author: Arnaldo Carvalho de Melo <[email protected]>

perf jitdump: Use dirname() return value in jit_open()
    
jit_open() calls dirname(jd->dir) but ignores the return value.  POSIX
says dirname() may return a pointer to internal static storage — glibc
does this when the path has no '/', returning "." from a static buffer
and leaving jd->dir unchanged with the original filename.

> diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> index 91aa1eea8229f..d3de307532d55 100644
> --- a/tools/perf/util/jitdump.c
> +++ b/tools/perf/util/jitdump.c
> @@ -146,6 +146,7 @@ jit_open(struct jit_buf_desc *jd, const char *name)
>  	ssize_t bs, bsz = 0;
>  	void *n, *buf = NULL;
>  	int ret, retval = -1;
> +	char *dname;
>  
>  	nsinfo__mountns_enter(jd->nsi, &nsc);
>  	jd->in = fopen(name, "r");

[ ... ]

> @@ -241,7 +242,9 @@ jit_open(struct jit_buf_desc *jd, const char *name)
>  	 */
>  	strncpy(jd->dir, name, PATH_MAX - 1);
>  	jd->dir[PATH_MAX - 1] = '\0';
> -	dirname(jd->dir);
> +	dname = dirname(jd->dir);
> +	if (dname != jd->dir)
> +		strlcpy(jd->dir, dname, sizeof(jd->dir));
>  	free(buf);
>  
>  	return 0;

[Severity: High]
This is a pre-existing issue, but is it possible to corrupt memory when
processing JIT_CODE_DEBUG_INFO?

In tools/perf/util/jitdump.c:jit_get_next_entry(), the bounds-checking
logic is only executed if the host and file endianness differ:

    case JIT_CODE_DEBUG_INFO:
        if (jd->needs_bswap) { ... }

If they share the same endianness, this check is skipped. Then, in
tools/perf/util/genelf_debug.c:jit_process_debug_info(), the code iterates
blindly using the untrusted nr_debug_entries:

    for (i = 0; i < nr_debug_entries; i++) {
        ent->addr = ent->addr - code_addr;
        ent = debug_entry_next(ent);
    }

Could a maliciously crafted jitdump file with an artificially large
nr_entry cause an out-of-bounds heap write here?

[Severity: High]
This is a pre-existing issue, but does this code leak heap information
into the generated ELF files?

In tools/perf/util/jitdump.c:jit_repipe_unwinding_info(), internal
structure sizes are read without validating them against the record
payload length:

    jd->eh_frame_hdr_size = jr->unwinding.eh_frame_hdr_size;
    jd->unwinding_size = jr->unwinding.unwinding_size;

Later, in tools/perf/util/genelf.c:jit_add_eh_frame_info(), these
unvalidated sizes are used to compute offsets:

    uint64_t unwinding_table_size = unwinding_size - unwinding_header_size;
    ...
    d->d_buf = unwinding + unwinding_table_size;

If unwinding_size exceeds the payload, could this instruct libelf to read
adjacent heap memory? Furthermore, if unwinding_size is less than
unwinding_header_size, could unwinding_table_size underflow and cause
out-of-bounds pointer arithmetic?

[Severity: High]
This is a pre-existing issue, but is there a missing NUL-termination check
for the symbol name?

In tools/perf/util/jitdump.c:jit_repipe_code_load(), the string pointer is
calculated directly from the payload:

    sym   = (void *)((unsigned long)jr + sizeof(jr->load));

When this is passed to tools/perf/util/genelf.c:jit_write_elf(), it
calculates the length:

    symlen = 2 + strlen(sym);

If the symbol name payload lacks a NUL terminator, will strlen() read past
the end of the heap allocation and crash the process?

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