From cb09223a56c50dbd963ce89e340e8c82d9118c28 Mon Sep 17 00:00:00 2001 From: Alexander Egorenkov Date: Fri, 3 Sep 2021 22:03:33 +0200 Subject: [PATCH] zdump/dfi_elf: Fix stack buffer overflow in nt_read() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sanity check ELF notes descriptor size before reading it. This prevents stack buffer overflows in case a dump contains invalid ELF notes. Instead of reading a note's descriptor into a temporary buffer on stack, read it directly into the buffer given to nt_read() but also provide a maximum length of the given buffer to nt_read() in order to prevent overflows. This problem was found with valgrind and AFL fuzzing + ASAN. AFL + ASAN findings: [root@t83lp49 s390-tools]# ./zdump/zgetdump -iVVVVV ~/zgetdump-fuzzing/findings/crashes/id\:000008\,sig\:06\,src\:000007\,op\:arith8\,pos\:67\,val\:+3 TRACE: DFI initialization DEBUG: DFI trying s390tape DEBUG: DFI s390tape returned with rc -19 DEBUG: DFI trying devmem DEBUG: DFI devmem returned with rc -19 DEBUG: DFI trying s390mv_ext DEBUG: DFI s390mv_ext returned with rc -19 DEBUG: DFI trying s390mv DEBUG: DFI s390mv returned with rc -19 DEBUG: DFI trying s390_ext DEBUG: DFI S390 extended initialization DEBUG: DFI s390_ext returned with rc -19 DEBUG: DFI trying s390 DEBUG: DFI S390 initialization DEBUG: DFI s390 returned with rc -19 DEBUG: DFI trying lkcd DEBUG: DFI lkcd returned with rc -19 DEBUG: DFI trying elf DEBUG: DFI ELF initialization DEBUG: DFI ELF e_phnum 11 DEBUG: DFI ELF p_type[0] 0x4 DEBUG: DFI ELF n_type 0x0 AddressSanitizer:DEADLYSIGNAL ================================================================= ==208548==ERROR: AddressSanitizer: stack-overflow on address 0x03ffef05d000 (pc 0x0000010051b0 bp 0x03fff107dc40 sp 0x03ffef05dac8 T0) #0 0x10051b0 (/root/s390-tools/zdump/zgetdump+0x10051b0) SUMMARY: AddressSanitizer: stack-overflow (/root/s390-tools/zdump/zgetdump+0x10051b0) ==208548==ABORTING valgrind findings: ==56423== Source and destination overlap in memcpy(0x4a86d38, 0x4a875e0, 4096) ==56423== at 0x4839F86: memcpy (in /usr/lib/s390x-linux-gnu/valgrind/vgpreload_memcheck-s390x-linux.so) ==56423== by 0x114253: memcpy (string_fortified.h:34) ==56423== by 0x114253: nt_read (dfi_elf.c:95) ==56423== by 0x1145FF: nt_s390_prefix_read (dfi_elf.c:195) ==56423== by 0x1145FF: pt_notes_add (dfi_elf.c:259) ==56423== by 0x1145FF: dfi_elf_init (dfi_elf.c:326) ==56423== by 0x112A57: dfi_init (dfi.c:1212) ==56423== by 0x10D663: do_dump_info (zgetdump.c:127) ==56423== by 0x10D663: main (zgetdump.c:182) ==56423== ==56423== Invalid write of size 8 ==56423== at 0x4839E28: memcpy (in /usr/lib/s390x-linux-gnu/valgrind/vgpreload_memcheck-s390x-linux.so) ==56423== by 0x114253: memcpy (string_fortified.h:34) ==56423== by 0x114253: nt_read (dfi_elf.c:95) ==56423== by 0x1145FF: nt_s390_prefix_read (dfi_elf.c:195) ==56423== by 0x1145FF: pt_notes_add (dfi_elf.c:259) ==56423== by 0x1145FF: dfi_elf_init (dfi_elf.c:326) ==56423== by 0x112A57: dfi_init (dfi.c:1212) ==56423== by 0x10D663: do_dump_info (zgetdump.c:127) ==56423== by 0x10D663: main (zgetdump.c:182) ==56423== Address 0x4a86ee0 is 0 bytes after a block of size 912 alloc'd ==56423== at 0x483675E: calloc (in /usr/lib/s390x-linux-gnu/valgrind/vgpreload_memcheck-s390x-linux.so) ==56423== by 0x10E71D: zg_alloc (zg.c:93) ==56423== by 0x114793: nt_prstatus_read (dfi_elf.c:123) ==56423== by 0x114793: pt_notes_add (dfi_elf.c:234) ==56423== by 0x114793: dfi_elf_init (dfi_elf.c:326) ==56423== by 0x112A57: dfi_init (dfi.c:1212) ==56423== by 0x10D663: do_dump_info (zgetdump.c:127) ==56423== by 0x10D663: main (zgetdump.c:182) Signed-off-by: Alexander Egorenkov Suggested-by: Marc Hartmayer Reported-by: Steffen Eiden Signed-off-by: Jan Höppner --- zdump/dfi_elf.c | 57 +++++++++++++++++++++++-------------------------- 1 file changed, 27 insertions(+), 30 deletions(-) diff --git a/zdump/dfi_elf.c b/zdump/dfi_elf.c index f6f616aa..a8e98afa 100644 --- a/zdump/dfi_elf.c +++ b/zdump/dfi_elf.c @@ -70,38 +70,36 @@ static int pt_load_add(Elf64_Phdr *phdr) return 0; } -/* - * Skip name of note - */ -static void nt_name_skip(Elf64_Nhdr *note) -{ - zg_seek_cur(g.fh, ROUNDUP(note->n_namesz, 4), ZG_CHECK); -} - /* * Read note */ -static int nt_read(Elf64_Nhdr *note, void *buf) +static int nt_read(const Elf64_Nhdr *note, void *buf, size_t buf_len) { - off_t buf_len = ROUNDUP(note->n_descsz, 4); - char tmp_buf[buf_len]; + ssize_t nread; - util_log_print(UTIL_LOG_TRACE, "DFI ELF n_descsz %u\n", buf_len); - - nt_name_skip(note); - if (zg_read(g.fh, tmp_buf, buf_len, ZG_CHECK_ERR) != buf_len) + /* We cannot read more than the current note provides */ + if (note->n_descsz < buf_len) return -EINVAL; - if (buf) - memcpy(buf, tmp_buf, note->n_descsz); + /* Skip note's name and position file at note's descriptor */ + zg_seek_cur(g.fh, ROUNDUP(note->n_namesz, 4), ZG_CHECK); + /* Read note's descriptor */ + nread = zg_read(g.fh, buf, buf_len, ZG_CHECK_ERR); + if (nread < 0 || (size_t)nread != buf_len) + return -EINVAL; + /* Skip the rest of note's descriptor until the next note */ + zg_seek_cur(g.fh, ROUNDUP(note->n_descsz, 4) - buf_len, ZG_CHECK); return 0; } /* * Skip note */ -static int nt_skip(Elf64_Nhdr *note) +static void nt_skip(const Elf64_Nhdr *note) { - return nt_read(note, NULL); + /* Skip note's name + descriptor and position file at the next note */ + zg_seek_cur(g.fh, + ROUNDUP(note->n_namesz, 4) + ROUNDUP(note->n_descsz, 4), + ZG_CHECK); } /* @@ -122,7 +120,7 @@ static struct dfi_cpu *nt_prstatus_read(Elf64_Nhdr *note) struct dfi_cpu *cpu = dfi_cpu_alloc(); struct nt_prstatus_64 nt_prstatus; - if (nt_read(note, &nt_prstatus)) + if (nt_read(note, &nt_prstatus, sizeof(nt_prstatus))) return NULL; memcpy(cpu->gprs, &nt_prstatus.gprs, sizeof(cpu->gprs)); @@ -141,7 +139,7 @@ static int nt_fpregset_read(struct dfi_cpu *cpu, Elf64_Nhdr *note) struct nt_fpregset_64 nt_fpregset; check_cpu(cpu, "FPREGSET"); - if (nt_read(note, &nt_fpregset)) + if (nt_read(note, &nt_fpregset, sizeof(nt_fpregset))) return -EINVAL; memcpy(&cpu->fpc, &nt_fpregset.fpc, sizeof(cpu->fpc)); @@ -155,7 +153,7 @@ static int nt_fpregset_read(struct dfi_cpu *cpu, Elf64_Nhdr *note) static int nt_s390_timer_read(struct dfi_cpu *cpu, Elf64_Nhdr *note) { check_cpu(cpu, "S390_TIMER"); - return nt_read(note, &cpu->timer); + return nt_read(note, &cpu->timer, sizeof(cpu->timer)); } /* @@ -164,7 +162,7 @@ static int nt_s390_timer_read(struct dfi_cpu *cpu, Elf64_Nhdr *note) static int nt_s390_todcmp_read(struct dfi_cpu *cpu, Elf64_Nhdr *note) { check_cpu(cpu, "S390_TODCMP"); - return nt_read(note, &cpu->todcmp); + return nt_read(note, &cpu->todcmp, sizeof(cpu->todcmp)); } /* @@ -173,7 +171,7 @@ static int nt_s390_todcmp_read(struct dfi_cpu *cpu, Elf64_Nhdr *note) static int nt_s390_todpreg_read(struct dfi_cpu *cpu, Elf64_Nhdr *note) { check_cpu(cpu, "S390_TODPREG"); - return nt_read(note, &cpu->todpreg); + return nt_read(note, &cpu->todpreg, sizeof(cpu->todpreg)); } /* @@ -182,7 +180,7 @@ static int nt_s390_todpreg_read(struct dfi_cpu *cpu, Elf64_Nhdr *note) static int nt_s390_ctrs_read(struct dfi_cpu *cpu, Elf64_Nhdr *note) { check_cpu(cpu, "S390_CTRS"); - return nt_read(note, &cpu->ctrs); + return nt_read(note, &cpu->ctrs, sizeof(cpu->ctrs)); } /* @@ -191,7 +189,7 @@ static int nt_s390_ctrs_read(struct dfi_cpu *cpu, Elf64_Nhdr *note) static int nt_s390_prefix_read(struct dfi_cpu *cpu, Elf64_Nhdr *note) { check_cpu(cpu, "S390_PREFIX"); - return nt_read(note, &cpu->prefix); + return nt_read(note, &cpu->prefix, sizeof(cpu->prefix)); } /* @@ -200,7 +198,7 @@ static int nt_s390_prefix_read(struct dfi_cpu *cpu, Elf64_Nhdr *note) static int nt_s390_vxrs_low_read(struct dfi_cpu *cpu, Elf64_Nhdr *note) { check_cpu(cpu, "S390_VXRS_LOW"); - return nt_read(note, &cpu->vxrs_low); + return nt_read(note, &cpu->vxrs_low, sizeof(cpu->vxrs_low)); } /* @@ -209,7 +207,7 @@ static int nt_s390_vxrs_low_read(struct dfi_cpu *cpu, Elf64_Nhdr *note) static int nt_s390_vxrs_high_read(struct dfi_cpu *cpu, Elf64_Nhdr *note) { check_cpu(cpu, "S390_VXRS_HIGH"); - return nt_read(note, &cpu->vxrs_high); + return nt_read(note, &cpu->vxrs_high, sizeof(cpu->vxrs_high)); } /* @@ -271,8 +269,7 @@ static int pt_notes_add(Elf64_Phdr *phdr) dfi_cpu_content_fac_add(DFI_CPU_CONTENT_FAC_VX); break; default: - if (nt_skip(¬e)) - return -EINVAL; + nt_skip(¬e); break; } }