Re: [PATCH v1 1/6] perf symbol: Fix use-after-free in filename__read_build_id

[Arnaldo Carvalho de Melo <acme@kernel.org>]

On Tue, May 27, 2025 at 11:06:58AM -0700, Ian Rogers wrote:
> The same buf is used for the program headers and reading notes. As the
> notes memory may be reallocated then this corrupts the memory pointed
> to by the phdr. Using the same buffer is in any case a logic
> error. Rather than deal with the duplicated code, introduce an elf32
> boolean and a union for either the elf32 or elf64 headers that are in
> use. Let the program headers have their own memory and grow the buffer
> for notes as necessary.

Applied.

- Arnaldo
 
> Before `perf list -j` compiled with asan would crash with:
> ```
> ==4176189==ERROR: AddressSanitizer: heap-use-after-free on address 0x5160000070b8 at pc 0x555d3b15075b bp 0x7ffebb5a8090 sp 0x7ffebb5a8088
> READ of size 8 at 0x5160000070b8 thread T0
>     #0 0x555d3b15075a in filename__read_build_id tools/perf/util/symbol-minimal.c:212:25
>     #1 0x555d3ae43aff in filename__sprintf_build_id tools/perf/util/build-id.c:110:8
> ...
> 
> 0x5160000070b8 is located 312 bytes inside of 560-byte region [0x516000006f80,0x5160000071b0)
> freed by thread T0 here:
>     #0 0x555d3ab21840 in realloc (perf+0x264840) (BuildId: 12dff2f6629f738e5012abdf0e90055518e70b5e)
>     #1 0x555d3b1506e7 in filename__read_build_id tools/perf/util/symbol-minimal.c:206:11
> ...
> 
> previously allocated by thread T0 here:
>     #0 0x555d3ab21423 in malloc (perf+0x264423) (BuildId: 12dff2f6629f738e5012abdf0e90055518e70b5e)
>     #1 0x555d3b1503a2 in filename__read_build_id tools/perf/util/symbol-minimal.c:182:9
> ...
> ```
> 
> Note: this bug is long standing and not introduced by the other asan
> fix in commit fa9c4977fbfb ("perf symbol-minimal: Fix double free in
> filename__read_build_id").
> 
> Fixes: b691f64360ecec49 ("perf symbols: Implement poor man's ELF parser")
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/symbol-minimal.c | 168 +++++++++++++------------------
>  1 file changed, 70 insertions(+), 98 deletions(-)
> 
> diff --git a/tools/perf/util/symbol-minimal.c b/tools/perf/util/symbol-minimal.c
> index d8da3da01fe6..c9b7a1ca5e52 100644
> --- a/tools/perf/util/symbol-minimal.c
> +++ b/tools/perf/util/symbol-minimal.c
> @@ -90,11 +90,23 @@ int filename__read_build_id(const char *filename, struct build_id *bid)
>  {
>  	FILE *fp;
>  	int ret = -1;
> -	bool need_swap = false;
> +	bool need_swap = false, elf32;
>  	u8 e_ident[EI_NIDENT];
> -	size_t buf_size;
> -	void *buf;
>  	int i;
> +	union {
> +		struct {
> +			Elf32_Ehdr ehdr32;
> +			Elf32_Phdr *phdr32;
> +		};
> +		struct {
> +			Elf64_Ehdr ehdr64;
> +			Elf64_Phdr *phdr64;
> +		};
> +	} hdrs;
> +	void *phdr;
> +	size_t phdr_size;
> +	void *buf = NULL;
> +	size_t buf_size = 0;
>  
>  	fp = fopen(filename, "r");
>  	if (fp == NULL)
> @@ -108,119 +120,79 @@ int filename__read_build_id(const char *filename, struct build_id *bid)
>  		goto out;
>  
>  	need_swap = check_need_swap(e_ident[EI_DATA]);
> +	elf32 = e_ident[EI_CLASS] == ELFCLASS32;
>  
> -	/* for simplicity */
> -	fseek(fp, 0, SEEK_SET);
> -
> -	if (e_ident[EI_CLASS] == ELFCLASS32) {
> -		Elf32_Ehdr ehdr;
> -		Elf32_Phdr *phdr;
> -
> -		if (fread(&ehdr, sizeof(ehdr), 1, fp) != 1)
> -			goto out;
> +	if (fread(elf32 ? (void *)&hdrs.ehdr32 : (void *)&hdrs.ehdr64,
> +		  elf32 ? sizeof(hdrs.ehdr32) : sizeof(hdrs.ehdr32),
> +		  1, fp) != 1)
> +		goto out;
>  
> -		if (need_swap) {
> -			ehdr.e_phoff = bswap_32(ehdr.e_phoff);
> -			ehdr.e_phentsize = bswap_16(ehdr.e_phentsize);
> -			ehdr.e_phnum = bswap_16(ehdr.e_phnum);
> +	if (need_swap) {
> +		if (elf32) {
> +			hdrs.ehdr32.e_phoff = bswap_32(hdrs.ehdr32.e_phoff);
> +			hdrs.ehdr32.e_phentsize = bswap_16(hdrs.ehdr32.e_phentsize);
> +			hdrs.ehdr32.e_phnum = bswap_16(hdrs.ehdr32.e_phnum);
> +		} else {
> +			hdrs.ehdr64.e_phoff = bswap_64(hdrs.ehdr64.e_phoff);
> +			hdrs.ehdr64.e_phentsize = bswap_16(hdrs.ehdr64.e_phentsize);
> +			hdrs.ehdr64.e_phnum = bswap_16(hdrs.ehdr64.e_phnum);
>  		}
> +	}
> +	phdr_size = elf32 ? hdrs.ehdr32.e_phentsize * hdrs.ehdr32.e_phnum
> +			  : hdrs.ehdr64.e_phentsize * hdrs.ehdr64.e_phnum;
> +	phdr = malloc(phdr_size);
> +	if (phdr == NULL)
> +		goto out;
>  
> -		buf_size = ehdr.e_phentsize * ehdr.e_phnum;
> -		buf = malloc(buf_size);
> -		if (buf == NULL)
> -			goto out;
> -
> -		fseek(fp, ehdr.e_phoff, SEEK_SET);
> -		if (fread(buf, buf_size, 1, fp) != 1)
> -			goto out_free;
> -
> -		for (i = 0, phdr = buf; i < ehdr.e_phnum; i++, phdr++) {
> -			void *tmp;
> -			long offset;
> -
> -			if (need_swap) {
> -				phdr->p_type = bswap_32(phdr->p_type);
> -				phdr->p_offset = bswap_32(phdr->p_offset);
> -				phdr->p_filesz = bswap_32(phdr->p_filesz);
> -			}
> -
> -			if (phdr->p_type != PT_NOTE)
> -				continue;
> -
> -			offset = phdr->p_offset;
> -			if (phdr->p_filesz > buf_size) {
> -				buf_size = phdr->p_filesz;
> -				tmp = realloc(buf, buf_size);
> -				if (tmp == NULL)
> -					goto out_free;
> -				buf = tmp;
> -			}
> -			fseek(fp, offset, SEEK_SET);
> -			if (fread(buf, phdr->p_filesz, 1, fp) != 1)
> -				goto out_free;
> +	fseek(fp, elf32 ? hdrs.ehdr32.e_phoff : hdrs.ehdr64.e_phoff, SEEK_SET);
> +	if (fread(phdr, phdr_size, 1, fp) != 1)
> +		goto out_free;
>  
> -			ret = read_build_id(buf, phdr->p_filesz, bid, need_swap);
> -			if (ret == 0) {
> -				ret = bid->size;
> -				break;
> -			}
> -		}
> -	} else {
> -		Elf64_Ehdr ehdr;
> -		Elf64_Phdr *phdr;
> +	if (elf32)
> +		hdrs.phdr32 = phdr;
> +	else
> +		hdrs.phdr64 = phdr;
>  
> -		if (fread(&ehdr, sizeof(ehdr), 1, fp) != 1)
> -			goto out;
> +	for (i = 0; i < elf32 ? hdrs.ehdr32.e_phnum : hdrs.ehdr64.e_phnum; i++) {
> +		size_t p_filesz;
>  
>  		if (need_swap) {
> -			ehdr.e_phoff = bswap_64(ehdr.e_phoff);
> -			ehdr.e_phentsize = bswap_16(ehdr.e_phentsize);
> -			ehdr.e_phnum = bswap_16(ehdr.e_phnum);
> +			if (elf32) {
> +				hdrs.phdr32[i].p_type = bswap_32(hdrs.phdr32[i].p_type);
> +				hdrs.phdr32[i].p_offset = bswap_32(hdrs.phdr32[i].p_offset);
> +				hdrs.phdr32[i].p_filesz = bswap_32(hdrs.phdr32[i].p_offset);
> +			} else {
> +				hdrs.phdr64[i].p_type = bswap_32(hdrs.phdr64[i].p_type);
> +				hdrs.phdr64[i].p_offset = bswap_64(hdrs.phdr64[i].p_offset);
> +				hdrs.phdr64[i].p_filesz = bswap_64(hdrs.phdr64[i].p_filesz);
> +			}
>  		}
> +		if ((elf32 ? hdrs.phdr32[i].p_type : hdrs.phdr64[i].p_type) != PT_NOTE)
> +			continue;
>  
> -		buf_size = ehdr.e_phentsize * ehdr.e_phnum;
> -		buf = malloc(buf_size);
> -		if (buf == NULL)
> -			goto out;
> -
> -		fseek(fp, ehdr.e_phoff, SEEK_SET);
> -		if (fread(buf, buf_size, 1, fp) != 1)
> -			goto out_free;
> -
> -		for (i = 0, phdr = buf; i < ehdr.e_phnum; i++, phdr++) {
> +		p_filesz = elf32 ? hdrs.phdr32[i].p_filesz : hdrs.phdr64[i].p_filesz;
> +		if (p_filesz > buf_size) {
>  			void *tmp;
> -			long offset;
> -
> -			if (need_swap) {
> -				phdr->p_type = bswap_32(phdr->p_type);
> -				phdr->p_offset = bswap_64(phdr->p_offset);
> -				phdr->p_filesz = bswap_64(phdr->p_filesz);
> -			}
> -
> -			if (phdr->p_type != PT_NOTE)
> -				continue;
>  
> -			offset = phdr->p_offset;
> -			if (phdr->p_filesz > buf_size) {
> -				buf_size = phdr->p_filesz;
> -				tmp = realloc(buf, buf_size);
> -				if (tmp == NULL)
> -					goto out_free;
> -				buf = tmp;
> -			}
> -			fseek(fp, offset, SEEK_SET);
> -			if (fread(buf, phdr->p_filesz, 1, fp) != 1)
> +			buf_size = p_filesz;
> +			tmp = realloc(buf, buf_size);
> +			if (tmp == NULL)
>  				goto out_free;
> +			buf = tmp;
> +		}
> +		fseek(fp, elf32 ? hdrs.phdr32[i].p_offset : hdrs.phdr64[i].p_offset, SEEK_SET);
> +		if (fread(buf, p_filesz, 1, fp) != 1)
> +			goto out_free;
>  
> -			ret = read_build_id(buf, phdr->p_filesz, bid, need_swap);
> -			if (ret == 0) {
> -				ret = bid->size;
> -				break;
> -			}
> +		ret = read_build_id(buf, p_filesz, bid, need_swap);
> +		if (ret == 0) {
> +			ret = bid->size;
> +			break;
>  		}
>  	}
>  out_free:
>  	free(buf);
> +	free(phdr);
>  out:
>  	fclose(fp);
>  	return ret;
> -- 
> 2.49.0.1204.g71687c7c1d-goog
>