[PATCH v2 0/5] perf jitdump: Fix debug entry access, unwinding state and sample id sizing

[PATCH v2 0/5] perf jitdump: Fix debug entry access, unwinding state and sample id sizing

[Arnaldo Carvalho de Melo]

Hi,

Five fixes for the jitdump/genelf code path, all reported by the
sashiko-bot AI reviewer while reviewing the hardening series that is now
in perf-tools-next:

  - struct debug_entry ends with a variable-length name[], so entries
    after the first start at addresses that are not naturally aligned.

  - jit_repipe_code_load() only released the unwinding state when both
    unwinding_data and eh_frame_hdr_size were set.

  - the sample id area appended to the synthesized mmap2 records was
    cast to a fixed {u32 pid, tid; u64 time;} struct.  That only matches
    what evsel__id_hdr_size() accounts for when PERF_SAMPLE_TID is set
    as well, since the fields are appended in a fixed order skipping the
    ones not requested.  Patch 4 walks the area in that order and patch
    5 sizes the allocation with idr_size instead of a hardcoded +16, so
    that what is written and what is allocated agree.

Patches 1 and 5 are unchanged from v1.

Best regards,

- Arnaldo

What changed from v1 (599fad82626e5eba):

  PATCH 2/5:
  - Kept lineno and last_line signed.  v1 read ent->lineno into an
    unsigned int, so the "lineno - last_line" delta handed to
    emit_advance_lineno() wrapped to a large positive value and, being
    widened to a long, zero-extended instead of sign-extending: a
    backward line jump became a huge forward one, corrupting the DWARF
    line number program [sashiko-bot review of PATCH 2/5].

  PATCH 3/5:
  - Added Reviewed-by: Ian Rogers.

  PATCH 4/5 (new):
  - Walks the sample id area in the order evsel__id_hdr_size() accounts
    for (TID, TIME, ID, STREAM_ID, CPU, IDENTIFIER — 8 bytes each)
    advancing only past the fields whose sample_type bit is set,.

    This is the [Critical] finding from the sashiko-bot review of v1
    PATCH 4/5: with PERF_SAMPLE_TID unset, PERF_SAMPLE_TIME belongs at
    offset 0 and idr_size is 8, so the old code stored it at offset 8 —
    past the end of the allocation, and corrupting what a reader
    expects to find at offset 0 besides.  It is split out from the
    sizing change so that 5/5 no longer removes the slack that was
    masking it, and so that the same latent bug in
    jit_repipe_code_load() — which already sized with idr_size — is
    fixed in the same place.

  PATCH 5/5 (was 4/5):
  - Unchanged, and now safe: 4/5 above keeps the writes inside idr_size.

  DROPPED (was v1 PATCH 5/5):
  - "perf dso: Defer dropping the open list reference until after the
    lock".  Ian Rogers is not a fan of the deferral mechanism and
    suggested instead allocating dso_data separately from the dso, so
    that the lock can be scoped without it.  Dropped from this series
    while that is worked out.

  - The pre-existing issues sashiko-bot raised that are outside the
    scope of this series are recorded in tools/perf/TODO.hardening for
    follow-up work: item 170 (jit_get_next_entry() lacks per-record
    size validation), 171 (jit_process_dump()/jit_inject() swallow
    callback errors), 172 (jit_emit_elf() truncates 64-bit unwinding
    sizes to u32), 173 (jit_emit_elf() opens a predictable filename
    without O_EXCL/O_NOFOLLOW) and 176 (buffer_ext_add() realloc
    failure ignored by all callers).  The two this series does address
    are items 174 and 153, now marked fixed.

  - Rebased onto the current perf-tools-next head (92d50319b4f0c0bb).

