| Message ID | CALoOobMmpZyrAsXqKrLuvO+c1Et88Cq6Kb2rY8N76NA5ihmCYA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix BZ 23187 -- stack overflow for many Phdr[]s | expand |
On 21/05/2018 17:34, Paul Pluzhnikov wrote: > Greetings, > > Attached patch fixes several instances of unconstrained stack allocation, > which causes ld.so to overflow stack and crash while processing DSO with > 1000s of Phdr[]s, and adds a test case. > > 2018-05-21 Paul Pluzhnikov <ppluzhnikov@google.com> > > [BZ #23187] > * elf/Makefile (tst-many-phdrs): New test. > * elf/tst-many-phdrs.c: New. > * elf/dl-load.c (_dl_map_object_from_fd): Constrain stack > allocation. Looks like the CL is mangled somehow, 'allocation' should be aligned with tab. > @@ -960,7 +966,18 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, > phdr = (void *) (fbp->buf + header->e_phoff); > else > { > - phdr = alloca (maplength); > + if (maplength < __MAX_ALLOCA_CUTOFF) > + phdr = alloca (maplength); > + else > + { > + phdr_malloced = malloc (maplength); > + if (phdr_malloced == NULL) > + { > + errstring = N_("cannot allocate memory for program header"); > + goto call_lose_errno; > + } > + phdr = phdr_malloced; > + } I think we should avoid use alloca in newer code in favor of current framework that tries to leverage this kind of behaviour (initial stack allocation with potential heap usage). They also have the advantage of avoid using __MAX_ALLOCA_CUTOFF, which is kind arbitrary and do not really address total stack usage over multiple function calls. What about use dynarray for this as below. The initial sizes are still arbitraty and should cover most common cases, but I do not have a strong opinion here (they have total stack usage of 384 bytes for loadcmd_list and 896 for phdr_list on 64 bits architectures). --- diff --git a/elf/dl-load.c b/elf/dl-load.c index 4312369..807cba6 100644 --- a/elf/dl-load.c +++ b/elf/dl-load.c @@ -792,6 +792,17 @@ lose (int code, int fd, const char *name, char *realname, struct link_map *l, _dl_signal_error (code, name, NULL, msg); } +#define DYNARRAY_STRUCT loadcmd_list +#define DYNARRAY_ELEMENT struct loadcmd +#define DYNARRAY_PREFIX loadcmd_list_ +#define DYNARRAY_INITIAL_SIZE 8 +#include <malloc/dynarray-skeleton.c> + +#define DYNARRAY_STRUCT phdr_list +#define DYNARRAY_ELEMENT ElfW(Phdr) +#define DYNARRAY_PREFIX phdr_list_ +#define DYNARRAY_INITIAL_SIZE 16 +#include <malloc/dynarray-skeleton.c> /* Map in the shared object NAME, actually located in REALNAME, and already opened on FD. */ @@ -809,6 +820,8 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, const ElfW(Ehdr) *header; const ElfW(Phdr) *phdr; const ElfW(Phdr) *ph; + struct loadcmd_list loadcmds; + struct phdr_list phdrlist; size_t maplength; int type; /* Initialize to keep the compiler happy. */ @@ -817,6 +830,9 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, struct r_debug *r = _dl_debug_initialize (0, nsid); bool make_consistent = false; + loadcmd_list_init (&loadcmds); + phdr_list_init (&phdrlist); + /* Get file information. */ struct r_file_id id; if (__glibc_unlikely (!_dl_get_file_id (fd, &id))) @@ -827,6 +843,8 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, call_lose: lose (errval, fd, name, realname, l, errstring, make_consistent ? r : NULL, nsid); + phdr_list_free (&phdrlist); + loadcmd_list_free (&loadcmds); } /* Look again to see if the real name matched another already loaded. */ @@ -960,7 +978,13 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, phdr = (void *) (fbp->buf + header->e_phoff); else { - phdr = alloca (maplength); + if (!phdr_list_resize (&phdrlist, header->e_phnum)) + { + errstring = N_("cannot allocate memory for program header"); + goto call_lose_errno; + } + phdr = phdr_list_begin (&phdrlist); + __lseek (fd, header->e_phoff, SEEK_SET); if ((size_t) __libc_read (fd, (void *) phdr, maplength) != maplength) { @@ -976,7 +1000,11 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, { /* Scan the program header table, collecting its load commands. */ - struct loadcmd loadcmds[l->l_phnum]; + if (!loadcmd_list_resize (&loadcmds, l->l_phnum)) + { + errstring = N_("cannot allocate memory for program header"); + goto call_lose_errno; + } size_t nloadcmds = 0; bool has_holes = false; @@ -1021,7 +1049,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, goto call_lose; } - struct loadcmd *c = &loadcmds[nloadcmds++]; + struct loadcmd *c = loadcmd_list_at (&loadcmds, nloadcmds++); c->mapstart = ALIGN_DOWN (ph->p_vaddr, GLRO(dl_pagesize)); c->mapend = ALIGN_UP (ph->p_vaddr + ph->p_filesz, GLRO(dl_pagesize)); c->dataend = ph->p_vaddr + ph->p_filesz; @@ -1116,16 +1144,21 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, } /* Length of the sections to be loaded. */ - maplength = loadcmds[nloadcmds - 1].allocend - loadcmds[0].mapstart; + const struct loadcmd *lc = loadcmd_list_at (&loadcmds, nloadcmds - 1); + const struct loadcmd *l0 = loadcmd_list_begin (&loadcmds); + maplength = lc->allocend - l0->mapstart; /* Now process the load commands and map segments into memory. This is responsible for filling in: l_map_start, l_map_end, l_addr, l_contiguous, l_text_end, l_phdr */ - errstring = _dl_map_segments (l, fd, header, type, loadcmds, nloadcmds, + errstring = _dl_map_segments (l, fd, header, type, l0, nloadcmds, maplength, has_holes, loader); if (__glibc_unlikely (errstring != NULL)) goto call_lose; + + phdr_list_free (&phdrlist); + loadcmd_list_free (&loadcmds); } if (l->l_ld == 0) @@ -1463,9 +1496,12 @@ open_verify (const char *name, int fd, ElfW(Phdr) *phdr, *ph; ElfW(Word) *abi_note; ElfW(Word) *abi_note_malloced = NULL; + struct phdr_list phdrlist; unsigned int osversion; size_t maplength; + phdr_list_init (&phdrlist); + /* We successfully opened the file. Now verify it is a file we can use. */ __set_errno (0); @@ -1596,7 +1632,13 @@ open_verify (const char *name, int fd, phdr = (void *) (fbp->buf + ehdr->e_phoff); else { - phdr = alloca (maplength); + if (!phdr_list_resize (&phdrlist, ehdr->e_phnum)) + { + errstring = N_("cannot allocate memory for program header"); + goto call_lose; + } + phdr = phdr_list_begin (&phdrlist); + __lseek (fd, ehdr->e_phoff, SEEK_SET); if ((size_t) __libc_read (fd, (void *) phdr, maplength) != maplength) { @@ -1687,6 +1729,7 @@ open_verify (const char *name, int fd, break; } + phdr_list_free (&phdrlist); free (abi_note_malloced); } diff --git a/elf/dl-minimal.c b/elf/dl-minimal.c index 25ceded..943a960 100644 --- a/elf/dl-minimal.c +++ b/elf/dl-minimal.c @@ -28,6 +28,7 @@ #include <ldsodefs.h> #include <_itoa.h> #include <malloc/malloc-internal.h> +#include <malloc/dynarray.h> #include <assert.h> @@ -191,6 +192,15 @@ __libc_fatal (const char *message) rtld_hidden_def (__libc_fatal) void +__libc_dynarray_at_failure (size_t size, size_t index) +{ + _dl_fatal_printf ("Fatal glibc error: " + "array index %zu not less than array length %zu\n", + index, size); +} +rtld_hidden_def (__libc_dynarray_at_failure) + +void __attribute__ ((noreturn)) __chk_fail (void) { diff --git a/malloc/dynarray.h b/malloc/dynarray.h index 0b171da..c4df88b 100644 --- a/malloc/dynarray.h +++ b/malloc/dynarray.h @@ -174,6 +174,7 @@ libc_hidden_proto (__libc_dynarray_resize) libc_hidden_proto (__libc_dynarray_resize_clear) libc_hidden_proto (__libc_dynarray_finalize) libc_hidden_proto (__libc_dynarray_at_failure) +rtld_hidden_proto (__libc_dynarray_at_failure) #endif #endif /* _DYNARRAY_H */ --- > __lseek (fd, header->e_phoff, SEEK_SET); > if ((size_t) __libc_read (fd, (void *) phdr, maplength) != maplength) > { > @@ -976,10 +993,24 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, > > { > /* Scan the program header table, collecting its load commands. */ > - struct loadcmd loadcmds[l->l_phnum]; > + struct loadcmd *loadcmds; > size_t nloadcmds = 0; > + const size_t loadcmds_size = sizeof (*loadcmds) * l->l_phnum; > bool has_holes = false; > > + if (loadcmds_size < __MAX_ALLOCA_CUTOFF) > + loadcmds = alloca (loadcmds_size); > + else > + { > + loadcmds_malloced = malloc (loadcmds_size); > + if (loadcmds_malloced == NULL) > + { > + errstring = N_("cannot allocate memory for program header"); > + goto call_lose_errno; > + } > + loadcmds = loadcmds_malloced; > + } > + > /* The struct is initialized to zero so this is not necessary: > l->l_ld = 0; > l->l_phdr = 0; > @@ -1126,6 +1157,11 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, > maplength, has_holes, loader); > if (__glibc_unlikely (errstring != NULL)) > goto call_lose; > + > + free (phdr_malloced); > + free (loadcmds_malloced); > + phdr_malloced = NULL; > + loadcmds_malloced = NULL; > } > > if (l->l_ld == 0) > @@ -1460,7 +1496,7 @@ open_verify (const char *name, int fd, > if (fd != -1) > { > ElfW(Ehdr) *ehdr; > - ElfW(Phdr) *phdr, *ph; > + ElfW(Phdr) *phdr, *ph, *phdr_malloced = NULL; > ElfW(Word) *abi_note; > ElfW(Word) *abi_note_malloced = NULL; > unsigned int osversion; > @@ -1596,7 +1632,18 @@ open_verify (const char *name, int fd, > phdr = (void *) (fbp->buf + ehdr->e_phoff); > else > { > - phdr = alloca (maplength); > + if (maplength < __MAX_ALLOCA_CUTOFF) > + phdr = alloca (maplength); > + else > + { > + phdr_malloced = malloc (maplength); > + if (phdr_malloced == NULL) > + { > + errstring = N_("cannot allocate memory for program header"); > + goto call_lose; > + } > + phdr = phdr_malloced; > + } > __lseek (fd, ehdr->e_phoff, SEEK_SET); > if ((size_t) __libc_read (fd, (void *) phdr, maplength) != maplength) > { > @@ -1687,6 +1734,7 @@ open_verify (const char *name, int fd, > > break; > } > + free (phdr_malloced); > free (abi_note_malloced); > } > > diff --git a/elf/tst-many-phdrs.c b/elf/tst-many-phdrs.c > new file mode 100644 > index 0000000000..ea8b0a4ec5 > --- /dev/null > +++ b/elf/tst-many-phdrs.c > @@ -0,0 +1,170 @@ > +/* Copyright (C) 2018 Free Software Foundation, Inc. Missing initial descriptive line. > + 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/>. */ > + > +/* A test that generates DSO with 65535 Phdr[]s, and asks ld.so > + to list its dependencies (after adjusting stack limits). > + > + This is used to test for stack overflow in dl-load.c */ > + > +#include <elf.h> > +#include <limits.h> > +#include <link.h> > +#include <stdio.h> > +#include <stdlib.h> > +#include <string.h> > +#include <unistd.h> > +#include <sys/resource.h> > +#include <support/check.h> > + > +static const char * > +get_ld_so (void) > +{ > + struct link_map *lm = _r_debug.r_map; > + while (lm != NULL) > + { > + if (lm->l_name != NULL && strstr (lm->l_name, "elf/ld") != NULL) > + return lm->l_name; > + lm = lm->l_next; > + } > + FAIL_EXIT1 ("Did not find ld.so"); > +} > + > +static const char * > +write_dso (void) > +{ > + ElfW(Ehdr) ehdr; > + ElfW(Phdr) phdr; > + ElfW(Dyn) dyn; > + const char *const ld_so = get_ld_so (); > + > + FILE *fp = fopen (ld_so, "rb"); > + if (fp == NULL) > + FAIL_EXIT1 ("fopen %s: %m", ld_so); > + > + /* Use Ehdr from ld.so so we don't have to depend on specific e_machine > + and e_ident[EI_DATA] -- we simply use the same values as ld.so. */ > + if (fread (&ehdr, sizeof (ehdr), 1, fp) != 1) > + FAIL_EXIT1 ("fread: %m"); > + fclose (fp); > + > + static char fname[] = "/tmp/tst-many-phdrs-XXXXXX"; > + int fd = mkstemp (fname); > + if (fd == -1) > + FAIL_EXIT1 ("mkstemp failed: %m"); > + > + fp = fdopen (fd, "wb"); > + if (fp == NULL) > + FAIL_EXIT1 ("fdopen failed: %m"); > + > + ehdr.e_phoff = sizeof (ehdr) + sizeof (dyn); > + ehdr.e_phentsize = sizeof (phdr); > + ehdr.e_phnum = 0xFFFF; /* Max possible number of Phdr[]s. */ > + > + if (1 != fwrite (&ehdr, sizeof (ehdr), 1, fp)) > + FAIL_EXIT1 ("fwrite ehdr: %m"); > + > + /* Write a single Dyn DT_NUL entry. That isn't enough to make this a > + valid DSO (dynstr and dynsym and hash are also necessary), but is enough > + to trigger stack overflow when listing its dependencies. */ > + memset (&dyn, 0, sizeof (dyn)); > + if (1 != fwrite (&dyn, sizeof (dyn), 1, fp)) > + FAIL_EXIT1 ("fwrite dyn: %m"); > + > + /* Add PT_DYNAMIC pointing to the Dyn entry we just wrote. */ > + memset (&phdr, 0, sizeof (phdr)); > + phdr.p_offset = sizeof (ehdr); > + phdr.p_vaddr = sizeof (ehdr); > + phdr.p_paddr = sizeof (ehdr); > + phdr.p_filesz = sizeof (dyn); > + phdr.p_memsz = sizeof (dyn); > + phdr.p_flags = PF_R; > + phdr.p_type = PT_DYNAMIC; > + > + if (1 != fwrite (&phdr, sizeof (phdr), 1, fp)) > + FAIL_EXIT1 ("fwrite phdr: %m"); > + > + /* Now the rest of Phdr[]s. */ > + memset (&phdr, 0, sizeof (phdr)); > + phdr.p_offset = 0; > + phdr.p_filesz = sizeof (ehdr); > + phdr.p_memsz = sizeof (ehdr); > + phdr.p_flags = PF_R; > + > + for (int j = 1; j < ehdr.e_phnum; j++) > + { > + /* At least one PT_LOAD is required. */ > + phdr.p_type = j == 1 ? PT_LOAD : PT_NOTE; > + if (1 != fwrite (&phdr, sizeof (phdr), 1, fp)) > + FAIL_EXIT1 ("fwrite phdr: %m"); > + } > + > + fclose (fp); > + return fname; > +} > + > +static int > +do_test (int argc, char *argv[]) > +{ > + const char *const dso = write_dso (); > + const char *const ld_so = get_ld_so (); > + struct rlimit rl; > + > + if (getrlimit (RLIMIT_STACK, &rl) == -1) > + FAIL_EXIT1 ("getrlimit (RLIMIT_STACK): %m"); > + > + char cmd[4 * 4096]; > + int n = snprintf (cmd, sizeof (cmd), "%s --list %s", ld_so, dso); > + if (n >= sizeof (cmd)) > + FAIL_EXIT1 ("buffer too small: need %d bytes", n); > + > + printf ("cmd: %s\n", cmd); > + > + if (rl.rlim_cur == RLIM_INFINITY) > + { > + printf ("Capping stack at 8MiB\n"); > + rl.rlim_cur = 8 << 20; > + } > + > + while (rl.rlim_cur >= 128 * 1024) > + { > + printf ("Stack size: %zu\n", (size_t) rl.rlim_cur); > + int rc = system (cmd); > + > + if (rc != 0) > + { > + /* Clean up. Comment this out to debug ld.so. */ > + unlink (dso); > + > + if (WIFEXITED (rc)) > + FAIL_EXIT1 ("child exited with %d", WEXITSTATUS (rc)); > + else if (WIFSIGNALED (rc)) > + FAIL_EXIT1 ("child exited with signal %d", WTERMSIG (rc)); > + else > + FAIL_EXIT1 ("child exited with rc %d", rc); > + } > + > + rl.rlim_cur /= 2; > + if (setrlimit (RLIMIT_STACK, &rl) == -1) > + FAIL_EXIT1 ("setrlimit: %m"); > + } > + > + unlink (dso); > + return 0; > +} > + > +#define TEST_FUNCTION_ARGV do_test > +#include <support/test-driver.c> >
On 05/22/2018 03:49 PM, Adhemerval Zanella wrote: > > > On 21/05/2018 17:34, Paul Pluzhnikov wrote: >> Greetings, >> >> Attached patch fixes several instances of unconstrained stack allocation, >> which causes ld.so to overflow stack and crash while processing DSO with >> 1000s of Phdr[]s, and adds a test case. >> >> 2018-05-21 Paul Pluzhnikov <ppluzhnikov@google.com> >> >> [BZ #23187] >> * elf/Makefile (tst-many-phdrs): New test. >> * elf/tst-many-phdrs.c: New. >> * elf/dl-load.c (_dl_map_object_from_fd): Constrain stack >> allocation. > > Looks like the CL is mangled somehow, 'allocation' should be aligned with tab. > >> @@ -960,7 +966,18 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, >> phdr = (void *) (fbp->buf + header->e_phoff); >> else >> { >> - phdr = alloca (maplength); >> + if (maplength < __MAX_ALLOCA_CUTOFF) >> + phdr = alloca (maplength); >> + else >> + { >> + phdr_malloced = malloc (maplength); >> + if (phdr_malloced == NULL) >> + { >> + errstring = N_("cannot allocate memory for program header"); >> + goto call_lose_errno; >> + } >> + phdr = phdr_malloced; >> + } > > I think we should avoid use alloca in newer code in favor of current > framework that tries to leverage this kind of behaviour (initial stack > allocation with potential heap usage). They also have the advantage of > avoid using __MAX_ALLOCA_CUTOFF, which is kind arbitrary and do not > really address total stack usage over multiple function calls. > > What about use dynarray for this as below. The initial sizes are still > arbitraty and should cover most common cases, but I do not have a strong > opinion here (they have total stack usage of 384 bytes for loadcmd_list > and 896 for phdr_list on 64 bits architectures). > +1. Should we use a scratch buffer? Which has constant size and can be expanded only if required?
* Adhemerval Zanella: > What about use dynarray for this as below. The initial sizes are still > arbitraty and should cover most common cases, but I do not have a strong > opinion here (they have total stack usage of 384 bytes for loadcmd_list > and 896 for phdr_list on 64 bits architectures). dynarray is not compatible with the dl-minimal.c malloc implementation. realloc in particular contains this: assert (ptr == alloc_last_block);
On 22/05/2018 17:25, Florian Weimer wrote: > * Adhemerval Zanella: > >> What about use dynarray for this as below. The initial sizes are still >> arbitraty and should cover most common cases, but I do not have a strong >> opinion here (they have total stack usage of 384 bytes for loadcmd_list >> and 896 for phdr_list on 64 bits architectures). > > dynarray is not compatible with the dl-minimal.c malloc implementation. > realloc in particular contains this: > > assert (ptr == alloc_last_block); > At least for this specific usage where only there is only DYARRAY_resize operation it works (no regression on glibc testcase), although I am not sure it is the best way to accomplish it. How hard would adapt dl-minimal.c malloc to work with dynarray? > > Should we use a scratch buffer? Which has constant size and can be expanded > only if required? The advantage of dynarray is it provides a slight better typed api with ties better a list/array abstraction, but for this usage I think a scratch_buffer is doable.
* Adhemerval Zanella: > At least for this specific usage where only there is only DYARRAY_resize > operation it works (no regression on glibc testcase), although I am not > sure it is the best way to accomplish it. How hard would adapt dl-minimal.c > malloc to work with dynarray? DJ posted a patch: https://patchwork.sourceware.org/patch/21325/ We have some disagreement regarding chunk coalescing, but this alone probably should not block acceptance of the patch. My original use case evaporated when I realized that we can't use the dl-minimal malloc during relocation, at least not under the malloc/free names because relocation itself changes which functions are called. >> Should we use a scratch buffer? Which has constant size and can be expanded >> only if required? > > The advantage of dynarray is it provides a slight better typed api with > ties better a list/array abstraction, but for this usage I think a > scratch_buffer is doable. Actually, I want to get rid of that part of the scratch buffer interface (the array handling part and scratch_buffer_set_array_size). It's redundant with dynarray. I think I posted a couple of cleanups which remove the last remaining use.
On 22/05/2018 19:51, Florian Weimer wrote: > * Adhemerval Zanella: > >> At least for this specific usage where only there is only DYARRAY_resize >> operation it works (no regression on glibc testcase), although I am not >> sure it is the best way to accomplish it. How hard would adapt dl-minimal.c >> malloc to work with dynarray? > > DJ posted a patch: > > https://patchwork.sourceware.org/patch/21325/ > > We have some disagreement regarding chunk coalescing, but this alone > probably should not block acceptance of the patch. > > My original use case evaporated when I realized that we can't use the > dl-minimal malloc during relocation, at least not under the > malloc/free names because relocation itself changes which functions > are called. I will review this patch and I think we can proceed once it is upstream (I take it should not have any real blocker based on your message). > >>> Should we use a scratch buffer? Which has constant size and can be expanded >>> only if required? >> >> The advantage of dynarray is it provides a slight better typed api with >> ties better a list/array abstraction, but for this usage I think a >> scratch_buffer is doable. > > Actually, I want to get rid of that part of the scratch buffer > interface (the array handling part and scratch_buffer_set_array_size). > It's redundant with dynarray. I think I posted a couple of cleanups > which remove the last remaining use. > Agreed.
On Fri, May 25, 2018 at 1:05 PM Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > On 22/05/2018 19:51, Florian Weimer wrote: > > * Adhemerval Zanella: > > > >> At least for this specific usage where only there is only DYARRAY_resize > >> operation it works (no regression on glibc testcase), although I am not > >> sure it is the best way to accomplish it. How hard would adapt dl-minimal.c > >> malloc to work with dynarray? > > > > DJ posted a patch: > > > > https://patchwork.sourceware.org/patch/21325/ > > > > We have some disagreement regarding chunk coalescing, but this alone > > probably should not block acceptance of the patch. > > > > My original use case evaporated when I realized that we can't use the > > dl-minimal malloc during relocation, at least not under the > > malloc/free names because relocation itself changes which functions > > are called. > > I will review this patch and I think we can proceed once it is upstream > (I take it should not have any real blocker based on your message). I am sorry, but I got very confused by the status of this thread/patch. I think all agree that using dynarray would be much better here. Am I therefore to wait for DJ's patch to land? The discussion on it appears to have stopped last August. I don't think there is any particular hurry to handle this (BZ 23187, very abnormal) test case, but I'd like to have a completion time horizon that is not measured in years :-) Thanks!
On Sat, Jun 2, 2018 at 1:30 PM Paul Pluzhnikov <ppluzhnikov@google.com> wrote: > > On Fri, May 25, 2018 at 1:05 PM Adhemerval Zanella > <adhemerval.zanella@linaro.org> wrote: > > > On 22/05/2018 19:51, Florian Weimer wrote: > > I will review this patch and I think we can proceed once it is upstream > > (I take it should not have any real blocker based on your message). > Am I therefore to wait for DJ's patch to land? The discussion on it > appears to have stopped last August. > > I don't think there is any particular hurry to handle this (BZ 23187, > very abnormal) test case, but I'd like to have a completion time > horizon that is not measured in years :-) It's now 4 months later, and there is still (AFAICT) no movement on DJ's patch :-( Is waiting for it to land still the right approach? Thanks,
diff --git a/elf/Makefile b/elf/Makefile index 2dcd2b88e0..eeaecd4f30 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-big-note + tst-debug1 tst-main1 tst-absolute-sym tst-big-note tst-many-phdrs # reldep9 tests-internal += loadtest unload unload2 circleload1 \ neededtest neededtest2 neededtest3 neededtest4 \ diff --git a/elf/dl-load.c b/elf/dl-load.c index 431236920f..a551d97b43 100644 --- a/elf/dl-load.c +++ b/elf/dl-load.c @@ -809,6 +809,8 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, const ElfW(Ehdr) *header; const ElfW(Phdr) *phdr; const ElfW(Phdr) *ph; + ElfW(Phdr) *phdr_malloced = NULL; + struct loadcmd *loadcmds_malloced = NULL; size_t maplength; int type; /* Initialize to keep the compiler happy. */ @@ -827,6 +829,10 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, call_lose: lose (errval, fd, name, realname, l, errstring, make_consistent ? r : NULL, nsid); + free (phdr_malloced); + free (loadcmds_malloced); + phdr_malloced = NULL; + loadcmds_malloced = NULL; } /* Look again to see if the real name matched another already loaded. */ @@ -960,7 +966,18 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, phdr = (void *) (fbp->buf + header->e_phoff); else { - phdr = alloca (maplength); + if (maplength < __MAX_ALLOCA_CUTOFF) + phdr = alloca (maplength); + else + { + phdr_malloced = malloc (maplength); + if (phdr_malloced == NULL) + { + errstring = N_("cannot allocate memory for program header"); + goto call_lose_errno; + } + phdr = phdr_malloced; + } __lseek (fd, header->e_phoff, SEEK_SET); if ((size_t) __libc_read (fd, (void *) phdr, maplength) != maplength) { @@ -976,10 +993,24 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, { /* Scan the program header table, collecting its load commands. */ - struct loadcmd loadcmds[l->l_phnum]; + struct loadcmd *loadcmds; size_t nloadcmds = 0; + const size_t loadcmds_size = sizeof (*loadcmds) * l->l_phnum; bool has_holes = false; + if (loadcmds_size < __MAX_ALLOCA_CUTOFF) + loadcmds = alloca (loadcmds_size); + else + { + loadcmds_malloced = malloc (loadcmds_size); + if (loadcmds_malloced == NULL) + { + errstring = N_("cannot allocate memory for program header"); + goto call_lose_errno; + } + loadcmds = loadcmds_malloced; + } + /* The struct is initialized to zero so this is not necessary: l->l_ld = 0; l->l_phdr = 0; @@ -1126,6 +1157,11 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, maplength, has_holes, loader); if (__glibc_unlikely (errstring != NULL)) goto call_lose; + + free (phdr_malloced); + free (loadcmds_malloced); + phdr_malloced = NULL; + loadcmds_malloced = NULL; } if (l->l_ld == 0) @@ -1460,7 +1496,7 @@ open_verify (const char *name, int fd, if (fd != -1) { ElfW(Ehdr) *ehdr; - ElfW(Phdr) *phdr, *ph; + ElfW(Phdr) *phdr, *ph, *phdr_malloced = NULL; ElfW(Word) *abi_note; ElfW(Word) *abi_note_malloced = NULL; unsigned int osversion; @@ -1596,7 +1632,18 @@ open_verify (const char *name, int fd, phdr = (void *) (fbp->buf + ehdr->e_phoff); else { - phdr = alloca (maplength); + if (maplength < __MAX_ALLOCA_CUTOFF) + phdr = alloca (maplength); + else + { + phdr_malloced = malloc (maplength); + if (phdr_malloced == NULL) + { + errstring = N_("cannot allocate memory for program header"); + goto call_lose; + } + phdr = phdr_malloced; + } __lseek (fd, ehdr->e_phoff, SEEK_SET); if ((size_t) __libc_read (fd, (void *) phdr, maplength) != maplength) { @@ -1687,6 +1734,7 @@ open_verify (const char *name, int fd, break; } + free (phdr_malloced); free (abi_note_malloced); } diff --git a/elf/tst-many-phdrs.c b/elf/tst-many-phdrs.c new file mode 100644 index 0000000000..ea8b0a4ec5 --- /dev/null +++ b/elf/tst-many-phdrs.c @@ -0,0 +1,170 @@ +/* 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/>. */ + +/* A test that generates DSO with 65535 Phdr[]s, and asks ld.so + to list its dependencies (after adjusting stack limits). + + This is used to test for stack overflow in dl-load.c */ + +#include <elf.h> +#include <limits.h> +#include <link.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <unistd.h> +#include <sys/resource.h> +#include <support/check.h> + +static const char * +get_ld_so (void) +{ + struct link_map *lm = _r_debug.r_map; + while (lm != NULL) + { + if (lm->l_name != NULL && strstr (lm->l_name, "elf/ld") != NULL) + return lm->l_name; + lm = lm->l_next; + } + FAIL_EXIT1 ("Did not find ld.so"); +} + +static const char * +write_dso (void) +{ + ElfW(Ehdr) ehdr; + ElfW(Phdr) phdr; + ElfW(Dyn) dyn; + const char *const ld_so = get_ld_so (); + + FILE *fp = fopen (ld_so, "rb"); + if (fp == NULL) + FAIL_EXIT1 ("fopen %s: %m", ld_so); + + /* Use Ehdr from ld.so so we don't have to depend on specific e_machine + and e_ident[EI_DATA] -- we simply use the same values as ld.so. */ + if (fread (&ehdr, sizeof (ehdr), 1, fp) != 1) + FAIL_EXIT1 ("fread: %m"); + fclose (fp); + + static char fname[] = "/tmp/tst-many-phdrs-XXXXXX"; + int fd = mkstemp (fname); + if (fd == -1) + FAIL_EXIT1 ("mkstemp failed: %m"); + + fp = fdopen (fd, "wb"); + if (fp == NULL) + FAIL_EXIT1 ("fdopen failed: %m"); + + ehdr.e_phoff = sizeof (ehdr) + sizeof (dyn); + ehdr.e_phentsize = sizeof (phdr); + ehdr.e_phnum = 0xFFFF; /* Max possible number of Phdr[]s. */ + + if (1 != fwrite (&ehdr, sizeof (ehdr), 1, fp)) + FAIL_EXIT1 ("fwrite ehdr: %m"); + + /* Write a single Dyn DT_NUL entry. That isn't enough to make this a + valid DSO (dynstr and dynsym and hash are also necessary), but is enough + to trigger stack overflow when listing its dependencies. */ + memset (&dyn, 0, sizeof (dyn)); + if (1 != fwrite (&dyn, sizeof (dyn), 1, fp)) + FAIL_EXIT1 ("fwrite dyn: %m"); + + /* Add PT_DYNAMIC pointing to the Dyn entry we just wrote. */ + memset (&phdr, 0, sizeof (phdr)); + phdr.p_offset = sizeof (ehdr); + phdr.p_vaddr = sizeof (ehdr); + phdr.p_paddr = sizeof (ehdr); + phdr.p_filesz = sizeof (dyn); + phdr.p_memsz = sizeof (dyn); + phdr.p_flags = PF_R; + phdr.p_type = PT_DYNAMIC; + + if (1 != fwrite (&phdr, sizeof (phdr), 1, fp)) + FAIL_EXIT1 ("fwrite phdr: %m"); + + /* Now the rest of Phdr[]s. */ + memset (&phdr, 0, sizeof (phdr)); + phdr.p_offset = 0; + phdr.p_filesz = sizeof (ehdr); + phdr.p_memsz = sizeof (ehdr); + phdr.p_flags = PF_R; + + for (int j = 1; j < ehdr.e_phnum; j++) + { + /* At least one PT_LOAD is required. */ + phdr.p_type = j == 1 ? PT_LOAD : PT_NOTE; + if (1 != fwrite (&phdr, sizeof (phdr), 1, fp)) + FAIL_EXIT1 ("fwrite phdr: %m"); + } + + fclose (fp); + return fname; +} + +static int +do_test (int argc, char *argv[]) +{ + const char *const dso = write_dso (); + const char *const ld_so = get_ld_so (); + struct rlimit rl; + + if (getrlimit (RLIMIT_STACK, &rl) == -1) + FAIL_EXIT1 ("getrlimit (RLIMIT_STACK): %m"); + + char cmd[4 * 4096]; + int n = snprintf (cmd, sizeof (cmd), "%s --list %s", ld_so, dso); + if (n >= sizeof (cmd)) + FAIL_EXIT1 ("buffer too small: need %d bytes", n); + + printf ("cmd: %s\n", cmd); + + if (rl.rlim_cur == RLIM_INFINITY) + { + printf ("Capping stack at 8MiB\n"); + rl.rlim_cur = 8 << 20; + } + + while (rl.rlim_cur >= 128 * 1024) + { + printf ("Stack size: %zu\n", (size_t) rl.rlim_cur); + int rc = system (cmd); + + if (rc != 0) + { + /* Clean up. Comment this out to debug ld.so. */ + unlink (dso); + + if (WIFEXITED (rc)) + FAIL_EXIT1 ("child exited with %d", WEXITSTATUS (rc)); + else if (WIFSIGNALED (rc)) + FAIL_EXIT1 ("child exited with signal %d", WTERMSIG (rc)); + else + FAIL_EXIT1 ("child exited with rc %d", rc); + } + + rl.rlim_cur /= 2; + if (setrlimit (RLIMIT_STACK, &rl) == -1) + FAIL_EXIT1 ("setrlimit: %m"); + } + + unlink (dso); + return 0; +} + +#define TEST_FUNCTION_ARGV do_test +#include <support/test-driver.c>
Greetings, Attached patch fixes several instances of unconstrained stack allocation, which causes ld.so to overflow stack and crash while processing DSO with 1000s of Phdr[]s, and adds a test case. 2018-05-21 Paul Pluzhnikov <ppluzhnikov@google.com> [BZ #23187] * elf/Makefile (tst-many-phdrs): New test. * elf/tst-many-phdrs.c: New. * elf/dl-load.c (_dl_map_object_from_fd): Constrain stack allocation. (open_verify): Likewise.