Re: [PATCH 01/12] perf jitdump: Fix extended header read that always fails

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing size validation in `jit_get_next_entry` allows out-of-bounds heap access and arbitrary memory exfiltration.
- [High] Memory leak of the `event` structure in `jit_repipe_code_move`.
- [High] Memory leak of `debug_data` and `unwinding_data` buffers.
- [Medium] Undefined behavior from calling `funlockfile()` on an unlocked stream during early initialization error paths.
- [Low] Ineffective use of `flockfile()` for cross-process file synchronization.
--

--- Patch [1]: [PATCH 01/12] perf jitdump: Fix extended header read that always fails ---
commit 9c0b715100e3938e32e2120f1ac2090e75d6c0a1
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf jitdump: Fix extended header read that always fails

This commit fixes an issue where jit_open() sets bsz = bs before the
fread() that uses bs - bsz as the read size, making the read size zero.
It separates the buffer growth from the read to ensure extended header
bytes are correctly consumed.

> diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> index 83005b30b9bf3..4b7c7ba7cd95d 100644
> --- a/tools/perf/util/jitdump.c
> +++ b/tools/perf/util/jitdump.c
> @@ -224,10 +224,12 @@ jit_open(struct jit_buf_desc *jd, const char *name)
>  		n = realloc(buf, bs);
>  		if (!n)
>  			goto error;
> -		bsz = bs;
>  		buf = n;
> -		/* read extra we do not know about */
> -		ret = fread(buf, bs - bsz, 1, jd->in);
> +		bsz = bs;
> +	}
> +	if (bs > 0) {
> +		/* consume extended header bytes from the stream */
> +		ret = fread(buf, bs, 1, jd->in);
>  		if (ret != 1)
>  			goto error;
>  	}

[Severity: Medium]
This is a pre-existing issue, but I noticed a potential problem in the error
handling earlier in jit_open().

When the initial buffer allocation fails:

buf = malloc(bsz);
if (!buf)
    goto error;

/*
 * protect from writer modifying the file while we are reading it
 */
flockfile(jd->in);
// ...
error:
    free(buf);
    funlockfile(jd->in);

Does this cause undefined behavior by calling funlockfile() on a stream that
was never locked by flockfile()?

[Severity: Low]
This isn't a bug introduced by this patch, but is flockfile() providing the
intended synchronization here?

/*
 * protect from writer modifying the file while we are reading it
 */
flockfile(jd->in);

The comment mentions protecting against a writer modifying the file while
reading, but flockfile() only provides thread-level synchronization within the
same process. It doesn't lock the file descriptor at the OS level against a
separate JIT compiler process.

Might a file-level lock be needed instead?

[Severity: High]
This is a pre-existing issue, but could we have an out-of-bounds heap access
when reading the record prefix?

In jit_get_next_entry(), the bounds check only verifies the prefix size:

bs = (size_t)size;
if (bs < sizeof(*prefix))
    return NULL;

If a malformed file provides a prefix->total_size smaller than the specific
record struct (e.g., 17 bytes instead of the 56 bytes needed for
struct jr_code_load), the fread() will succeed but leave stale data.

Later, functions like jit_repipe_code_load() use this stale heap data:

csize = jr->load.code_size;
// ...
code  = (unsigned long)jr + jr->load.p.total_size - csize;

Can this result in an out-of-bounds pointer calculation and arbitrary memory
being exfiltrated to the output ELF file?

[Severity: High]
This is a pre-existing issue, but does jit_repipe_code_move() leak the event
structure?

event = calloc(1, sizeof(*event) + 16);
if (!event)
    return -1;
// ...
out:
    perf_sample__exit(&sample);
    return ret;

Unlike other repipe functions that free the event, the event pointer here
appears to go out of scope without being freed, potentially causing the tool
to OOM on large dumps with many JIT_CODE_MOVE records.

[Severity: High]
This is a pre-existing issue, but is there a memory leak when processing debug
info records?

In jit_repipe_debug_info(), memory is allocated and assigned unconditionally:

data = malloc(sz);
if (!data)
    return -1;

memcpy(data, &jr->info.entries, sz);

jd->debug_data = data;

This replaces jd->debug_data without freeing any previously held allocation.

Additionally, in jit_process():

nsinfo__put(jd.nsi);
free(jd.buf);

return ret;

It looks like jd.debug_data and jd.unwinding_data are not cleaned up when
processing finishes. Should these buffers be freed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805133013.235016-1-acme@kernel.org?part=1