Re: [Crash-utility] Crash-utility Digest, Vol 178, Issue 21

在 2020年07月30日 21:34, crash-utility-request(a)redhat.com 写道: > Message: 4 > Date: Thu, 30 Jul 2020 15:34:30 +0200 > From: Mathias Krause <minipli(a)grsecurity.net&gt; > To: crash-utility(a)redhat.com > Subject: [Crash-utility] [PATCH 3/3] Support core files with "unusual" > layout > Message-ID: <20200730133430.7773-4-minipli(a)grsecurity.net&gt; > Content-Type: text/plain; charset=US-ASCII > > The netdump code not only gets used for netdump/diskdump files, but also > for kdump core files. These can also be generated with the 'vmss2core' > tool that'll produce a slightly different format that isn't as densely > packed as we expect it to be. In fact, the implicit assumption that the > ELF program headers directly follow the ELF header isn't always true for Hi, Mathias Thanks for your patch. I agree with you, the actual files may differ. > these files, as they may contain a small padding area after the ELF > header -- which is totally conforming in regards to the ELF spec. This > padding in combination with the implicit assumption of densely packed > headers make us interpret the padding bytes as program headers which is > obviously wrong. > > Support these kind of core files too by not blindly assuming the program > headers follow the ELF header but by looking at the program header > offset in the ELF header and use that instead. Add some guarding sanity > checks to decline operating on obviously malicious or broken core files. > > To not needlessly make things too complicated, allow a "padding space" of > up to 128 bytes only. > > Signed-off-by: Mathias Krause <minipli(a)grsecurity.net&gt; > --- > netdump.c | 86 ++++++++++++++++++++++++++++++++++++++----------------- > netdump.h | 2 ++ > 2 files changed, 61 insertions(+), 27 deletions(-) > > diff --git a/netdump.c b/netdump.c > index 406416af36bf..0490bb52a8ed 100644 > --- a/netdump.c > +++ b/netdump.c > @@ -132,7 +132,7 @@ is_netdump(char *file, ulong source_query) > } > } > > - size = MIN_NETDUMP_ELF_HEADER_SIZE; > + size = SAFE_NETDUMP_ELF_HEADER_SIZE; > if ((eheader = (char *)malloc(size)) == NULL) { > fprintf(stderr, "cannot malloc minimum ELF header buffer\n"); > clean_exit(1); > @@ -219,8 +219,22 @@ is_netdump(char *file, ulong source_query) > source_query)) > goto bailout; > > - load32 = (Elf32_Phdr *) > - &eheader[sizeof(Elf32_Ehdr)+sizeof(Elf32_Phdr)]; For the Program Header table, could it be optional? If present, the value of e_phoff should be non-zero, otherwise its value is zero. Would it be better to check if the value of e_phoff is valid?

