Re: [PATCH 09/12] perf jitdump: Use dirname() return value in jit_open()

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

Pre-existing issues:
- [High] Missing total_size validation in JIT_CODE_MOVE record processing allows uninitialized heap memory to be leaked into the output perf.data file.
- [High] An insufficient memory allocation in jit_repipe_code_move() causes a heap out-of-bounds read when writing the generated event to the output file.
--

--- Patch [9]: [PATCH 09/12] perf jitdump: Use dirname() return value in jit_open() ---
commit 8e32408a1e72fef109700c74dd502aaacc3125c7
Author: Arnaldo Carvalho de Melo <[email protected]>

perf jitdump: Use dirname() return value in jit_open()

jit_open() calls dirname(jd->dir) but ignores the return value.  POSIX
says dirname() may return a pointer to internal static storage — glibc
does this when the path has no '/', returning "." from a static buffer
and leaving jd->dir unchanged with the original filename.

Capture the return value and copy it back to jd->dir when dirname()
returns a different pointer.

Fixes: 9b07e27f88b9 ("perf inject: Add jitdump mmap injection support")
Reported-by: sashiko-bot <[email protected]>
Cc: Stephane Eranian <[email protected]>
Assisted-by: Claude:claude-opus-4.6
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>

> diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> index 3085091b95a51..02840dbf8a1fc 100644
> --- a/tools/perf/util/jitdump.c
> +++ b/tools/perf/util/jitdump.c

[ ... ]

> static int jit_repipe_code_move(struct jit_buf_desc *jd, union jr_entry *jr)
> {
> 	struct perf_sample sample;
> 	union perf_event *event;
> 	const struct perf_tool *tool = jd->session->tool;
> 	char *filename;
> 	size_t size;
> 	struct stat st;
> 	int usize;
> 	u16 idr_size;
> 	int ret;
> 	pid_t nspid, pid, tid;
> 	struct {
> 		u32 pid, tid;
> 		u64 time;
> 	} *id;
> 
> 	nspid = jr->load.pid;
> 	pid   = jr_entry_pid(jd, jr);
> 	tid   = jr_entry_tid(jd, jr);

[Severity: High]
This isn't a bug introduced by this patch, but is there a missing bounds check
here?

Unlike jit_repipe_code_load(), this function appears to blindly access fields
from the code move record without first verifying that the record total_size
is at least sizeof(jr->move).

If a parsed jitdump file contains a truncated code move record, could this
read uninitialized fields from the jd->buf buffer and leak leftover heap data
into the output perf.data file?

> 	usize = jd->unwinding_mapped_size;
> 	idr_size = jd->machine->id_hdr_size;
> 
> 	/*
> 	 * +16 to account for sample_id_all (hack)
> 	 */
> 	event = calloc(1, sizeof(*event) + 16);

[Severity: High]
This is a pre-existing issue, but is this allocation large enough for the
generated event?

The memory is allocated with a hardcoded + 16 extra bytes. If the session's
sample_type includes multiple fields (like CPU, TIME, and TID), idr_size can
easily exceed 16 bytes.

> 	if (!event)
> 		return -1;

[ ... ]

> 	event->mmap2.header.type = PERF_RECORD_MMAP2;
> 	event->mmap2.header.misc = PERF_RECORD_MISC_USER;
> 	event->mmap2.header.size = (sizeof(event->mmap2) -
> 			(sizeof(event->mmap2.filename) - size) + idr_size);

[Severity: High]
This is also a pre-existing issue related to the allocation above, but might
this cause a heap out-of-bounds read?

When the header size is calculated here using the dynamically larger idr_size,
the size will exceed the allocated space by (idr_size - 16) bytes.

Would this cause perf_data__write() to read out-of-bounds heap memory?

Should the code dynamically allocate (sizeof(*event) + idr_size) instead?

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