* failed reading "Smallest x86 ELF Hello World" @ 2017-09-28 19:06 Josh Stone 2017-10-04 18:55 ` Mark Wielaard 0 siblings, 1 reply; 5+ messages in thread From: Josh Stone @ 2017-09-28 19:06 UTC (permalink / raw) To: elfutils-devel From http://timelessname.com/elfbin/ Elfutils completely fails to read the file: $ eu-readelf --version eu-readelf (elfutils) 0.169 Copyright [...] $ eu-readelf -a hello eu-readelf: failed reading 'hello': invalid file descriptor It's not clear that we should care, since that page even says it's "a completely corrupted x86 ELF Binary that still runs." But since it's good enough for the kernel to run it, I'd hope for *something* from tools. It also seems weird to get the message "invalid file descriptor", from ELF_E_INVALID_FILE, which makes it sound like more like EBADF. The file descriptor itself is fine - it just doesn't like the ELF within. (DWARF_E_INVALID_FILE's message is just "invalid file".) FWIW, binutils readelf does better: $ readelf --version GNU readelf version 2.27-24.fc26 Copyright [...] $ readelf -a hello ELF Header: Magic: 7f 45 4c 46 01 01 01 48 69 20 57 6f 72 6c 64 0a Class: ELF32 Data: 2's complement, little endian Version: 1 (current) OS/ABI: <unknown: 48> ABI Version: 105 Type: EXEC (Executable file) Machine: Intel 80386 Version: 0x1 Entry point address: 0x8048080 Start of program headers: 52 (bytes into file) Start of section headers: 309248 (bytes into file) Flags: 0x80cd0000 Size of this header: 22763 (bytes) Size of program headers: 32 (bytes) Number of program headers: 2 Size of section headers: 40 (bytes) Number of section headers: 5 Section header string table index: 4 readelf: hello: Error: Reading 0xc8 bytes extends past end of file for section headers readelf: hello: Error: Section headers are not available! Program Headers: Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align LOAD 0x000000 0x08048000 0x08048000 0x000a2 0x000a2 R E 0x1000 LOAD 0x0000a4 0x080490a4 0x080490a4 0x00009 0x00009 W 0x9007b900 There is no dynamic section in this file. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: failed reading "Smallest x86 ELF Hello World" 2017-09-28 19:06 failed reading "Smallest x86 ELF Hello World" Josh Stone @ 2017-10-04 18:55 ` Mark Wielaard 2017-10-04 18:57 ` [PATCH 1/2] libelf: Add ELF_E_INVALID_ELF error value Mark Wielaard 2017-10-13 15:09 ` failed reading "Smallest x86 ELF Hello World" Mark Wielaard 0 siblings, 2 replies; 5+ messages in thread From: Mark Wielaard @ 2017-10-04 18:55 UTC (permalink / raw) To: Josh Stone, elfutils-devel On Thu, 2017-09-28 at 12:06 -0700, Josh Stone wrote: > From http://timelessname.com/elfbin/ > > Elfutils completely fails to read the file: > > $ eu-readelf --version > eu-readelf (elfutils) 0.169 > Copyright [...] > $ eu-readelf -a hello > eu-readelf: failed reading 'hello': invalid file descriptor > > It's not clear that we should care, since that page even says it's "a > completely corrupted x86 ELF Binary that still runs." But since it's > good enough for the kernel to run it, I'd hope for *something* from > tools. > > It also seems weird to get the message "invalid file descriptor", > from ELF_E_INVALID_FILE, which makes it sound like more like > EBADF. The file descriptor itself is fine - it just doesn't like the > ELF within. At first I though it was because eu-readelf uses libdwfl to get more information about the ELF file. To get all the data about the Dwfl_Modules we might hit some error that might not be relevant for the initial showing of data. But that isn't really it in this case. The first issue is indeed that almost anything that goes wrong when setting up the initial Elf handle ends up being described as ELF_E_INVALID_FILE. Which is not always the correct error code. So I introduced ELF_E_INVALID_ELF which indicates it is bad ELF data being encountered and not just the inability to read the data from the file descriptor. Also in a couple of cases we didn't explicitly set the libelf errno to indicate what really went wrong. I made sure we always do now. libelf: Add ELF_E_INVALID_ELF error value. This at least gives a nicer error message: eu-readelf: failed reading './hello': invalid ELF file data But while auditing this code it is clear we go out of our way to get the section (count) making sure we don't touch any bad data. If there is a change we might read anything bad from the (mmapped) file then we explicitly set the elf->state.elf[64|32].scns.cnt to zero. Which is respected throughout libelf whenever we try to touch section headers. Except... during the initial read we double check e_shoff is sane and error out early. Even though the code right below it explicitly doesn't use it when scncnt is zero. So we can fix this sanity check. libelf: Don't error out when sanity checking e_shoff if scncnt is zero. This then gives similar output to binutils readelf: ELF Header: Magic: 7f 45 4c 46 01 01 01 48 69 20 57 6f 72 6c 64 0a Class: ELF32 Data: 2's complement, little endian Ident Version: 1 (current) OS/ABI: <unknown>: 72 ABI Version: 105 Type: EXEC (Executable file) Machine: Intel 80386 Version: 1 (current) Entry point address: 0x8048080 Start of program headers: 52 (bytes into file) Start of section headers: 309248 (bytes into file) Flags: 0x80cd0000 Size of this header: 22763 (bytes) Size of program header entries: 32 (bytes) Number of program headers entries: 2 Size of section header entries: 40 (bytes) Number of section headers entries: 5 Section header string table index: 4 Section Headers: [Nr] Name Type Addr Off Size ES Flags Lk Inf Al Program Headers: Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align LOAD 0x000000 0x08048000 0x08048000 0x0000a2 0x0000a2 R E 0x1000 LOAD 0x0000a4 0x080490a4 0x080490a4 0x000009 0x000009 W 0x9007b900 Section to Segment mapping: Segment Sections... 00 01 Maybe it could be improved a little more by adding a warning that 5 section [headers] were expected, but none could be read. But I leave it at this for now. Cheers, Mark ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] libelf: Add ELF_E_INVALID_ELF error value. 2017-10-04 18:55 ` Mark Wielaard @ 2017-10-04 18:57 ` Mark Wielaard 2017-10-04 18:57 ` [PATCH 2/2] libelf: Don't error out when sanity checking e_shoff if scncnt is zero Mark Wielaard 2017-10-13 15:09 ` failed reading "Smallest x86 ELF Hello World" Mark Wielaard 1 sibling, 1 reply; 5+ messages in thread From: Mark Wielaard @ 2017-10-04 18:57 UTC (permalink / raw) To: elfutils-devel; +Cc: Josh Stone, Mark Wielaard Add ELF_E_INVALID_ELF which is set when the ELF file data is bad. This is different from ELF_E_INVALID_FILE which is set when the file could not be read. Signed-off-by: Mark Wielaard <mark@klomp.org> --- libelf/ChangeLog | 12 +++++++++ libelf/elf_begin.c | 61 ++++++++++++++++++++++++++++++++++------------ libelf/elf_error.c | 7 +++++- libelf/elf_getshdrstrndx.c | 20 ++++++++++----- libelf/libelfP.h | 1 + tests/ChangeLog | 4 +++ tests/msg_tst.c | 1 + 7 files changed, 83 insertions(+), 23 deletions(-) diff --git a/libelf/ChangeLog b/libelf/ChangeLog index 7bd9e1b..36b57dd 100644 --- a/libelf/ChangeLog +++ b/libelf/ChangeLog @@ -1,3 +1,15 @@ +2017-10-04 Mark Wielaard <mark@klomp.org> + + * libelfP.h: Add ELF_E_INVALID_ELF to error values enum. + * elf_error.c (ELF_E_INVALID_ELF_IDX): New define. Use it as value + for ELF_E_INVALID_ELF in msgidx. + * elf_getshdrstrndx.c (elf_getshdrstrndx): Distinquish between pread + failing and not having enough data. + * elf_begin.c (get_shnum): Likewise. Explicitly set libelf errno on + too large value. + (file_read_elf): Make sure to always set libelf errno when returning + NULL. Distinquish between i/o file and elf data errors. + 2017-08-18 Ulf Hermann <ulf.hermann@qt.io> * gelf_xlate.c: Use attribute_packed. diff --git a/libelf/elf_begin.c b/libelf/elf_begin.c index 6f85038..5545278 100644 --- a/libelf/elf_begin.c +++ b/libelf/elf_begin.c @@ -158,6 +158,7 @@ get_shnum (void *map_address, unsigned char *e_ident, int fildes, off_t offset, else { Elf32_Word size; + ssize_t r; if (likely (map_address != NULL)) /* gcc will optimize the memcpy to a simple memory @@ -167,11 +168,19 @@ get_shnum (void *map_address, unsigned char *e_ident, int fildes, off_t offset, + offset))->sh_size, sizeof (Elf32_Word)); else - if (unlikely (pread_retry (fildes, &size, sizeof (Elf32_Word), - offset + ehdr.e32->e_shoff - + offsetof (Elf32_Shdr, sh_size)) + if (unlikely ((r = pread_retry (fildes, &size, + sizeof (Elf32_Word), + offset + ehdr.e32->e_shoff + + offsetof (Elf32_Shdr, + sh_size))) != sizeof (Elf32_Word))) - return (size_t) -1l; + { + if (r < 0) + __libelf_seterrno (ELF_E_INVALID_FILE); + else + __libelf_seterrno (ELF_E_INVALID_ELF); + return (size_t) -1l; + } if (e_ident[EI_DATA] != MY_ELFDATA) CONVERT (size); @@ -207,6 +216,7 @@ get_shnum (void *map_address, unsigned char *e_ident, int fildes, off_t offset, + offset))->sh_size; else { + ssize_t r; if (likely (map_address != NULL)) /* gcc will optimize the memcpy to a simple memory access while taking care of alignment issues. */ @@ -215,19 +225,30 @@ get_shnum (void *map_address, unsigned char *e_ident, int fildes, off_t offset, + offset))->sh_size, sizeof (Elf64_Xword)); else - if (unlikely (pread_retry (fildes, &size, sizeof (Elf64_Xword), - offset + ehdr.e64->e_shoff - + offsetof (Elf64_Shdr, sh_size)) + if (unlikely ((r = pread_retry (fildes, &size, + sizeof (Elf64_Xword), + offset + ehdr.e64->e_shoff + + offsetof (Elf64_Shdr, + sh_size))) != sizeof (Elf64_Xword))) - return (size_t) -1l; + { + if (r < 0) + __libelf_seterrno (ELF_E_INVALID_FILE); + else + __libelf_seterrno (ELF_E_INVALID_ELF); + return (size_t) -1l; + } if (e_ident[EI_DATA] != MY_ELFDATA) CONVERT (size); } if (size > ~((GElf_Word) 0)) - /* Invalid value, it is too large. */ - return (size_t) -1l; + { + /* Invalid value, it is too large. */ + __libelf_seterrno (ELF_E_INVALID_ELF); + return (size_t) -1l; + } result = size; } @@ -255,11 +276,13 @@ file_read_elf (int fildes, void *map_address, unsigned char *e_ident, && e_ident[EI_DATA] != ELFDATA2MSB))) { /* Cannot handle this. */ - __libelf_seterrno (ELF_E_INVALID_FILE); + __libelf_seterrno (ELF_E_INVALID_ELF); return NULL; } - /* Determine the number of sections. */ + /* Determine the number of sections. Returns -1 and sets libelf errno + if the file handle or elf file is invalid. Returns zero if there + are no section headers (or they cannot be read). */ size_t scncnt = get_shnum (map_address, e_ident, fildes, offset, maxsize); if (scncnt == (size_t) -1l) /* Could not determine the number of sections. */ @@ -269,10 +292,16 @@ file_read_elf (int fildes, void *map_address, unsigned char *e_ident, if (e_ident[EI_CLASS] == ELFCLASS32) { if (scncnt > SIZE_MAX / (sizeof (Elf_Scn) + sizeof (Elf32_Shdr))) - return NULL; + { + __libelf_seterrno (ELF_E_INVALID_ELF); + return NULL; + } } else if (scncnt > SIZE_MAX / (sizeof (Elf_Scn) + sizeof (Elf64_Shdr))) - return NULL; + { + __libelf_seterrno (ELF_E_INVALID_ELF); + return NULL; + } /* We can now allocate the memory. Even if there are no section headers, we allocate space for a zeroth section in case we need it later. */ @@ -281,7 +310,7 @@ file_read_elf (int fildes, void *map_address, unsigned char *e_ident, Elf *elf = allocate_elf (fildes, map_address, offset, maxsize, cmd, parent, ELF_K_ELF, scnmax * sizeof (Elf_Scn)); if (elf == NULL) - /* Not enough memory. */ + /* Not enough memory. allocate_elf will have set libelf errno. */ return NULL; assert ((unsigned int) scncnt == scncnt); @@ -350,7 +379,7 @@ file_read_elf (int fildes, void *map_address, unsigned char *e_ident, { free_and_out: free (elf); - __libelf_seterrno (ELF_E_INVALID_FILE); + __libelf_seterrno (ELF_E_INVALID_ELF); return NULL; } elf->state.elf32.shdr diff --git a/libelf/elf_error.c b/libelf/elf_error.c index 888b389..5364e68 100644 --- a/libelf/elf_error.c +++ b/libelf/elf_error.c @@ -94,8 +94,12 @@ static const char msgstr[] = (ELF_E_NOMEM_IDX + sizeof "out of memory") N_("invalid file descriptor") "\0" -#define ELF_E_INVALID_OP_IDX \ +#define ELF_E_INVALID_ELF_IDX \ (ELF_E_INVALID_FILE_IDX + sizeof "invalid file descriptor") + N_("invalid ELF file data") + "\0" +#define ELF_E_INVALID_OP_IDX \ + (ELF_E_INVALID_ELF_IDX + sizeof "invalid ELF file data") N_("invalid operation") "\0" #define ELF_E_NO_VERSION_IDX \ @@ -280,6 +284,7 @@ static const uint_fast16_t msgidx[ELF_E_NUM] = [ELF_E_INVALID_ENCODING] = ELF_E_INVALID_ENCODING_IDX, [ELF_E_NOMEM] = ELF_E_NOMEM_IDX, [ELF_E_INVALID_FILE] = ELF_E_INVALID_FILE_IDX, + [ELF_E_INVALID_ELF] = ELF_E_INVALID_ELF_IDX, [ELF_E_INVALID_OP] = ELF_E_INVALID_OP_IDX, [ELF_E_NO_VERSION] = ELF_E_NO_VERSION_IDX, [ELF_E_INVALID_CMD] = ELF_E_INVALID_CMD_IDX, diff --git a/libelf/elf_getshdrstrndx.c b/libelf/elf_getshdrstrndx.c index aead2fe..ad884fd 100644 --- a/libelf/elf_getshdrstrndx.c +++ b/libelf/elf_getshdrstrndx.c @@ -133,13 +133,17 @@ elf_getshdrstrndx (Elf *elf, size_t *dst) /* We avoid reading in all the section headers. Just read the first one. */ Elf32_Shdr shdr_mem; + ssize_t r; - if (unlikely (pread_retry (elf->fildes, &shdr_mem, - sizeof (Elf32_Shdr), offset) + if (unlikely ((r = pread_retry (elf->fildes, &shdr_mem, + sizeof (Elf32_Shdr), offset)) != sizeof (Elf32_Shdr))) { /* We must be able to read this ELF section header. */ - __libelf_seterrno (ELF_E_INVALID_FILE); + if (r < 0) + __libelf_seterrno (ELF_E_INVALID_FILE); + else + __libelf_seterrno (ELF_E_INVALID_ELF); result = -1; goto out; } @@ -194,13 +198,17 @@ elf_getshdrstrndx (Elf *elf, size_t *dst) /* We avoid reading in all the section headers. Just read the first one. */ Elf64_Shdr shdr_mem; + ssize_t r; - if (unlikely (pread_retry (elf->fildes, &shdr_mem, - sizeof (Elf64_Shdr), offset) + if (unlikely ((r = pread_retry (elf->fildes, &shdr_mem, + sizeof (Elf64_Shdr), offset)) != sizeof (Elf64_Shdr))) { /* We must be able to read this ELF section header. */ - __libelf_seterrno (ELF_E_INVALID_FILE); + if (r < 0) + __libelf_seterrno (ELF_E_INVALID_FILE); + else + __libelf_seterrno (ELF_E_INVALID_ELF); result = -1; goto out; } diff --git a/libelf/libelfP.h b/libelf/libelfP.h index a4a0a3a..ca805ac 100644 --- a/libelf/libelfP.h +++ b/libelf/libelfP.h @@ -102,6 +102,7 @@ enum ELF_E_INVALID_ENCODING, ELF_E_NOMEM, ELF_E_INVALID_FILE, + ELF_E_INVALID_ELF, ELF_E_INVALID_OP, ELF_E_NO_VERSION, ELF_E_INVALID_CMD, diff --git a/tests/ChangeLog b/tests/ChangeLog index 7b6bf30..35688dc 100644 --- a/tests/ChangeLog +++ b/tests/ChangeLog @@ -1,3 +1,7 @@ +2017-10-04 Mark Wielaard <mark@klomp.org> + + * msg_tst.c: Handle ELF_E_INVALID_ELF. + 2017-09-10 Mark Wielaard <mark@klomp.org> * run-ar.sh: New test. diff --git a/tests/msg_tst.c b/tests/msg_tst.c index 7baea0a..aa974d0 100644 --- a/tests/msg_tst.c +++ b/tests/msg_tst.c @@ -39,6 +39,7 @@ static struct { ELF_E_INVALID_ENCODING, "invalid encoding" }, { ELF_E_NOMEM, "out of memory" }, { ELF_E_INVALID_FILE, "invalid file descriptor" }, + { ELF_E_INVALID_ELF, "invalid ELF file data" }, { ELF_E_INVALID_OP, "invalid operation" }, { ELF_E_NO_VERSION, "ELF version not set" }, { ELF_E_INVALID_CMD, "invalid command" }, -- 1.8.3.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/2] libelf: Don't error out when sanity checking e_shoff if scncnt is zero. 2017-10-04 18:57 ` [PATCH 1/2] libelf: Add ELF_E_INVALID_ELF error value Mark Wielaard @ 2017-10-04 18:57 ` Mark Wielaard 0 siblings, 0 replies; 5+ messages in thread From: Mark Wielaard @ 2017-10-04 18:57 UTC (permalink / raw) To: elfutils-devel; +Cc: Josh Stone, Mark Wielaard We won't use the e_shoff value in that case because we will set elf->state.elf[64|32].scns.cnt to zero to indicate not to read any section header data from the file. Signed-off-by: Mark Wielaard <mark@klomp.org> --- libelf/ChangeLog | 5 +++++ libelf/elf_begin.c | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/libelf/ChangeLog b/libelf/ChangeLog index 36b57dd..6aa1c6f 100644 --- a/libelf/ChangeLog +++ b/libelf/ChangeLog @@ -1,5 +1,10 @@ 2017-10-04 Mark Wielaard <mark@klomp.org> + * elf_begin.c (file_read_elf): Skip sanity checking e_shoff if scncnt + is zero, we won't use it then. + +2017-10-04 Mark Wielaard <mark@klomp.org> + * libelfP.h: Add ELF_E_INVALID_ELF to error values enum. * elf_error.c (ELF_E_INVALID_ELF_IDX): New define. Use it as value for ELF_E_INVALID_ELF in msgidx. diff --git a/libelf/elf_begin.c b/libelf/elf_begin.c index 5545278..fb3a5b5 100644 --- a/libelf/elf_begin.c +++ b/libelf/elf_begin.c @@ -373,7 +373,7 @@ file_read_elf (int fildes, void *map_address, unsigned char *e_ident, || (((uintptr_t) ((char *) ehdr + e_shoff) & (__alignof__ (Elf32_Shdr) - 1)) == 0))) { - if (unlikely (e_shoff >= maxsize) + if (unlikely (scncnt > 0 && e_shoff >= maxsize) || unlikely (maxsize - e_shoff < scncnt * sizeof (Elf32_Shdr))) { @@ -475,7 +475,7 @@ file_read_elf (int fildes, void *map_address, unsigned char *e_ident, || (((uintptr_t) ((char *) ehdr + e_shoff) & (__alignof__ (Elf64_Shdr) - 1)) == 0))) { - if (unlikely (e_shoff >= maxsize) + if (unlikely (scncnt > 0 && e_shoff >= maxsize) || unlikely (maxsize - e_shoff < scncnt * sizeof (Elf64_Shdr))) goto free_and_out; -- 1.8.3.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: failed reading "Smallest x86 ELF Hello World" 2017-10-04 18:55 ` Mark Wielaard 2017-10-04 18:57 ` [PATCH 1/2] libelf: Add ELF_E_INVALID_ELF error value Mark Wielaard @ 2017-10-13 15:09 ` Mark Wielaard 1 sibling, 0 replies; 5+ messages in thread From: Mark Wielaard @ 2017-10-13 15:09 UTC (permalink / raw) To: Josh Stone, elfutils-devel On Wed, 2017-10-04 at 20:55 +0200, Mark Wielaard wrote: > The first issue is indeed that almost anything that goes wrong when > setting up the initial Elf handle ends up being described as > ELF_E_INVALID_FILE. Which is not always the correct error code. So I > introduced ELF_E_INVALID_ELF which indicates it is bad ELF data being > encountered and not just the inability to read the data from the file > descriptor. Also in a couple of cases we didn't explicitly set the > libelf errno to indicate what really went wrong. I made sure we > always > do now. > > libelf: Add ELF_E_INVALID_ELF error value. > > [...] > > But while auditing this code it is clear we go out of our way to get > the section (count) making sure we don't touch any bad data. If there > is a change we might read anything bad from the (mmapped) file then > we > explicitly set the elf->state.elf[64|32].scns.cnt to zero. Which is > respected throughout libelf whenever we try to touch section headers. > Except... during the initial read we double check e_shoff is sane and > error out early. Even though the code right below it explicitly > doesn't > use it when scncnt is zero. So we can fix this sanity check. > > libelf: Don't error out when sanity checking e_shoff if scncnt is > zero. I pushed both these commits to master now. Cheers, Mark ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-10-13 15:09 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-09-28 19:06 failed reading "Smallest x86 ELF Hello World" Josh Stone 2017-10-04 18:55 ` Mark Wielaard 2017-10-04 18:57 ` [PATCH 1/2] libelf: Add ELF_E_INVALID_ELF error value Mark Wielaard 2017-10-04 18:57 ` [PATCH 2/2] libelf: Don't error out when sanity checking e_shoff if scncnt is zero Mark Wielaard 2017-10-13 15:09 ` failed reading "Smallest x86 ELF Hello World" Mark Wielaard
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).