Re: [PATCH 07/12] perf jitdump: Fix funlockfile on unlocked stream in jit_open() error path

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

Pre-existing issues:
- [High] Missing validation of `nr_entry` in `JIT_CODE_DEBUG_INFO` records leads to heap out-of-bounds read and write.
- [High] Unvalidated sizes in `JIT_CODE_UNWINDING_INFO` records lead to a heap information leak.
--

--- Patch [7]: [PATCH 07/12] perf jitdump: Fix funlockfile on unlocked stream in jit_open() error path ---
commit 8cca1c1db77a8a564c00747fab840f2d6744e3a9
Author: Arnaldo Carvalho de Melo <[email protected]>

perf jitdump: Fix funlockfile on unlocked stream in jit_open() error path
    
If the malloc() for the initial read buffer fails, jit_open() jumps to
the error label which calls funlockfile(jd->in).  However, flockfile()
is called later in the function, so at this point the stream was never
locked.  Calling funlockfile() on an unlocked stream is undefined
behavior per POSIX.

> diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> index 45f0e21b0e780..ae63366b86c6d 100644
> --- a/tools/perf/util/jitdump.c
> +++ b/tools/perf/util/jitdump.c
> @@ -157,7 +157,7 @@ jit_open(struct jit_buf_desc *jd, const char *name)
>  
>  	buf = malloc(bsz);
>  	if (!buf)
> -		goto error;
> +		goto error_noflock;
>  
>  	/*
>  	 * protect from writer modifying the file while we are reading it
> @@ -246,8 +246,9 @@ jit_open(struct jit_buf_desc *jd, const char *name)
>  
>  	return 0;
>  error:
> -	free(buf);
>  	funlockfile(jd->in);
> +error_noflock:
> +	free(buf);
>  	fclose(jd->in);
>  	return retval;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but is there a missing validation
of nr_entry in JIT_CODE_DEBUG_INFO records?

If jd->needs_bswap is false, nr_entry is not validated against the actual
record size in jit_get_next_entry():

tools/perf/util/jitdump.c:jit_get_next_entry() {
    ...
    if (jd->needs_bswap) {
        void *end = (void *)jr + jr->prefix.total_size;
    ...
}

jit_repipe_debug_info() then blindly copies this unvalidated count into
jd->nr_debug_entries. Finally, jit_process_debug_info() loops
nr_debug_entries times:

tools/perf/util/genelf_debug.c:jit_process_debug_info() {
    ...
    for (i = 0; i < nr_debug_entries; i++) {
        ent->addr = ent->addr - code_addr;
        ent = debug_entry_next(ent);
    }
    ...
}

Could an attacker provide a JIT dump with a small payload but a massive
nr_entry, causing an out-of-bounds strlen read followed by an out-of-bounds
write (ent->addr = ...), corrupting heap memory?

[Severity: High]
This is a pre-existing issue, but are there unvalidated sizes in
JIT_CODE_UNWINDING_INFO records?

When parsing a JIT_CODE_UNWINDING_INFO record, jit_repipe_unwinding_info()
blindly copies the attacker-controlled unwinding_size and eh_frame_hdr_size
fields without checking if they fit within the allocated heap buffer:

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

These unvalidated sizes are later used by jit_add_eh_frame_info() to
configure libelf data sections:

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

Could an attacker craft a record with a small total_size but massive
unwinding sizes, forcing libelf to read past the end of the heap buffer and
write megabytes of the perf inject process's heap memory into the generated
ELF file, causing an information leak?

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