Re: [PATCH 3/5] perf jitdump: Free unwinding data even when eh_frame_hdr_size is zero

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

Pre-existing issues:
- [Critical] Arbitrary File Overwrite via Predictable JIT ELF Filename (Symlink Attack).
- [High] Unwinding and debug data leak to subsequent records on error paths.
- [High] Silent suppression of parsing errors in jit_process_dump() loop.
- [Medium] State leak and MMAP2 length truncation in jit_repipe_code_move().
- [Medium] Ineffective use of flockfile() for inter-process synchronization.
--

commit 9551fc5ddf55f406039867f081e66a51333a4aa8
Author: Arnaldo Carvalho de Melo [off-list ref]

perf jitdump: Free unwinding data even when eh_frame_hdr_size is zero

This patch fixes an issue in jit_repipe_code_load() where unwinding state
was not properly cleared when eh_frame_hdr_size was zero. It updates the
cleanup condition to rely solely on the presence of unwinding_data, ensuring
it is freed correctly and not mistakenly applied to subsequent records.

Looking at jit_emit_elf() in the same file:

tools/perf/util/jitdump.c:jit_emit_elf() {
...
	nsinfo__mountns_enter(jd->nsi, &nsc);
	fd = open(filename, O_CREAT|O_TRUNC|O_WRONLY, 0644);
	saved_errno = errno;
...
}

[Severity: Critical]
This is a pre-existing issue, but does this open() call enable a symlink
attack?

perf inject --jit reads perf.data to find JIT dump directories, which are
specified by the monitored process and can be in world-writable locations
(e.g., /tmp). It constructs a predictable filename jitted-<pid>-<index>.so
in this directory.

The open() is called with O_CREAT|O_TRUNC|O_WRONLY but without O_NOFOLLOW
or O_EXCL.

Can a local attacker create a symlink at this predictable path pointing to
a critical system file (like /etc/shadow)? If perf inject is run by root
or a privileged user on this untrusted perf.data, would the target file be
truncated and overwritten with ELF data?


Looking at jit_repipe_code_load():

tools/perf/util/jitdump.c:jit_repipe_code_load() {
...
	/* 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;
	}
...
}

[Severity: High]
This isn't a bug introduced by this patch, but do jd->unwinding_data and
jd->debug_data leak if the function returns here?

jd->unwinding_data and jd->debug_data are global state pointers holding
dynamically allocated memory for the current JIT record.

If jit_repipe_code_load() hits this error condition, it returns -1
immediately. The allocated memory is not freed, and the size fields are not
reset.

Because jit_process_dump() ignores this error and continues parsing, will
the leaked unwinding and debug data be erroneously applied to the next
successful JIT_CODE_LOAD record, corrupting its ELF sections?


Looking at jit_process_dump():

tools/perf/util/jitdump.c:jit_process_dump() {
...
	while ((jr = jit_get_next_entry(jd))) {
		switch(jr->prefix.id) {
...
		default:
			ret = 0;
			continue;
		}
	}
	return ret;
}

[Severity: High]
This is a pre-existing issue, but does this loop silently suppress parsing
errors?

The while loop iterates over records and assigns the result of processing
functions to ret. There is no error-checking mechanism (e.g., if (ret)
break;) to halt processing.

If a subsequent record is successfully parsed, ret is overwritten with 0.
If an unknown record type is encountered, the default switch case explicitly
resets ret = 0.

Does this cause critical memory allocation failures, invalid record sizes,
or corrupted streams in the middle of a JIT dump to be completely ignored?

[Severity: Medium]
This is a pre-existing issue, but does modifying jit_repipe_code_load() to
unconditionally clear jd->unwinding_mapped_size cause a state leak and
MMAP2 length truncation in jit_repipe_code_move()?

jit_repipe_code_move() computes the new mapping length by reading the global
jd->unwinding_mapped_size into usize:

tools/perf/util/jitdump.c:jit_repipe_code_move() {
...
	usize = jd->unwinding_mapped_size;
...
	event->mmap2.len   = usize ? ALIGN_8(jr->move.code_size) + usize
				   : jr->move.code_size;
...
}

Because of the unconditional clear in this patch, unless a JIT agent emits
an out-of-sequence JIT_CODE_UNWINDING_INFO record immediately before the
MOVE record, will usize always be 0?

Does this cause the new MMAP2 record length to incorrectly truncate the
memory mapping, failing to cover the .eh_frame section from the original
JIT allocation?

Furthermore, if an agent does emit an unwinding record before the move,
jit_repipe_code_move() uses the size but fails to clear it. Does this leak
the state to all subsequent MOVE records?

 		jd->unwinding_size = 0;
 	}

Looking at jit_open():

tools/perf/util/jitdump.c:jit_open() {
...
	/*
	 * protect from writer modifying the file while we are reading it
	 */
	flockfile(jd->in);
...
}

[Severity: Medium]
This isn't a bug introduced by this patch, but is this use of flockfile()
ineffective for inter-process synchronization?

The comment states this is to protect from a writer modifying the file while
reading it. However, the writer is the JVM or JIT agent executing in a
different process.

Since flockfile() is a POSIX thread-synchronization primitive that only
locks the FILE * structure for thread-safe stdio operations within the
same process, it does not acquire an OS-level file lock.

Does this leave the reader vulnerable to data races and parsing corruption
if the dump file is read concurrently while the JIT agent is appending to it?

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