| Message ID | CALoOobPKf5bP6BMt436ufCYYgAvdgQmPz4kBnrYQBn-Jc2uzcA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix BZ 20419 -- stack overflow with huge PT_NOTE segment | expand |
Ping? > 2018-04-23 Paul Pluzhnikov <ppluzhnikov@google.com> > [BZ #20419] > * elf/dl-load.c (open_verify): Fix stack overflow. > * elf/Makefile (tst-big-note): New test. > * elf/tst-big-note-lib.S: New. > * elf/tst-big-note.c: New.
On Mon, Apr 23, 2018 at 12:23 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote: > Greetings, > > Attached patch fixes BZ 20419 and adds a test case for it. > > Thanks, > > 2018-04-23 Paul Pluzhnikov <ppluzhnikov@google.com> > > [BZ #20419] > * elf/dl-load.c (open_verify): Fix stack overflow. > * elf/Makefile (tst-big-note): New test. > * elf/tst-big-note-lib.S: New. > * elf/tst-big-note.c: New. > LGTM. Thanks.
On 04/24/2018 12:53 AM, Paul Pluzhnikov wrote: > Greetings, > > Attached patch fixes BZ 20419 and adds a test case for it. Patch is OK but please add a detailed description of the problem in your git commit message; it should ideally be part of the submission. Siddhesh
On Thu, May 3, 2018 at 10:08 AM Siddhesh Poyarekar <siddhesh@gotplt.org> wrote: > Patch is OK but please add a detailed description of the problem in your > git commit message; it should ideally be part of the submission. + abi_note_malloced = abi_note = malloc (size); + if (abi_note == NULL) + goto read_error; I noticed that this could leak memory for a DSO with multiple PT_NOTEs (which LLD used to produce until that bug was fixed). Changed this part to: + /* There could be multiple PT_NOTEs. */ + abi_note_malloced = realloc (abi_note_malloced, size); + if (abi_note_malloced == NULL) + goto read_error; + + abi_note = abi_note_malloced; and committed. Thanks,
While working on the BZ 20419 patch, I noticed that this code in
elf/dl-load.c could cause similar stack overflow:
maplength = header->e_phnum * sizeof (ElfW(Phdr));
...
phdr = alloca (maplength);
An ELF binary with 131072 Phdrs is unlikely to happen in practice, except
in some kind of exploit.
Assuming we do want to protect against it (which is easy to do, so why
wouldn't we?), I am having hard time coming up with a reasonable test
strategy.
I could write a little helper program to generate such a binary "by hand"
(i.e. without compiler/linker involvement). Is there a simpler strategy?
Should I simply send a patch without a test case?
Thanks,
On 05/06/2018 11:47 PM, Paul Pluzhnikov wrote: > Assuming we do want to protect against it (which is easy to do, so why > wouldn't we?), I am having hard time coming up with a reasonable test > strategy. > > I could write a little helper program to generate such a binary "by hand" > (i.e. without compiler/linker involvement). Is there a simpler strategy? Unless the helper is more generally applicable (i.e. you have a bigger test plan) I'd say it is overkill for just this bug. That is, it would be nice to have such a helper but shouldn't be necessary for this bug. BTW on a related note, given the recent malloc tcache improvements how much does this alloca even save in terms of performance? If it is not very performance sensitive, it might make sense to just use malloc and not bother with alloca at all. Siddhesh
On 05/06/2018 08:17 PM, Paul Pluzhnikov wrote: > While working on the BZ 20419 patch, I noticed that this code in > elf/dl-load.c could cause similar stack overflow: > > maplength = header->e_phnum * sizeof (ElfW(Phdr)); > ... > phdr = alloca (maplength); > > An ELF binary with 131072 Phdrs is unlikely to happen in practice, except > in some kind of exploit. > > Assuming we do want to protect against it (which is easy to do, so why > wouldn't we?), I am having hard time coming up with a reasonable test > strategy. > > I could write a little helper program to generate such a binary "by hand" > (i.e. without compiler/linker involvement). Is there a simpler strategy? In theory, you could also manually craft a test binary and check that in. Although the test may have to patch the ELF file so that it is not rejected too early during the load processes. However, this will not work well in this particularly case because the required file size would be larger than 8 MiB if my math is correct. Thanks, Florian
On Mon, May 7, 2018 at 5:01 AM Florian Weimer <fweimer@redhat.com> wrote: > On 05/06/2018 08:17 PM, Paul Pluzhnikov wrote: > > An ELF binary with 131072 Phdrs is unlikely to happen in practice, except > > in some kind of exploit. It turns out that e_phnum is a 16-bit entity, so the max value it could have is 65535, which yields only 3.5MiB. I wrote a program to generate DSO with that many Phdr[]s, and confirmed that "ld.so --list foo.so" passes with default 8MiB stack, but crashes with anything below 6600KiB with SIGSEGV due to stack overflow. I think it's reasonable to expect "ld.so --list foo.so" to be able to run successfully with 1MiB (or even 128K) stack. My generator program is mostly machine independent, except it needs to set e_ident[EI_DATA] and e_machine appropriately. The easiest approach is to put this program and the test into sysdeps/x86_64 and make this x86_64-only test. Would that be acceptable? Alternatively, sysdeps/*/dl-machine.h could supply __ELF_NATIVE_MACHINE and __ELF_NATIVE_DATA_ENCODING (similar to __ELF_NATIVE_CLASS defined in bits/elfclass.h), which the program can then use to set e_machine etc. appropriately. Advice? Thanks,
On 05/14/2018 01:31 AM, Paul Pluzhnikov wrote: > I wrote a program to generate DSO with that many Phdr[]s, and confirmed > that "ld.so --list foo.so" passes with default 8MiB stack, but crashes with > anything below 6600KiB with SIGSEGV due to stack overflow. > > I think it's reasonable to expect "ld.so --list foo.so" to be able to run > successfully with 1MiB (or even 128K) stack. Well, if those are LOAD segments, then with tight limits you will run into VM limitations anyway. But I see your point. > My generator program is mostly machine independent, except it needs to set > e_ident[EI_DATA] and e_machine appropriately. > > The easiest approach is to put this program and the test into > sysdeps/x86_64 and make this x86_64-only test. Would that be acceptable? It should be a generic test is possible. Could you read the required values from the ELF header of the current executable? Thanks, Florian
diff --git a/elf/Makefile b/elf/Makefile index e658928305..2dcd2b88e0 100644 --- a/elf/Makefile +++ b/elf/Makefile @@ -186,7 +186,7 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \ tst-tlsalign tst-tlsalign-extern tst-nodelete-opened \ tst-nodelete2 tst-audit11 tst-audit12 tst-dlsym-error tst-noload \ tst-latepthread tst-tls-manydynamic tst-nodelete-dlclose \ - tst-debug1 tst-main1 tst-absolute-sym + tst-debug1 tst-main1 tst-absolute-sym tst-big-note # reldep9 tests-internal += loadtest unload unload2 circleload1 \ neededtest neededtest2 neededtest3 neededtest4 \ @@ -272,7 +272,9 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \ tst-audit12mod1 tst-audit12mod2 tst-audit12mod3 tst-auditmod12 \ tst-latepthreadmod $(tst-tls-many-dynamic-modules) \ tst-nodelete-dlclose-dso tst-nodelete-dlclose-plugin \ - tst-main1mod tst-libc_dlvsym-dso tst-absolute-sym-lib + tst-main1mod tst-libc_dlvsym-dso tst-absolute-sym-lib \ + tst-big-note-lib + ifeq (yes,$(have-mtls-dialect-gnu2)) tests += tst-gnu2-tls1 modules-names += tst-gnu2-tls1mod @@ -1450,3 +1452,5 @@ $(objpfx)tst-libc_dlvsym-static: $(common-objpfx)dlfcn/libdl.a tst-libc_dlvsym-static-ENV = \ LD_LIBRARY_PATH=$(objpfx):$(common-objpfx):$(common-objpfx)dlfcn $(objpfx)tst-libc_dlvsym-static.out: $(objpfx)tst-libc_dlvsym-dso.so + +$(objpfx)tst-big-note: $(objpfx)tst-big-note-lib.so diff --git a/elf/dl-load.c b/elf/dl-load.c index a5e3a25462..496b7c3cb6 100644 --- a/elf/dl-load.c +++ b/elf/dl-load.c @@ -16,6 +16,7 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ +#include <elf.h> #include <elf.h> #include <errno.h> #include <fcntl.h> @@ -1462,6 +1463,7 @@ open_verify (const char *name, int fd, ElfW(Ehdr) *ehdr; ElfW(Phdr) *phdr, *ph; ElfW(Word) *abi_note; + ElfW(Word) *abi_note_malloced = NULL; unsigned int osversion; size_t maplength; @@ -1633,10 +1635,22 @@ open_verify (const char *name, int fd, abi_note = (void *) (fbp->buf + ph->p_offset); else { - abi_note = alloca (size); + /* Note: __libc_use_alloca is not usable here, because + thread info may not have been set up yet. */ + if (size < __MAX_ALLOCA_CUTOFF) + abi_note = alloca (size); + else + { + abi_note_malloced = abi_note = malloc (size); + if (abi_note == NULL) + goto read_error; + } __lseek (fd, ph->p_offset, SEEK_SET); if (__libc_read (fd, (void *) abi_note, size) != size) - goto read_error; + { + free (abi_note_malloced); + goto read_error; + } } while (memcmp (abi_note, &expected_note, sizeof (expected_note))) @@ -1671,6 +1685,7 @@ open_verify (const char *name, int fd, break; } + free (abi_note_malloced); } return fd; diff --git a/elf/tst-big-note-lib.S b/elf/tst-big-note-lib.S new file mode 100644 index 0000000000..6b514a03cc --- /dev/null +++ b/elf/tst-big-note-lib.S @@ -0,0 +1,26 @@ +/* Bug 20419: test for stack overflow in elf/dl-load.c open_verify() + Copyright (C) 2018 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <http://www.gnu.org/licenses/>. */ + +/* This creates a .so with 8MiB PT_NOTE segment. + On a typical Linux system with 8MiB "ulimit -s", that was enough + to trigger stack overflow in open_verify. */ + +.pushsection .note.big,"a" +.balign 4 +.fill 8*1024*1024, 1, 0 +.popsection diff --git a/elf/tst-big-note.c b/elf/tst-big-note.c new file mode 100644 index 0000000000..fcd2b0ed82 --- /dev/null +++ b/elf/tst-big-note.c @@ -0,0 +1,26 @@ +/* Bug 20419: test for stack overflow in elf/dl-load.c open_verify() + Copyright (C) 2018 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <http://www.gnu.org/licenses/>. */ + +/* This file must be run from within a directory called "elf". */ + +int main (int argc, char *argv[]) +{ + /* Nothing to do here: merely linking against tst-big-note-lib.so triggers + the bug. */ + return 0; +}
Greetings, Attached patch fixes BZ 20419 and adds a test case for it. Thanks, 2018-04-23 Paul Pluzhnikov <ppluzhnikov@google.com> [BZ #20419] * elf/dl-load.c (open_verify): Fix stack overflow. * elf/Makefile (tst-big-note): New test. * elf/tst-big-note-lib.S: New. * elf/tst-big-note.c: New.