Hi,
if you run jankratochvil/gdbserver as: ./gdbserver 1234 corefile
without this change it prints: ./gdbserver: Error reading note at core file offset 0x468
due to:
81 if (unlikely (size > elf->maximum_size 82 || (off64_t) (elf->maximum_size - size) < offset)) (gdb) p size > elf->maximum_size $6 = 0 (gdb) p (off64_t) (elf->maximum_size - size) < offset $7 = 1 (gdb) p offset $10 = 1128 (gdb) p (off64_t) (elf->maximum_size - size) $8 = -1369 (gdb) p elf->maximum_size - size $9 = 18446744073709550247 (gdb) p/x elf->maximum_size $11 = 0xffffffffffffffff
In fact I believe Elf->maximum_size should be off64_t and not size_t as this way 32bit hosts cannot handle >2GB ELF files, can they? But as 32bit hosts are dead I did not try to do the more invasive change.
OK to merge to master? It is now in <jankratochvil/pending>.
No regressions on {x86_64,i686}-fedora13-linux-gnu.
Thanks, Jan
libelf/ 2010-12-19 Jan Kratochvil jan.kratochvil@redhat.com
* elf_getdata_rawchunk.c (elf_getdata_rawchunk): Fix off64_t overflow when MAXIMUM_SIZE == ~0.
--- a/libelf/elf_getdata_rawchunk.c +++ b/libelf/elf_getdata_rawchunk.c @@ -78,8 +78,8 @@ elf_getdata_rawchunk (elf, offset, size, type) return NULL; }
- if (unlikely (size > elf->maximum_size - || (off64_t) (elf->maximum_size - size) < offset)) + if (unlikely (offset < 0 || offset + (off64_t) size < offset + || offset + size > elf->maximum_size)) { /* Invalid request. */ __libelf_seterrno (ELF_E_INVALID_OP);
In fact I believe Elf->maximum_size should be off64_t and not size_t as this way 32bit hosts cannot handle >2GB ELF files, can they?
Yes, I guess it should change. In most places maximum_size is only used for the size that's mapped in, which by definition must fit in size_t. But there are a few places like this one where it's used for the size of the file when not mapped too. So it looks like it should be able to hold anything st_size could be.
But as 32bit hosts are dead I did not try to do the more invasive change.
:-) They are not dead enough to ignore this kind of problem yet.
OK to merge to master? It is now in <jankratochvil/pending>.
Yes, that looks fine. Someone should do the more extensive change, including an audit of all the uses of maximum_size. Would you like to do it?
Thanks, Roland
On Tue, 04 Jan 2011 19:03:20 +0100, Roland McGrath wrote:
OK to merge to master? It is now in <jankratochvil/pending>.
Yes, that looks fine.
Checked in: f1ccc410d6bda15ed49fc5d0fef9f6f65acbfb63
In fact I believe Elf->maximum_size should be off64_t and not size_t as this way 32bit hosts cannot handle >2GB ELF files, can they?
[...]
Someone should do the more extensive change, including an audit of all the uses of maximum_size. Would you like to do it?
FYI processing a WIP patch using a patch pinpointing the dangerous spots by:
typedef struct { loff_t x; } maxs_t; #define MAXS(x) ({ maxs_t maxs = { x }; maxs; })
and doing s/(size_t|off_t|loff_t)/maxs_t/ for final easy s/maxs_t/loff_t/. Just it is a heavy change requiring massive to-be removed modifications like:
- update_if_changed (ehdr->e_shoff, (GElf_Word) size, elf->flags);$ + update_if_changed (ehdr->e_shoff, (GElf_Word) size.x, elf->flags);$
missing standard type of unsigned loff_t to avoid signed/unsigned comparisons:
- if (ehdr.e64->e_shoff > maxsize + if (ehdr.e64->e_shoff > (unsigned long long) maxsize.x
and real changes like:
/* We try to map the file ourself. */$ - map_address = mmap (NULL, maxsize, (cmd == ELF_C_READ_MMAP$ - ? PROT_READ$ - : PROT_READ|PROT_WRITE),$ - cmd == ELF_C_READ_MMAP_PRIVATE$ - || cmd == ELF_C_READ_MMAP$ - ? MAP_PRIVATE : MAP_SHARED,$ - fildes, offset);$ + if (maxsize.x <= ~((size_t) 0))$ + map_address = mmap (NULL, maxsize.x, (cmd == ELF_C_READ_MMAP$ + ? PROT_READ$ + : PROT_READ|PROT_WRITE),$ + cmd == ELF_C_READ_MMAP_PRIVATE$ + || cmd == ELF_C_READ_MMAP$ + ? MAP_PRIVATE : MAP_SHARED,$ + fildes, offset.x);$ + else$ + map_address = MAP_FAILED;$
off_t and loff_t should be always the same due to AC_SYS_LARGEFILE in use. Therefore also unifying s/(off_t|loff_t)/loff_t/ as at least in public elfutils headers apps using elfutils-libs may not be using AC_SYS_LARGEFILE.
Thanks, Jan
On Tue, 04 Jan 2011 19:03:20 +0100, Roland McGrath wrote:
OK to merge to master? It is now in <jankratochvil/pending>.
Yes, that looks fine.
Checked in: f1ccc410d6bda15ed49fc5d0fef9f6f65acbfb63
Thanks.
off_t and loff_t should be always the same due to AC_SYS_LARGEFILE in use. Therefore also unifying s/(off_t|loff_t)/loff_t/ as at least in public elfutils headers apps using elfutils-libs may not be using AC_SYS_LARGEFILE.
Right. The only appearances of *off_t in the public API are in libelf.h, and those are already loff_t because of exactly this issue. But it would indeed be clearer to use loff_t uniformly in all the source even though off_t always means the same thing in the internal code because we compile it with -D_FILE_OFFSET_BITS=64.
Thanks, Roland
elfutils-devel@lists.fedorahosted.org