Re: [PATCH 05/12] perf jitdump: Check snprintf return before computing header size

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

Pre-existing issues:
- [High] Insufficient heap allocation in `jit_repipe_code_move` when `idr_size` exceeds 16 bytes causes an out-of-bounds heap read and information leak.
- [High] The dynamically allocated `event` buffer is never freed in `jit_repipe_code_move()`.
- [High] Hardcoded sample ID structure layout causes a heap out-of-bounds write and event corruption when standard layout assumptions are violated.
- [High] Predictable filenames are opened without `O_EXCL` and `O_NOFOLLOW` in a potentially shared directory, enabling a symlink attack to overwrite arbitrary files.
--

--- Patch [5]: [PATCH 05/12] perf jitdump: Check snprintf return before computing header size ---
commit c4d5fd2e9b69d8618378d2b3d3ea73b74c28f945
Author: Arnaldo Carvalho de Melo <[email protected]>

perf jitdump: Check snprintf return before computing header size

This commit clamps the return value of snprintf to PATH_MAX - 1 when computing
the length of a generated JIT filename. This prevents an out-of-bounds heap
read/write caused by size underflow when the jitted filename exceeds PATH_MAX.

> diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> index 078d3304d2b7e..fd11e07bf00b7 100644
> --- a/tools/perf/util/jitdump.c
> +++ b/tools/perf/util/jitdump.c
> @@ -493,6 +493,9 @@ static int jit_repipe_code_load(struct jit_buf_desc *jd, union jr_entry *jr)
>  			jd->dir,
>  			nspid,
>  			count);

[Severity: High]
This is a pre-existing issue, but does this predictable filename creation allow
for a symlink attack? 

The filename is constructed using jd->dir, which could be a world-writable or
attacker-controlled path. When jit_emit_elf() subsequently creates this file,
it omits O_EXCL and O_NOFOLLOW:

    fd = open(filename, O_CREAT|O_TRUNC|O_WRONLY, 0644);

If a local attacker places a symlink in this directory pointing to a sensitive
file like /etc/shadow, could this result in an arbitrary file overwrite when
the file is opened and truncated?

[Severity: High]
This isn't a bug introduced by this patch, but can the sample ID structure
layout assumption result in out-of-bounds heap writes?

In jit_repipe_code_load(), the pointer is mapped to the trailing bytes of the
payload and cast to a hardcoded struct:

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

If the user configures the perf session to omit TID or include other fields
preceding TID, idr_size might only be 8 bytes for time. Writing to id->time
(at offset 8) would then write exactly 8 bytes out of bounds of the allocated
heap space. The identical struct mapping is also repeated later in
jit_repipe_code_move().

> +	/* snprintf returns would-be length on truncation, clamp to buffer */
> +	if (size >= PATH_MAX)
> +		size = PATH_MAX - 1;
>  
>  	size++; /* for \0 */

[ ... ]

> @@ -623,6 +626,9 @@ static int jit_repipe_code_move(struct jit_buf_desc *jd, union jr_entry *jr)
>  	         jd->dir,
>  		 nspid,
>  		 jr->move.code_index);

[Severity: High]
This isn't a bug introduced by this patch, but might the hardcoded buffer
allocation size in jit_repipe_code_move() lead to an out-of-bounds read?

The event buffer is allocated with a fixed 16 bytes for the sample ID:

    event = calloc(1, sizeof(*event) + 16);

However, the header size is computed using the dynamic idr_size:

    event->mmap2.header.size = (sizeof(event->mmap2) -
            (sizeof(event->mmap2.filename) - size) + idr_size);

If idr_size exceeds 16 bytes (for example, if the perf session is configured
with PERF_SAMPLE_TID | PERF_SAMPLE_TIME | PERF_SAMPLE_ID), header.size will
exceed the allocated bounds. When the event is later written in
jit_inject_event():

    size = perf_data__write(jd->output, event, event->header.size);

Could this result in reading past the end of the heap buffer and leaking
sensitive heap contents to the output file?

[Severity: High]
This is a pre-existing issue, but is there a memory leak of the dynamically
allocated event buffer in jit_repipe_code_move()?

The function allocates the event buffer, but appears to return on both success
and error paths without freeing it:

out:
	perf_sample__exit(&sample);
	return ret;

Since perf can process millions of events in a single run, could leaking this
buffer per move event rapidly exhaust system memory and lead to an
Out-Of-Memory crash during standard analysis?

> +	/* snprintf returns would-be length on truncation, clamp to buffer */
> +	if (size >= PATH_MAX)
> +		size = PATH_MAX - 1;
>  
>  	size++; /* for \0 */

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