Arnaldo Carvalho de Melo (5):
  perf jitdump: Byte-swap debug entries via unaligned-safe accessors
  perf genelf: Use unaligned-safe accessors for debug entries
  perf jitdump: Free unwinding data even when eh_frame_hdr_size is zero
  perf jitdump: Write sample id fields in the order used by
    evsel__id_hdr_size()
  perf jitdump: Size code_move event allocation with idr_size

 tools/perf/util/genelf_debug.c | 28 +++++++-----
 tools/perf/util/jitdump.c      | 84 +++++++++++++++++++++++-----------
 2 files changed, 75 insertions(+), 37 deletions(-)

base-commit: 92d50319b4f0c0bbee8a236a09063272cd22faab
v1-head: 599fad82626e5eba485b4d4d188cbd34c0e12cbc

[PATCH 1/5] perf jitdump: Byte-swap debug entries via unaligned-safe accessors

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

debug_entry records are packed with a variable-length name[] field, so
entries after the first may start at addresses that are not naturally
aligned for their u64 addr and int lineno/discrim fields.  On strict
alignment architectures the byte-swap loop in jit_get_next_entry()
performed misaligned 64-bit loads and stores through struct member
access, which is undefined behavior.

Use get_unaligned()/put_unaligned() for the byte-swap of each field.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Stephane Eranian <eranian@google.com>
Assisted-by: LLM
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/jitdump.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
index d25a9fe9b020ce87..e0d5cc9a828189a2 100644
--- a/tools/perf/util/jitdump.c
+++ b/tools/perf/util/jitdump.c
@@ -14,6 +14,8 @@
 #include <sys/stat.h>
 #include <sys/mman.h>
 #include <linux/stringify.h>
+#include <linux/kernel.h>
+#include <linux/unaligned.h>
 
 #include "event.h"
 #include "debug.h"
@@ -343,9 +345,14 @@ jit_get_next_entry(struct jit_buf_desc *jd)
 				/* name must be NUL-terminated within the record */
 				if (!memchr(ent->name, '\0', (char *)end - ent->name))
 					break;
-				ent->addr    = bswap_64(ent->addr);
-				ent->lineno  = bswap_32(ent->lineno);
-				ent->discrim = bswap_32(ent->discrim);
+				/*
+				 * debug entries are packed with a variable-length
+				 * name[], so entries after the first may be
+				 * unaligned: byte-swap via unaligned-safe accessors.
+				 */
+				put_unaligned(bswap_64(get_unaligned(&ent->addr)), &ent->addr);
+				put_unaligned(bswap_32(get_unaligned(&ent->lineno)), &ent->lineno);
+				put_unaligned(bswap_32(get_unaligned(&ent->discrim)), &ent->discrim);
 				ent = debug_entry_next(ent);
 			}
 			/* clamp so downstream consumers don't overrun */
-- 
2.55.0

[PATCH 2/5] perf genelf: Use unaligned-safe accessors for debug entries

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

debug_entry records are packed with a variable-length name[] field, so
entries after the first may start at addresses that are not naturally
aligned.  jit_process_debug_info(), get_special_opcode() and
emit_lineno_info() read and write the u64 addr and int lineno fields
through struct member access, which is undefined behavior on
strict-alignment architectures.

Use get_unaligned()/put_unaligned() to read and update each field,
matching the layout the jitdump writers (LLVM, JVM agents) emit, which
packs entries without padding.

struct debug_entry.lineno is signed and emit_advance_lineno() takes a
long line delta that relies on sign extension, so the field is read
into an int: reading it into an unsigned int would turn a backward
line jump into a huge forward one and corrupt the line number program.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Stephane Eranian <eranian@google.com>
Assisted-by: LLM
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/genelf_debug.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/tools/perf/util/genelf_debug.c b/tools/perf/util/genelf_debug.c
index 8588b3e35e008396..7d9ef43aa6ac4ab6 100644
--- a/tools/perf/util/genelf_debug.c
+++ b/tools/perf/util/genelf_debug.c
@@ -12,6 +12,8 @@
  */
 #include <linux/compiler.h>
 #include <linux/zalloc.h>
