Re: [RFC PATCH v4 2/5] perf/trace-event: Write trace.dat metadata sections during parsing

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

New issues:
- [Critical] Missing bounds check on `raw_size` leads to massive out-of-bounds read and heap buffer overflow.
- [High] Hardcoded 16-byte page header breaks parsing of trace data recorded on 32-bit architectures.
- [Medium] Host state leakage / Cross-platform analysis violation when embedding the TRACECLOCK option.
- [Medium] Signed integer overflow causing undefined behavior in TIME_EXTEND bitshift.

Pre-existing issues:
- [Critical] Integer overflow in `read_saved_cmdline` size calculation leads to a heap buffer overflow on 32-bit systems.
--

--- Patch [2]: [RFC PATCH v4 2/5] perf/trace-event: Write trace.dat metadata sections during parsing ---
commit 75d79e7a5c5c4a1484c389705c19fc30dbdd1deb
Author: Tanushree Shah <[email protected]>

perf/trace-event: Write trace.dat metadata sections during parsing

Perf already captures the tracing metadata as a part of
data section in perf.data

When trace_dat_fp is set, write trace.dat compatible metadata
sections using the perf provided raw buffers.

> diff --git a/tools/perf/util/trace-dat.c b/tools/perf/util/trace-dat.c
> --- a/tools/perf/util/trace-dat.c
> +++ b/tools/perf/util/trace-dat.c
[ ... ]
> @@ -161,8 +164,12 @@ static int trace_dat__write_page(FILE *fp, unsigned long long base_ts,
>  		commit += rec_sizes[i];
>  	}
>  
> -	memcpy(page, &base_ts, sizeof(base_ts));
> -	memcpy(page + sizeof(base_ts), &commit, sizeof(commit));
> +	/* Byte-swap page header for cross-arch compatibility */
> +	ts_out = to_file_u64(pevent, base_ts);
> +	commit_out = to_file_u64(pevent, commit);
> +
> +	memcpy(page, &ts_out, sizeof(ts_out));
> +	memcpy(page + sizeof(ts_out), &commit_out, sizeof(commit_out));

[Severity: High]
Does this hardcode a 16-byte page header regardless of the original
architecture's commit size?

If the original trace was recorded on a 32-bit machine, its header_page
metadata usually specifies the commit field as a 4-byte local_t. Writing a
hardcoded 8-byte commit field here might cause parsers to misalign by 4
bytes, potentially interpreting the upper half of the 8-byte commit as
the first event record's header and breaking parsing.

[ ... ]
> @@ -229,11 +237,18 @@ static int trace_dat__write_cpu_dat(FILE *fp, int cpu, unsigned long long *file_
>  			if (!extend)
>  				return -ENOMEM;
>  
> -			extend_hdr =
> -				((time_delta & TRACE_DAT_RECORD_TIME_MASK) <<
> -				 TRACE_DAT_RECORD_TIME_SHIFT) |
> -				TRACE_DAT_RECORD_TYPE_TIME_EXTEND;
> -			delta_upper = time_delta >> TRACE_DAT_RECORD_TIME_SHIFT;
> +			if (tep_is_file_bigendian(pevent)) {
> +				extend_hdr = (time_delta & TRACE_DAT_RECORD_TIME_MASK) |
> +				     (TRACE_DAT_RECORD_TYPE_TIME_EXTEND << 27);
> +				delta_upper = time_delta >> 27;

[Severity: Medium]
Could this left shift result in undefined behavior?

Assuming TRACE_DAT_RECORD_TYPE_TIME_EXTEND is defined as 30 (a 32-bit signed
integer literal), shifting it left by 27 bits yields 4026531840. This exceeds
the maximum representable value of a 32-bit signed integer, which can cause
undefined behavior in C.

[ ... ]
> @@ -311,8 +332,11 @@ static int trace_dat__write_cpu_dat(FILE *fp, int cpu, unsigned long long *file_
>  		memcpy(data_rec, &hdr_word, TRACE_DAT_WORD_SIZE);
>  
>  		/* Large events: write actual byte length after header */
> -		if (type_len == 0)
> -			memcpy(data_rec + TRACE_DAT_WORD_SIZE, &data_len, TRACE_DAT_WORD_SIZE);
> +		if (type_len == 0) {
> +			unsigned int data_len_out = to_file_u32(pevent, data_len);
> +
> +			memcpy(data_rec + TRACE_DAT_WORD_SIZE, &data_len_out, TRACE_DAT_WORD_SIZE);
> +		}
>  
>  		memcpy(data_rec + payload_offset, event->raw, data_len);

[Severity: Critical]
Is it possible for a crafted perf.data file to cause an out-of-bounds read
and heap buffer overflow here?

If the original sample's raw_size is maliciously set near UINT_MAX
(e.g., 0xFFFFFFFF), the data_rec_size calculation in
trace_dat__write_cpu_dat() calculates data_rec_size = 8 + data_len, which
wraps around to 7. This bypasses the page size check and allocates an 8-byte
heap buffer. The memcpy() here then attempts to copy gigabytes of data into
that small buffer. 

Similarly, earlier in trace_dat__collect_cpu_event(), malloc(0xFFFFFFFF)
could succeed on a 64-bit host, causing the subsequent memcpy() to read
gigabytes out-of-bounds.

[ ... ]
> @@ -464,16 +500,17 @@ int trace_dat__write_options_section1(void)
>  	payload_start = ftell(trace_dat_fp);
>  
>  	/* CPUCOUNT option */
[ ... ]
>  	/* TRACECLOCK option */
> -	opt_id = TRACE_DAT_OPTION_TRACECLOCK;
> +	opt_id = to_file_u16(pevent, TRACE_DAT_OPTION_TRACECLOCK);
>  
>  	path = get_tracing_file("trace_clock");
>  	if (path) {
>  		clock_file = fopen(path, "r");

[Severity: Medium]
Does this inadvertently leak the conversion host's state into the
converted trace data?

Since this dynamically queries the local host's trace_clock via
get_tracing_file(), it embeds the local machine's unparsed clock file contents
(such as "[local] global...") instead of preserving the original recorded
machine's clock metadata, potentially breaking cross-platform reproducibility.

[ ... ]
> diff --git a/tools/perf/util/trace-event-read.c b/tools/perf/util/trace-event-read.c
> --- a/tools/perf/util/trace-event-read.c
> +++ b/tools/perf/util/trace-event-read.c
> @@ -383,6 +615,27 @@ static int read_saved_cmdline(struct tep_handle *pevent)
>  		pr_debug("error reading saved cmdlines\n");
>  		goto out;
>  	}
> +	/* Write cmdlines section with data */
> +	if (trace_dat_fp && !trace_dat_write_failed) {
> +		unsigned short section_id = to_file_u16(pevent, TRACE_DAT_SECTION_CMDLINE);
> +		unsigned short flags = to_file_u16(pevent, 0);

[Severity: Critical]
This is a pre-existing issue, but does the size calculation for the
saved command line buffer overflow on 32-bit systems?

Looking at read_saved_cmdline(), when size is read as a 64-bit value, if it
is 0xFFFFFFFF, the allocation calculation of size + 1 overflows to
0x100000000.

On 32-bit architectures, this is truncated to 0, causing malloc(0) to return
a small valid pointer. The subsequent do_read(buf, size) then passes
0xFFFFFFFF to the read() syscall, which can overwrite the heap and
potentially cause a denial of service or arbitrary code execution.

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