Re: [PATCH 02/12] perf jitdump: Fix debug_data and unwinding_data leaks

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

Pre-existing issues:
- [Critical] The `csize` value from the untrusted JIT dump file is not validated against `total_size`, allowing an integer underflow that leads to a massive out-of-bounds heap read and information leak.
- [Critical] Output `.so` files are opened without `O_EXCL` and `O_NOFOLLOW` in a potentially attacker-controlled directory, allowing a symlink attack to overwrite arbitrary files.
- [High] Code load events fail to consume and free `unwinding_data` and `debug_data` if their respective auxiliary sizes or counts are zero, leading to state contamination.
- [High] Calling `funlockfile()` without a preceding `flockfile()` on the `malloc()` error path causes undefined behavior.
- [High] Missing bounds check on the JIT record `total_size` before accessing the `sym` string pointer causes an out-of-bounds string read.
--

--- Patch [2]: [PATCH 02/12] perf jitdump: Fix debug_data and unwinding_data leaks ---
commit e7687575b20ca5df1e7780e1c7b436a5bc7d2f8c
Author: Arnaldo Carvalho de Melo <[email protected]>

perf jitdump: Fix debug_data and unwinding_data leaks
    
This commit fixes memory leaks in jit_repipe_debug_info() and jit_close().
It uses zfree() to correctly free existing allocations before reassignment
and cleans up at the end of the session.

> diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> --- a/tools/perf/util/jitdump.c
> +++ b/tools/perf/util/jitdump.c
> @@ -61,12 +61,12 @@ static int
>  jit_emit_elf(struct jit_buf_desc *jd,
>  	     char *filename,
[ ... ]
>  	nsinfo__mountns_enter(jd->nsi, &nsc);
>  	fd = open(filename, O_CREAT|O_TRUNC|O_WRONLY, 0644);

[Severity: Critical]
This isn't a bug introduced by this patch, but opening output files without
O_EXCL and O_NOFOLLOW in a potentially attacker-controlled directory might
allow a symlink attack to overwrite arbitrary files.

Could this be updated to use safer open flags?

[ ... ]

> @@ -117,6 +117,8 @@ jit_close(struct jit_buf_desc *jd)
>  	funlockfile(jd->in);
>  	fclose(jd->in);
>  	jd->in = NULL;
> +	zfree(&jd->debug_data);
> +	zfree(&jd->unwinding_data);
>  }

[ ... ]

> @@ -154,9 +154,9 @@ static int
>  jit_open(struct jit_buf_desc *jd, const char *name)
>  {
[ ... ]
>  	bsz = hmax(sizeof(header), sizeof(*prefix));
> 
>  	buf = malloc(bsz);
>  	if (!buf)
>  		goto error;
> 
>  	/*
>  	 * protect from writer modifying the file while we are reading it
>  	 */
>  	flockfile(jd->in);
[ ... ]
> @@ -217,9 +217,9 @@ static int
>  	return 0;
>  error:
>  	free(buf);
>  	funlockfile(jd->in);

[Severity: High]
This isn't a bug introduced by this patch, but if the initial malloc(bsz)
fails, we jump to error and call funlockfile(jd->in) without a preceding
flockfile().

Does calling funlockfile() on an unlocked FILE stream cause undefined
behavior or corruption of internal libc mutexes here?

>  	fclose(jd->in);
>  	return retval;
>  }

[ ... ]

> @@ -465,10 +465,10 @@ static int jit_repipe_code_load(struct jit_buf_desc *jd, union jr_entry *jr)
>  	nspid = jr->load.pid;
>  	pid   = jr_entry_pid(jd, jr);
>  	tid   = jr_entry_tid(jd, jr);
>  	csize = jr->load.code_size;
>  	usize = jd->unwinding_mapped_size;
>  	addr  = jr->load.code_addr;
>  	sym   = (void *)((unsigned long)jr + sizeof(jr->load));

[Severity: High]
This is a pre-existing issue, but is it possible for jr->load.p.total_size
to be smaller than sizeof(jr->load)?

The sym pointer is derived assuming total_size is at least 56 bytes, which
isn't validated before access. Could this lead to an out-of-bounds string
read if a smaller total_size is provided?

>  	code  = (unsigned long)jr + jr->load.p.total_size - csize;

[Severity: Critical]
This is a pre-existing issue, but the code pointer is calculated using csize
without checking if it exceeds total_size.

Could an attacker provide a csize larger than total_size to cause an integer
underflow, resulting in a massive out-of-bounds heap read and information
leak into the generated ELF file?

>  	count = jr->load.code_index;
>  	idr_size = jd->machine->id_hdr_size;
[ ... ]
> @@ -475,10 +475,10 @@ static int jit_repipe_code_load(struct jit_buf_desc *jd, union jr_entry *jr)
>  	event = calloc(1, sizeof(*event) + idr_size);
>  	if (!event)
>  		return -1;
> 
>  	filename = event->mmap2.filename;
>  	size = snprintf(filename, PATH_MAX, "%s/jitted-%d-%" PRIu64 ".so",
>  			jd->dir,
>  			nspid,
>  			count);

[Severity: Critical]
This isn't a bug introduced by this patch, but the filename is predictably
constructed using fields (nspid and count) controlled by the JIT dump.

Combined with the lack of O_EXCL|O_NOFOLLOW in jit_emit_elf(), does this
allow an attacker to perform a symlink attack when perf inject is run?

[ ... ]

> @@ -495,10 +495,10 @@ 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) {
>  		zfree(&jd->unwinding_data);
>  		jd->eh_frame_hdr_size = 0;
>  		jd->unwinding_mapped_size = 0;
>  		jd->unwinding_size = 0;
>  	}

[Severity: High]
This is a pre-existing issue, but this conditional cleanup leaves
jd->unwinding_data dangling if eh_frame_hdr_size is zero.

If a JIT runtime emits JIT_CODE_UNWINDING_INFO without .eh_frame_hdr, is the
unwinding data incorrectly applied to all subsequent JIT_CODE_LOAD events
until overwritten, producing corrupted ELF files?

[ ... ]

> @@ -665,6 +667,7 @@ static int jit_repipe_debug_info(struct jit_buf_desc *jd, union jr_entry *jr)
>  
>  	memcpy(data, &jr->info.entries, sz);
>  
> +	zfree(&jd->debug_data);
>  	jd->debug_data       = data;
>  
>  	/*

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