> + if (elf32->e_phoff != sizeof(Elf32_Ehdr)) { > + if (CRASHDEBUG(1)) > + error(WARNING, "%s: first PHdr not following " > + "EHdr (PHdr offset = %u)", file,

> + elf32->e_phoff); > + /* it's okay as long as we've read enough data */ > + if (elf32->e_phoff > size - 2 * sizeof(Elf32_Phdr)) { > + error(WARNING, "%s: PHdr to far into file!\n", > + file); > + goto bailout; > + } > + } > + > + /* skip the NOTE program header */ > + load32 = (Elf32_Phdr *) > + &eheader[elf32->e_phoff+sizeof(Elf32_Phdr)]; > > if ((load32->p_offset & (MIN_PAGE_SIZE-1)) || > (load32->p_align == 0)) > @@ -291,8 +305,22 @@ is_netdump(char *file, ulong source_query) > source_query)) > goto bailout; > > - load64 = (Elf64_Phdr *) > - &eheader[sizeof(Elf64_Ehdr)+sizeof(Elf64_Phdr)]; > + if (elf64->e_phoff != sizeof(Elf64_Ehdr)) { > + if (CRASHDEBUG(1)) > + error(WARNING, "%s: first PHdr not following " > + "EHdr (PHdr offset = %u)", file,

> + elf64->e_phoff); > + /* it's okay as long as we've read enough data */ > + if (elf64->e_phoff > size - 2 * sizeof(Elf64_Phdr)) { > + error(WARNING, "%s: PHdr to far into file!\n", > + file); > + goto bailout; > + } > + } > + > + /* skip the NOTE program header */ > + load64 = (Elf64_Phdr *) > + &eheader[elf64->e_phoff+sizeof(Elf64_Phdr)]; > > if ((load64->p_offset & (MIN_PAGE_SIZE-1)) || > (load64->p_align == 0)) > @@ -353,9 +381,8 @@ is_netdump(char *file, ulong source_query) > clean_exit(1); > } > nd->notes32 = (Elf32_Phdr *) > - &nd->elf_header[sizeof(Elf32_Ehdr)]; > - nd->load32 = (Elf32_Phdr *) > - &nd->elf_header[sizeof(Elf32_Ehdr)+sizeof(Elf32_Phdr)]; > + &nd->elf_header[nd->elf32->e_phoff]; > + nd->load32 = nd->notes32 + 1; > if (format == NETDUMP_ELF32) > nd->page_size = (uint)nd->load32->p_align; > dump_Elf32_Ehdr(nd->elf32); > @@ -392,9 +419,8 @@ is_netdump(char *file, ulong source_query) > clean_exit(1); > } > nd->notes64 = (Elf64_Phdr *) > - &nd->elf_header[sizeof(Elf64_Ehdr)]; > - nd->load64 = (Elf64_Phdr *) > - &nd->elf_header[sizeof(Elf64_Ehdr)+sizeof(Elf64_Phdr)]; > + &nd->elf_header[nd->elf64->e_phoff]; > + nd->load64 = nd->notes64 + 1; > if (format == NETDUMP_ELF64) > nd->page_size = (uint)nd->load64->p_align; > dump_Elf64_Ehdr(nd->elf64); > @@ -469,8 +495,8 @@ resize_elf_header(int fd, char *file, char **eheader_ptr, char **sect0_ptr, > case NETDUMP_ELF32: > case KDUMP_ELF32: > num_pt_load_segments = elf32->e_phnum - 1; > - header_size = sizeof(Elf32_Ehdr) + sizeof(Elf32_Phdr) + > - (sizeof(Elf32_Phdr) * num_pt_load_segments); > + header_size = MAX(sizeof(Elf32_Ehdr), elf32->e_phoff) + > + (sizeof(Elf32_Phdr) * (num_pt_load_segments + 1)); > break; > > case NETDUMP_ELF64: > @@ -513,8 +539,8 @@ resize_elf_header(int fd, char *file, char **eheader_ptr, char **sect0_ptr, > } else > num_pt_load_segments = elf64->e_phnum - 1; > > - header_size = sizeof(Elf64_Ehdr) + sizeof(Elf64_Phdr) + > - (sizeof(Elf64_Phdr) * num_pt_load_segments); > + header_size = MAX(sizeof(Elf64_Ehdr), elf64->e_phoff) + > + (sizeof(Elf64_Phdr) * (num_pt_load_segments + 1)); > break; > } > > @@ -544,7 +570,7 @@ resize_elf_header(int fd, char *file, char **eheader_ptr, char **sect0_ptr, > { > case NETDUMP_ELF32: > case KDUMP_ELF32: > - load32 = (Elf32_Phdr *)&eheader[sizeof(Elf32_Ehdr)+sizeof(Elf32_Phdr)]; > + load32 = (Elf32_Phdr *)&eheader[elf32->e_phoff+sizeof(Elf32_Phdr)]; > p_offset32 = load32->p_offset; > for (i = 0; i < num_pt_load_segments; i++, load32 += 1) { > if (load32->p_offset && > @@ -556,7 +582,7 @@ resize_elf_header(int fd, char *file, char **eheader_ptr, char **sect0_ptr, > > case NETDUMP_ELF64: > case KDUMP_ELF64: > - load64 = (Elf64_Phdr *)&eheader[sizeof(Elf64_Ehdr)+sizeof(Elf64_Phdr)]; > + load64 = (Elf64_Phdr *)&eheader[elf64->e_phoff+sizeof(Elf64_Phdr)]; > p_offset64 = load64->p_offset; > for (i = 0; i < num_pt_load_segments; i++, load64 += 1) { > if (load64->p_offset && > @@ -4459,8 +4485,12 @@ proc_kcore_init_32(FILE *fp, int kcore_fd) > close(fd); > > elf32 = (Elf32_Ehdr *)&eheader[0]; > - notes32 = (Elf32_Phdr *)&eheader[sizeof(Elf32_Ehdr)]; > - load32 = (Elf32_Phdr *)&eheader[sizeof(Elf32_Ehdr)+sizeof(Elf32_Phdr)]; > + if (elf32->e_phoff > sizeof(eheader) - 2 * sizeof(Elf32_Phdr)) { > + error(INFO, "/proc/kcore: ELF program header offset too big!\n"); > + return FALSE; > + } > + notes32 = (Elf32_Phdr *)&eheader[elf32->e_phoff]; > + load32 = notes32 + 1; > > pkd->segments = elf32->e_phnum - 1; > > @@ -4479,9 +4509,8 @@ proc_kcore_init_32(FILE *fp, int kcore_fd) > } > > BCOPY(&eheader[0], &pkd->elf_header[0], pkd->header_size); > - pkd->notes32 = (Elf32_Phdr *)&pkd->elf_header[sizeof(Elf32_Ehdr)]; > - pkd->load32 = (Elf32_Phdr *) > - &pkd->elf_header[sizeof(Elf32_Ehdr)+sizeof(Elf32_Phdr)]; > + pkd->notes32 = (Elf32_Phdr *)&pkd->elf_header[elf32->e_phoff]; > + pkd->load32 = pkd->notes32 + 1; > pkd->flags |= KCORE_ELF32; > > kcore_memory_dump(CRASHDEBUG(1) ? fp : pc->nullfp); > @@ -4529,8 +4558,12 @@ proc_kcore_init_64(FILE *fp, int kcore_fd) > close(fd); > > elf64 = (Elf64_Ehdr *)&eheader[0]; > - notes64 = (Elf64_Phdr *)&eheader[sizeof(Elf64_Ehdr)]; > - load64 = (Elf64_Phdr *)&eheader[sizeof(Elf64_Ehdr)+sizeof(Elf64_Phdr)]; > + if (elf64->e_phoff > sizeof(eheader) - 2 * sizeof(Elf64_Phdr)) { > + error(INFO, "/proc/kcore: ELF program header offset too big!\n"); > + return FALSE; > + } > + notes64 = (Elf64_Phdr *)&eheader[elf64->e_phoff]; > + load64 = notes64 + 1; > > pkd->segments = elf64->e_phnum - 1; > > @@ -4550,9 +4583,8 @@ proc_kcore_init_64(FILE *fp, int kcore_fd) > } > > BCOPY(&eheader[0], &pkd->elf_header[0], pkd->header_size); > - pkd->notes64 = (Elf64_Phdr *)&pkd->elf_header[sizeof(Elf64_Ehdr)]; > - pkd->load64 = (Elf64_Phdr *) > - &pkd->elf_header[sizeof(Elf64_Ehdr)+sizeof(Elf64_Phdr)]; > + pkd->notes64 = (Elf64_Phdr *)&pkd->elf_header[elf64->e_phoff]; > + pkd->load64 = pkd->notes64 + 1; > pkd->flags |= KCORE_ELF64; > > kcore_memory_dump(CRASHDEBUG(1) ? fp : pc->nullfp); > diff --git a/netdump.h b/netdump.h > index 7fa04f7c3a0f..844279bc4a00 100644 > --- a/netdump.h > +++ b/netdump.h > @@ -25,6 +25,8 @@ > sizeof(Elf64_Ehdr)+sizeof(Elf64_Phdr)+sizeof(Elf64_Phdr) > #define MIN_NETDUMP_ELF_HEADER_SIZE \ > MAX(MIN_NETDUMP_ELF32_HEADER_SIZE, MIN_NETDUMP_ELF64_HEADER_SIZE) > +#define SAFE_NETDUMP_ELF_HEADER_SIZE \ > + (MIN_NETDUMP_ELF_HEADER_SIZE+128) > Can you please describe more details why the size of padding area is 128 bytes? Are there any particular reasons?

Thanks. Lianbo

> #define NT_TASKSTRUCT 4 > #define NT_DISKDUMP 0x70000001 > -- 2.20.1