+#include <linux/kernel.h>
+#include <linux/unaligned.h>
 #include <sys/types.h>
 #include <stdio.h>
 #include <getopt.h>
@@ -303,11 +305,13 @@ static ubyte get_special_opcode(struct debug_entry *ent,
 {
 	unsigned int temp;
 	unsigned long delta_addr;
+	int lineno = get_unaligned(&ent->lineno);
+	uint64_t addr = get_unaligned(&ent->addr);
 
 	/*
 	 * delta from line_base
 	 */
-	temp = (ent->lineno - last_line) - default_debug_line_header.line_base;
+	temp = (lineno - last_line) - default_debug_line_header.line_base;
 
 	if (temp >= default_debug_line_header.line_range)
 		return 0;
@@ -315,7 +319,7 @@ static ubyte get_special_opcode(struct debug_entry *ent,
 	/*
 	 * delta of addresses
 	 */
-	delta_addr = (ent->addr - last_vma) / default_debug_line_header.minimum_instruction_length;
+	delta_addr = (addr - last_vma) / default_debug_line_header.minimum_instruction_length;
 
 	/* This is not sufficient to ensure opcode will be in [0-256] but
 	 * sufficient to ensure when summing with the delta lineno we will
@@ -362,6 +366,8 @@ static void emit_lineno_info(struct buffer_ext *be,
 	for (i = 0; i < nr_entry; i++, ent = debug_entry_next(ent)) {
 		int need_copy = 0;
 		ubyte special_opcode;
+		int lineno = get_unaligned(&ent->lineno);
+		uint64_t addr = get_unaligned(&ent->addr);
 
 		/*
 		 * check if filename changed, if so add it
@@ -376,24 +382,24 @@ static void emit_lineno_info(struct buffer_ext *be,
 
 		special_opcode = get_special_opcode(ent, last_line, last_vma);
 		if (special_opcode != 0) {
-			last_line = ent->lineno;
-			last_vma  = ent->addr;
+			last_line = lineno;
+			last_vma  = addr;
 			emit_opcode(be, special_opcode);
 		} else {
 			/*
 			 * lines differ, emit line delta
 			 */
-			if (last_line != ent->lineno) {
-				emit_advance_lineno(be, ent->lineno - last_line);
-				last_line = ent->lineno;
+			if (last_line != lineno) {
+				emit_advance_lineno(be, lineno - last_line);
+				last_line = lineno;
 				need_copy = 1;
 			}
 			/*
 			 * addresses differ, emit address delta
 			 */
-			if (last_vma != ent->addr) {
-				emit_advance_pc(be, ent->addr - last_vma);
-				last_vma = ent->addr;
+			if (last_vma != addr) {
+				emit_advance_pc(be, addr - last_vma);
+				last_vma = addr;
 				need_copy = 1;
 			}
 			/*
@@ -480,7 +486,7 @@ jit_process_debug_info(uint64_t code_addr,
 	int i;
 
 	for (i = 0; i < nr_debug_entries; i++) {
-		ent->addr = ent->addr - code_addr;
+		put_unaligned(get_unaligned(&ent->addr) - code_addr, &ent->addr);
 		ent = debug_entry_next(ent);
 	}
 	add_compilation_unit(di, buffer_ext_size(dl));
-- 
2.55.0

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

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

jit_repipe_code_load() only cleared the unwinding state when both
unwinding_data and eh_frame_hdr_size were set.  When a record carries
unwinding data but eh_frame_hdr_size is 0, the cleanup condition fails
and the unwinding state persists in jd, being applied to all subsequent
JIT_CODE_LOAD and JIT_CODE_MOVE records, duplicating unwinding sections
in the generated ELF files and inflating their event->mmap2.len.

The record is validated upstream so eh_frame_hdr_size <= unwinding_size
always holds.  Free the unwinding data based on the data pointer alone.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Reviewed-by: Ian Rogers <irogers@google.com>
Cc: Stephane Eranian <eranian@google.com>
Assisted-by: LLM
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/jitdump.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
index e0d5cc9a828189a2..efb40d93e33ae664 100644
--- a/tools/perf/util/jitdump.c
+++ b/tools/perf/util/jitdump.c
@@ -530,7 +530,7 @@ static int jit_repipe_code_load(struct jit_buf_desc *jd, union jr_entry *jr)
 		jd->nr_debug_entries = 0;
 	}
 
-	if (jd->unwinding_data && jd->eh_frame_hdr_size) {
+	if (jd->unwinding_data) {
 		zfree(&jd->unwinding_data);
 		jd->eh_frame_hdr_size = 0;
 		jd->unwinding_mapped_size = 0;
-- 
2.55.0

[PATCH 4/5] perf jitdump: Write sample id fields in the order used by evsel__id_hdr_size()

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

jit_repipe_code_load() and jit_repipe_code_move() cast the sample id
area appended to the synthesized mmap2 record to a fixed:

	struct {
		u32 pid, tid;
		u64 time;
	};

and store the timestamp at offset 8 whenever PERF_SAMPLE_TIME is set.
That matches what evsel__id_hdr_size() accounts for only when
PERF_SAMPLE_TID is set as well: the fields are appended in a fixed
order, skipping the ones not requested by sample_type, so with
PERF_SAMPLE_TID unset PERF_SAMPLE_TIME starts at offset 0 and idr_size
is 8.

Storing the timestamp at offset 8 then lands 8 bytes past the end of
the id area, which for an event allocated as sizeof(*event) + idr_size
is past the end of the heap allocation, besides corrupting the record
the tooling reading it back expects.

Walk the id area in the order used by evsel__id_hdr_size(), advancing
past each field only when its sample_type bit is set, and keep the
computed timestamp in a local variable instead of reading it back from
the event buffer.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Stephane Eranian <eranian@google.com>
Assisted-by: LLM
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/jitdump.c | 64 +++++++++++++++++++++++++++------------
 1 file changed, 44 insertions(+), 20 deletions(-)

diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
index efb40d93e33ae664..f8b937a95fe84573 100644
--- a/tools/perf/util/jitdump.c
+++ b/tools/perf/util/jitdump.c
@@ -472,10 +472,8 @@ static int jit_repipe_code_load(struct jit_buf_desc *jd, union jr_entry *jr)
 	int ret, csize;
 	uint64_t usize;
 	pid_t nspid, pid, tid;
-	struct {
-		u32 pid, tid;
-		u64 time;
-	} *id;
+	uint64_t timestamp = 0;
+	unsigned long id;
 
 	nspid = jr->load.pid;
 	pid   = jr_entry_pid(jd, jr);
@@ -561,13 +559,27 @@ static int jit_repipe_code_load(struct jit_buf_desc *jd, union jr_entry *jr)
 	event->mmap2.flags = MAP_SHARED;
 	event->mmap2.ino_generation = 1;
 
-	id = (void *)((unsigned long)event + event->mmap.header.size - idr_size);
+	/*
+	 * The sample id fields are appended in the order accounted for by
+	 * evsel__id_hdr_size(), skipping the ones not requested in
+	 * sample_type, so they cannot be written through a fixed struct:
+	 * with PERF_SAMPLE_TID unset, PERF_SAMPLE_TIME starts at offset 0
+	 * and idr_size is 8, so storing it at offset 8 runs past the end of
+	 * the event allocation.
+	 */
+	id = (unsigned long)event + event->mmap.header.size - idr_size;
 	if (jd->sample_type & PERF_SAMPLE_TID) {
-		id->pid  = pid;
-		id->tid  = tid;
+		struct { u32 pid, tid; } *id_tid = (void *)id;
+
+		id_tid->pid = pid;
+		id_tid->tid = tid;
+		id += sizeof(u64);
+	}
+	if (jd->sample_type & PERF_SAMPLE_TIME) {
+		timestamp = convert_timestamp(jd, jr->load.p.timestamp);
+		*(u64 *)id = timestamp;
+		id += sizeof(u64);
 	}
-	if (jd->sample_type & PERF_SAMPLE_TIME)
-		id->time = convert_timestamp(jd, jr->load.p.timestamp);
 
 	/*
 	 * create pseudo sample to induce dso hit increment
@@ -577,7 +589,7 @@ static int jit_repipe_code_load(struct jit_buf_desc *jd, union jr_entry *jr)
 	sample.cpumode = PERF_RECORD_MISC_USER;
 	sample.pid  = pid;
 	sample.tid  = tid;
-	sample.time = id->time;
+	sample.time = timestamp;
 	sample.ip   = addr;
 
 	ret = perf_event__process_mmap2(tool, event, &sample, jd->machine);
@@ -624,10 +636,8 @@ static int jit_repipe_code_move(struct jit_buf_desc *jd, union jr_entry *jr)
 	u16 idr_size;
 	int ret;
 	pid_t nspid, pid, tid;
-	struct {
-		u32 pid, tid;
-		u64 time;
-	} *id;
+	uint64_t timestamp = 0;
+	unsigned long id;
 
 	nspid = jr->load.pid;
 	pid   = jr_entry_pid(jd, jr);
@@ -675,13 +685,27 @@ static int jit_repipe_code_move(struct jit_buf_desc *jd, union jr_entry *jr)
 	event->mmap2.flags = MAP_SHARED;
 	event->mmap2.ino_generation = 1;
 
-	id = (void *)((unsigned long)event + event->mmap.header.size - idr_size);
+	/*
+	 * The sample id fields are appended in the order accounted for by
+	 * evsel__id_hdr_size(), skipping the ones not requested in
+	 * sample_type, so they cannot be written through a fixed struct:
+	 * with PERF_SAMPLE_TID unset, PERF_SAMPLE_TIME starts at offset 0
+	 * and idr_size is 8, so storing it at offset 8 runs past the end of
+	 * the event allocation.
+	 */
+	id = (unsigned long)event + event->mmap.header.size - idr_size;
 	if (jd->sample_type & PERF_SAMPLE_TID) {
-		id->pid  = pid;
-		id->tid  = tid;
+		struct { u32 pid, tid; } *id_tid = (void *)id;
+
+		id_tid->pid = pid;
+		id_tid->tid = tid;
+		id += sizeof(u64);
+	}
+	if (jd->sample_type & PERF_SAMPLE_TIME) {
+		timestamp = convert_timestamp(jd, jr->load.p.timestamp);
+		*(u64 *)id = timestamp;
+		id += sizeof(u64);
 	}
-	if (jd->sample_type & PERF_SAMPLE_TIME)
-		id->time = convert_timestamp(jd, jr->load.p.timestamp);
 
 	/*
 	 * create pseudo sample to induce dso hit increment
@@ -691,7 +715,7 @@ static int jit_repipe_code_move(struct jit_buf_desc *jd, union jr_entry *jr)
 	sample.cpumode = PERF_RECORD_MISC_USER;
 	sample.pid  = pid;
 	sample.tid  = tid;
-	sample.time = id->time;
+	sample.time = timestamp;
 	sample.ip   = jr->move.new_code_addr;
 
 	ret = perf_event__process_mmap2(tool, event, &sample, jd->machine);
-- 
2.55.0

[PATCH 5/5] perf jitdump: Size code_move event allocation with idr_size

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

jit_repipe_code_move() allocated the mmap2 event with a hardcoded
+16, but computes event->mmap2.header.size as sizeof(event->mmap2)
minus unused filename bytes plus idr_size.  When idr_size is larger
than 16, header.size exceeds the allocation, so perf_data__write()
reads past the heap allocation, leaking adjacent heap memory into the
generated perf.data file.

Size the allocation with idr_size like jit_repipe_code_load() does.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Stephane Eranian <eranian@google.com>
Assisted-by: LLM
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/jitdump.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
index f8b937a95fe84573..45a05316b3ca65b7 100644
--- a/tools/perf/util/jitdump.c
+++ b/tools/perf/util/jitdump.c
@@ -646,9 +646,10 @@ static int jit_repipe_code_move(struct jit_buf_desc *jd, union jr_entry *jr)
 	idr_size = jd->machine->id_hdr_size;
 
 	/*
-	 * +16 to account for sample_id_all (hack)
+	 * Sample ID is written past the end of the mmap2 record; size
+	 * the allocation to account for it instead of a hardcoded +16.
 	 */
-	event = calloc(1, sizeof(*event) + 16);
+	event = calloc(1, sizeof(*event) + idr_size);
 	if (!event)
 		return -1;
 
-- 
2.55.0

Re: [PATCH 5/5] perf jitdump: Size code_move event allocation with idr_size

[sashiko-bot]

> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> jit_repipe_code_move() allocated the mmap2 event with a hardcoded
> +16, but computes event->mmap2.header.size as sizeof(event->mmap2)
> minus unused filename bytes plus idr_size.  When idr_size is larger
> than 16, header.size exceeds the allocation, so perf_data__write()
> reads past the heap allocation, leaking adjacent heap memory into the
> generated perf.data file.
> 
> Size the allocation with idr_size like jit_repipe_code_load() does.
> 
> Reported-by: sashiko-bot <sashiko-bot@kernel.org>
> Cc: Stephane Eranian <eranian@google.com>
> Assisted-by: LLM
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

Re: [PATCH 2/5] perf genelf: Use unaligned-safe accessors for debug entries

[sashiko-bot]

> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> debug_entry records are packed with a variable-length name[] field, so
> entries after the first may start at addresses that are not naturally
> aligned.  jit_process_debug_info(), get_special_opcode() and
> emit_lineno_info() read and write the u64 addr and int lineno fields
> through struct member access, which is undefined behavior on
> strict-alignment architectures.
> 
> Use get_unaligned()/put_unaligned() to read and update each field,
> matching the layout the jitdump writers (LLVM, JVM agents) emit, which
> packs entries without padding.
> 
> struct debug_entry.lineno is signed and emit_advance_lineno() takes a
> long line delta that relies on sign extension, so the field is read
> into an int: reading it into an unsigned int would turn a backward
> line jump into a huge forward one and corrupt the line number program.
> 
> Reported-by: sashiko-bot <sashiko-bot@kernel.org>
> Cc: Stephane Eranian <eranian@google.com>
> Assisted-by: LLM
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

Re: [PATCH 1/5] perf jitdump: Byte-swap debug entries via unaligned-safe accessors

[sashiko-bot]

> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> debug_entry records are packed with a variable-length name[] field, so
> entries after the first may start at addresses that are not naturally
> aligned for their u64 addr and int lineno/discrim fields.  On strict
> alignment architectures the byte-swap loop in jit_get_next_entry()
> performed misaligned 64-bit loads and stores through struct member
> access, which is undefined behavior.
> 
> Use get_unaligned()/put_unaligned() for the byte-swap of each field.
> 
> Reported-by: sashiko-bot <sashiko-bot@kernel.org>
> Cc: Stephane Eranian <eranian@google.com>
> Assisted-by: LLM
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

Re: [PATCH 4/5] perf jitdump: Write sample id fields in the order used by evsel__id_hdr_size()

[sashiko-bot]

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

Pre-existing issues:
- [High] A malicious or artificially constructed perf.data file can trigger a 16-byte heap buffer overflow in jit_repipe_code_load and jit_repipe_code_move when sample_id_all is unset.
--

commit 899175dbae0b3cbabf63f3a42932d8de3e4aaf9e
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf jitdump: Write sample id fields in the order used by evsel__id_hdr_size()

This commit modifies jit_repipe_code_load() and jit_repipe_code_move() to
write sample ID fields sequentially based on the requested sample_type bits,
resolving out-of-bounds writes that occurred when PERF_SAMPLE_TID was unset
but PERF_SAMPLE_TIME was present.

> diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> index efb40d93e33ae..f8b937a95fe84 100644
> --- a/tools/perf/util/jitdump.c
> +++ b/tools/perf/util/jitdump.c
[ ... ]
> @@ -561,13 +559,27 @@ static int jit_repipe_code_load(struct jit_buf_desc *jd, union jr_entry *jr)
>  	event->mmap2.flags = MAP_SHARED;
>  	event->mmap2.ino_generation = 1;
>  
> -	id = (void *)((unsigned long)event + event->mmap.header.size - idr_size);
> +	/*
> +	 * The sample id fields are appended in the order accounted for by
> +	 * evsel__id_hdr_size(), skipping the ones not requested in
> +	 * sample_type, so they cannot be written through a fixed struct:
> +	 * with PERF_SAMPLE_TID unset, PERF_SAMPLE_TIME starts at offset 0
> +	 * and idr_size is 8, so storing it at offset 8 runs past the end of
> +	 * the event allocation.
> +	 */
> +	id = (unsigned long)event + event->mmap.header.size - idr_size;
>  	if (jd->sample_type & PERF_SAMPLE_TID) {
> -		id->pid  = pid;
> -		id->tid  = tid;
> +		struct { u32 pid, tid; } *id_tid = (void *)id;
> +
> +		id_tid->pid = pid;
> +		id_tid->tid = tid;
> +		id += sizeof(u64);
> +	}
> +	if (jd->sample_type & PERF_SAMPLE_TIME) {
> +		timestamp = convert_timestamp(jd, jr->load.p.timestamp);
> +		*(u64 *)id = timestamp;
> +		id += sizeof(u64);
>  	}

[Severity: High]
This is a pre-existing issue in jit_repipe_code_load, but could a malicious
or artificially constructed perf.data file trigger a 16-byte heap buffer
overflow here when sample_id_all is unset?

If an attacker provides a maliciously crafted perf.data file where
sample_id_all is unset (causing idr_size == 0) but PERF_SAMPLE_TID and
PERF_SAMPLE_TIME bits are set in sample_type, and provides a jitdump file
with a deeply nested path such that the formatted filename length reaches
PATH_MAX, the mmap2 record size consumes the entire union perf_event
allocation.

Since the allocation is determined by idr_size earlier in the function:

    idr_size = jd->machine->id_hdr_size;
    event = calloc(1, sizeof(*event) + idr_size);

The code then unconditionally writes up to 16 bytes of sample ID fields past
the end of the calloc buffer. This issue leaves jit_repipe_code_load and
jit_repipe_code_move (where it is introduced by a follow-up patch) vulnerable
to memory corruption via untrusted input.

Could we add validation to ensure idr_size correctly accounts for the required
fields before we perform these writes?

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

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

[sashiko-bot]

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 <acme@redhat.com>

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?


> diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> index e0d5cc9a82818..efb40d93e33ae 100644
> --- a/tools/perf/util/jitdump.c
> +++ b/tools/perf/util/jitdump.c
> @@ -530,7 +530,7 @@ static int jit_repipe_code_load(struct jit_buf_desc *jd, union jr_entry *jr)
>  		jd->nr_debug_entries = 0;
>  	}
>  
> -	if (jd->unwinding_data && jd->eh_frame_hdr_size) {
> +	if (jd->unwinding_data) {
>  		zfree(&jd->unwinding_data);
>  		jd->eh_frame_hdr_size = 0;
>  		jd->unwinding_mapped_size = 0;

[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