Re: [PATCH 06/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:
- [Critical] Unvalidated nr_entry field in JIT_CODE_DEBUG_INFO leads to arbitrary heap out-of-bounds read and write.
- [High] Insecure file creation in jit_emit_elf allows arbitrary file overwrite via symlink attacks, potentially leading to privilege escalation.
--

--- Patch [6]: [PATCH 06/12] perf jitdump: Fix funlockfile on unlocked stream in jit_open() error path ---
commit 9c3ece5485fc42d2bde29b52e999638dd26c389f
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 fd11e07bf00b7..c3f11d1c1d76d 100644
> --- a/tools/perf/util/jitdump.c
> +++ b/tools/perf/util/jitdump.c
> @@ -155,7 +155,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
> @@ -244,8 +244,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 is a pre-existing issue, but I noticed an insecure file creation bug
in jit_emit_elf() while reviewing this 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;
	nsinfo__mountns_exit(&nsc);
    ...
}

The open() call truncates and writes to the file, but lacks O_EXCL or
O_NOFOLLOW.

Does this leave the file creation vulnerable to symlink-based file overwrite
attacks?

If a local attacker pre-creates a symlink matching the expected .so output
filename (e.g., jitted-<pid>-<count>.so) in a world-writable directory (like
/tmp), could they point it to a sensitive system file such as /etc/shadow?

When a privileged user runs perf inject on the attacker's perf.data file,
could the target file be truncated and overwritten with an ELF binary
containing jitted code?


[Severity: Critical]
This is a pre-existing issue, but the nr_entry field is completely unvalidated
against the actual JIT_CODE_DEBUG_INFO record size when jd->needs_bswap is
false (native endianness):

tools/perf/util/jitdump.c:jit_get_next_entry() {
    ...
	case JIT_CODE_DEBUG_INFO:
		if (jd->needs_bswap) {
    ...
			/* clamp so downstream consumers don't overrun */
			jr->info.nr_entry = n;
		}
		break;
    ...
}

If a maliciously crafted jitdump file provides an artificially large nr_entry,
could it cause this loop in jit_process_debug_info() to blindly iterate
based on the unvalidated value?

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

If so, it appears debug_entry_next() could read out of bounds, and the
ent->addr = ent->addr - code_addr; operation might repeatedly subtract fully
attacker-controlled values (code_addr) from arbitrary heap metadata.

Could this lead to arbitrary code execution when processing an untrusted file?

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