| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aDcGKINHFEEacu3x@x1>
Date: Wed, 28 May 2025 09:48:40 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Ian Rogers <irogers@...gle.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
Namhyung Kim <namhyung@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>,
Adrian Hunter <adrian.hunter@...el.com>,
Kan Liang <kan.liang@...ux.intel.com>,
Miguel Ojeda <ojeda@...nel.org>,
Alex Gaynor <alex.gaynor@...il.com>,
Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
Björn Roy Baron <bjorn3_gh@...tonmail.com>,
Benno Lossin <benno.lossin@...ton.me>,
Andreas Hindborg <a.hindborg@...nel.org>,
Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
Danilo Krummrich <dakr@...nel.org>,
Jiapeng Chong <jiapeng.chong@...ux.alibaba.com>,
James Clark <james.clark@...aro.org>,
Howard Chu <howardchu95@...il.com>,
Weilin Wang <weilin.wang@...el.com>,
Stephen Brennan <stephen.s.brennan@...cle.com>,
Andi Kleen <ak@...ux.intel.com>, Dmitry Vyukov <dvyukov@...gle.com>,
linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/6] perf symbol: Fix use-after-free in
filename__read_build_id
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@...gle.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
>
Powered by blists - more mailing lists