在 2020年07月30日 21:34, crash-utility-requ...@redhat.com 写道:
> Message: 4
> Date: Thu, 30 Jul 2020 15:34:30 +0200
> From: Mathias Krause <mini...@grsecurity.net>
> To: crash-utility@redhat.com
> Subject: [Crash-utility] [PATCH 3/3] Support core files with "unusual"
> layout
> Message-ID: <20200730133430.7773-4-mini...@grsecurity.net>
> 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 <mini...@grsecurity.net>
> ---
> 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
--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
