From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/14094 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Rodger Combs Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH 3/3] crt: add dcrt1, with support for locating the dynamic loader at runtime Date: Sat, 27 Apr 2019 17:51:17 -0500 Message-ID: <56F34851-93B5-43D7-8968-4316F0F76157@gmail.com> References: <1556327609-27385-1-git-send-email-rodger.combs@gmail.com> <1556327609-27385-3-git-send-email-rodger.combs@gmail.com> <20190427171907.GT23599@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.8\)) Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="250214"; mail-complaints-to="usenet@blaine.gmane.org" To: musl@lists.openwall.com Original-X-From: musl-return-14110-gllmg-musl=m.gmane.org@lists.openwall.com Sun Apr 28 00:51:37 2019 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.89) (envelope-from ) id 1hKWAS-0012yK-Eg for gllmg-musl@m.gmane.org; Sun, 28 Apr 2019 00:51:36 +0200 Original-Received: (qmail 31788 invoked by uid 550); 27 Apr 2019 22:51:34 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 31763 invoked from network); 27 Apr 2019 22:51:33 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:content-transfer-encoding:mime-version:subject:date:references :to:in-reply-to:message-id; bh=14NQPOLW0dvNWc6I9lI/TkDNyi0/IE0gR7DFYLRHBoI=; b=bgqd0OlGw3O+CIHaSQJ0P40I+1JWv17e35rkq9IOm1KcLdZNLbZSzpkfMSR9kWvdpC Y+4EtkEAgdnBHh7OBMcrPBdll1NQB0XorBIVw3uY7fVFnTRMQV/dhZbsSTinFzvE6AI+ PKTiB0Xw6+3NJbmAQAcGrXlnhrGlsZBp9YIZIqlTlbJPCjWr1siZ1As4GU8shpcaOqud g6cWaDXgj4ZJYLArpo53PeYDjwxW5MRFAWF5lg5u/GUYtRKxVLyKVn6DJ3uz9BP/MWYS ofwoRgf8MBxb2PBauGb9W2dwr3pT3tDQTVkSaMqPM3OLMHor9c9sYMQp+94g/4Pk2t/z ZwXw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:content-transfer-encoding:mime-version :subject:date:references:to:in-reply-to:message-id; bh=14NQPOLW0dvNWc6I9lI/TkDNyi0/IE0gR7DFYLRHBoI=; b=NHmyVIzDuH6vzFotnnma83jy/7kRpYjDStHUJIyBfP76AQYFmcam4mGdD8I9O1OIVC wWwdbXDY2qawT2idk7xldmCsdZjLduZacD+w7xPlzOFheIRsJvuUcKMktPXxNeHoGJjX WClWVtTv2zftB41jUUGtk94Su/Gg+7OEz1n9DcnmLeC4Aqe3F8KxyTERM72SwGgj95y0 DVRXDi5WQjP/TSh3PLASYLB6NP9/HRZyidOzBSMNOwHfXP0YkOUvI7SLRRuq8Ceronqi tc84I7tEdNYU14MoW71ED9f0e5QhWR+pY2Gxz4uYTlCCf0jS0ZcAFyhBeUwesI/fzbX1 xnaw== X-Gm-Message-State: APjAAAWDKTa+LTkVdUjaVGgePoqycKtSzC7M4arSizfY3egPobYkk5Fe 3djKmKUYENXGBPsXrmwuZMVp8k2o9qQ= X-Google-Smtp-Source: APXvYqzyLDevdE9iN9sT2u7uxw0QyCPVRyMYHk35Hgj/rn4z5EY2lS1PzT6wTUqEF7fNDAJGIuOmaQ== X-Received: by 2002:a02:c4c6:: with SMTP id h6mr28836499jaj.33.1556405480258; Sat, 27 Apr 2019 15:51:20 -0700 (PDT) In-Reply-To: <20190427171907.GT23599@brightrain.aerifal.cx> X-Mailer: Apple Mail (2.3445.104.8) Xref: news.gmane.org gmane.linux.lib.musl.general:14094 Archived-At: > On Apr 27, 2019, at 12:19, Rich Felker wrote: >=20 > On Fri, Apr 26, 2019 at 08:13:29PM -0500, Rodger Combs wrote: >> diff --git a/crt/dcrt1.c b/crt/dcrt1.c >> new file mode 100644 >> index 0000000..47c6dc2 >> --- /dev/null >> +++ b/crt/dcrt1.c >> @@ -0,0 +1,362 @@ >> +#define SYSCALL_NO_TLS >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include "atomic.h" >> +#include "dynlink.h" >> +#include "syscall.h" >> + >> +extern weak hidden const size_t _DYNAMIC[]; >> + >> +int main(); >> +weak void _init(); >> +weak void _fini(); >> +weak _Noreturn int __libc_start_main(int (*)(), int, char **, >> + void (*)(), void(*)(), void(*)()); >> + >> +#define DLSTART_PROLOGUE __libc_start_main(main, argc, argv, _init, = _fini, 0); >> + >> +#define START "_start" >> +#define _dlstart_c _start_c >> +#include "../ldso/dlstart.c" >> + >> +struct dso { >> + unsigned char *base; >> + struct fdpic_loadmap *loadmap; >> + size_t *dynv; >> + Phdr *phdr; >> + int phnum; >> + size_t phentsize; >> + unsigned char *map; >> + size_t map_len; >> +}; >=20 > There is no need for making a dummy strict dso or exposing struct dso > to this file. The code to be taken/shared from map_library just needs > to be refactored so that the part that writes things into struct dso > remains in dynlink.c. Or, since we're punting on fdpic support for > this for now, a simplified version of map_library without fdpic > support (it gets quite simple then) could perhaps just be put here for > now, with the intent of refactoring to unify later. There are enough relevant values here that it seems to warrant using a = struct (rather than a bunch of individual pointer args), and fairly = little of map_library had to be disabled (basically just the stuff = dealing with TLS and GNU relro/stack stuff) to avoid accesses to other = members. I suppose those portions could be factored out fairly easily, = and then this minimal struct could be made a member of the dso struct? = That just seemed like it'd require a fair bit more change to the = existing dynlink code than this. >=20 >> +#ifndef PAGE_SIZE >> +#define PAGE_SIZE SYSCALL_MMAP2_UNIT >> +#endif >> [...] >> +static inline int crt_mprotect(void *addr, size_t len, int prot) >> +{ >> + size_t start, end; >> + start =3D (size_t)addr & -PAGE_SIZE; >> + end =3D (size_t)((char *)addr + len + PAGE_SIZE-1) & -PAGE_SIZE; >> + return __syscall(SYS_mprotect, start, end-start, prot); >> +} >> +#define mprotect crt_mprotect >=20 > This is wrong. mprotect does not take units of SYSCALL_MMAP2_UNIT. You > need to get the variable page size from auxv. Ah, I was confused because the actual mprotect implementation uses = PAGE_SIZE, which is defined in the public headers as a fixed value = (though looking closer, I'm not sure if it's always the same as = SYSCALL_MMAP2_UNIT anyway), but in the internal headers it's an access = to the __libc struct. Sure. >=20 >> +static char *crt_getenv(const char *name, char **environ) >> +{ >> + size_t l =3D crt_strchrnul(name, '=3D') - name; >> + if (l && !name[l] && environ) >> + for (char **e =3D environ; *e; e++) >> + if (!crt_strncmp(name, *e, l) && l[*e] =3D=3D = '=3D') >> + return *e + l+1; >> + return 0; >> +} >=20 > I really question the use of environment here at all, both from a > standpoint of simplicity/least-surprise, and from a standpoint of > intended usage case. If you're on a non-musl system, the environment > is likely to contain settings that wouldn't be appropriate to musl > anyway, meaing you probably need to remove them for this to work at > all, and then if you really need to force a path other than the > builtin relative one for a particular musl-linked binary, you might as > well just invoke ldso as a command, making the override local to that > invocation rather than inherited across exec of other programs. My goal is to make locating the dynamic loader work the same way as = locating any other library you load, including the ability to override = that path via LD_LIBRARY_PATH for testing, interception, etc. Having a = dedicated var for it means you can set it and have a program and all of = its child processes use an alternate loader when testing something, or = as a way to adjust the loader path after build (if patchelf or similar = tools don't work well for you). >=20 > If you have compelling reasons you want to search the environment, > let's discuss it, but if it's just gratuitous "maybe it will be > useful", let's apply YAGNI. >=20 >> +static void get_rpaths(const char **rpath, const char **runpath, = const size_t *dyn) >> +{ >> + /* DT_STRTAB is pre-relocated for us by dlstart */ >> + const char *strings =3D (void*)dyn[DT_STRTAB]; >> + >> + if (dyn[0] & (1 << DT_RPATH)) >> + *rpath =3D strings + dyn[DT_RPATH]; >> + if (dyn[0] & (1 << DT_RUNPATH)) >> + *runpath =3D strings + dyn[DT_RUNPATH]; >> +} >=20 > musl does not honor the legacy difference in search order between > rpath and runpath (see dynlink.c), but if wwe don't search via > environment here it doesn't matter anyway. It was easy enough to implement, so I did =C2=AF\_(=E3=83=84)_/=C2=AF = but if we explicitly don't want to implement the old behavior I can = remove it. >=20 >> +static size_t find_linker(char *outbuf, size_t bufsize, const char = *this_path, size_t thisl, const size_t *dyn, char **environ, int secure) >> +{ >> + const char *paths[3] =3D {0}; // rpath, envpath, runpath >> + size_t i; >> + int fd; >> + >> + if (thisl) >> + thisl--; >> + while (thisl > 1 && this_path[thisl] =3D=3D '/') >> + thisl--; >> + while (thisl > 0 && this_path[thisl] !=3D '/') >> + thisl--; >> + >> + if (!secure) { >> + const char *envpath =3D crt_getenv("LD_LOADER_PATH", = environ); >> + if (envpath) { >> + size_t envlen =3D crt_strlen(envpath); >> + if (envlen < bufsize) { >> + crt_memcpy(outbuf, envpath, envlen + 1); >> + return envlen + 1; >> + } >> + } >> + } >> + >> + get_rpaths(&paths[0], &paths[2], dyn); >> + >> + paths[1] =3D secure ? NULL : crt_getenv("LD_LIBRARY_PATH", = environ); >> + >> + for (i =3D 0; i < 3; i++) { >> + int relative =3D 0; >> + const char *p =3D paths[i]; >> + char *o =3D outbuf; >> + if (!p) >> + continue; >> + for (;;) { >> + if (!crt_strncmp(p, "$ORIGIN", 7) || >> + !crt_strncmp(p, "${ORIGIN}", 9)) = { >> + relative =3D 1; >> + if (o + thisl + 1 < outbuf + bufsize) { >> + crt_memcpy(o, this_path, thisl); >> + o +=3D thisl; >> + } else { >> + o =3D outbuf + bufsize - 1; >> + } >> + p +=3D (p[1] =3D=3D '{' ? 9 : 7); >> + } else if (*p =3D=3D ':' || !*p) { >> +#define LDSO_FILENAME "ld-musl-" LDSO_ARCH ".so.1" >> + relative |=3D outbuf[0] !=3D '/'; >> + if ((!secure || !relative) && o + = sizeof(LDSO_FILENAME) + 1 < outbuf + bufsize) { >> + *o++ =3D '/'; >> + crt_memcpy(o, LDSO_FILENAME, = sizeof(LDSO_FILENAME)); >> + if (!access(outbuf, R_OK | = X_OK)) >> + return (o + = sizeof(LDSO_FILENAME)) - outbuf; >> + } >> + if (!*p) >> + break; >> + relative =3D 0; >> + o =3D outbuf; >> + p++; >> + } else { >> + if (o < outbuf + bufsize) >> + *o++ =3D *p; >> + p++; >> + } >> + } >> + } >> + >> + // Didn't find a usable loader anywhere, so try the hardcoded = path :shrug: >> + crt_memcpy(outbuf, LDSO_PATHNAME, sizeof(LDSO_PATHNAME)); >> + return sizeof(LDSO_PATHNAME); >> +} >=20 > Including fallbacks that alter behavior is probably a bad idea > (failing preferable). If the changes in your other patch to dynlink.c > are needed to make it work with dcrt1, then it won't necessarily even > work to load a (possibly older) system-wide musl ldso; however if > that's the case I Think we should fix it. This behavior is analogous to dynlink.c's behavior when it searches = rpath for a file and can't find it there. It uses "/etc/ld-musl-" = LDSO_ARCH ".path" and falls back on "/lib:/usr/local/lib:/usr/lib" if = that's unavailable and I'm just using a single hardcoded path, but = beyond that I don't see this as significantly conceptually different. It = also allows executables built with this to work on systems with musl = installed even without having the vendored copy on-hand, which makes it = easier to deploy a single binary that can use a vendored copy if = provided, but can be packaged without it for systems with musl. The changes aren't needed for dcrt1 when the app is run directly; = they're needed when running a dcrt app (or other static PIE app) via = ldso on the command line. >=20 >> +static void final_start_c(long *p) >> +{ >> + int argc =3D p[0]; >> + char **argv =3D (void *)(p+1); >> + __libc_start_main(main, argc, argv, _init, _fini, 0); >> +} >> + >> +hidden _Noreturn void __dls2(unsigned char *base, size_t *p, size_t = *dyn) >> +{ >> + int argc =3D p[0]; >> + char **argv =3D (void *)(p+1); >> + int fd; >> + int secure; >> + int prot =3D PROT_READ; >> + struct dso loader; >> + Ehdr *loader_hdr; >> + Phdr *new_hdr; >> + void *entry; >> + char this_path[2*NAME_MAX+2] =3D {0}; >> + size_t thisl; >> + char linker_path[2*NAME_MAX+2] =3D {0}; >=20 > 2*NAME_MAX is a fairly short arbitrary limit. Why not PATH_MAX, or a > VLA that adapts up to PATH_MAX (to avoid expanding stack when longer > is not needed)? I got this limit from load_library; if we think it should be larger, we = should change it in both places. I don't think there's much use in doing VLAs here (at least, not if = they're going to have fixed maximums that are reasonably low), since = we're going to CRTJMP out of this anyway. >=20 >> + size_t linker_len; >> + size_t i; >> + size_t aux[AUX_CNT]; >> + size_t *auxv; >> + char **environ =3D argv + argc + 1; >> + >> + /* Find aux vector just past environ[] and use it to initialize >> + * global data that may be needed before we can make syscalls. */ >> + for (i =3D argc + 1; argv[i]; i++); >> + auxv =3D (void *)(argv + i + 1); >> + decode_vec(auxv, aux, AUX_CNT); >> + secure =3D ((aux[0] & 0x7800) !=3D 0x7800 || aux[AT_UID] !=3D = aux[AT_EUID] >> + || aux[AT_GID] !=3D aux[AT_EGID] || aux[AT_SECURE]); >=20 > At this point we can just abort if secure !=3D 0. There is unbounded > attack surface trying to load a (possibly relative) ldso with elevated > privileges. No more so than dynlink.c normally has when loading other SOs. Like = there, I don't follow $ORIGIN in secure mode, and additionally here I = don't handle relative-to-cwd paths in secure mode. I don't see a problem = with allowing a load from an absolute rpath, or from the hardcoded path, = using this mechanism, though. Basically, I'm intending for this to be a feature that you could just = turn on in your linker flags for everything you build, and get the = functionality in the cases where you want it, at no significant cost in = those where you don't. >=20 >> + thisl =3D readlink("/proc/self/exe", this_path, sizeof = this_path); >=20 > We might consider whether use of AT_EXECFN (possibly with additional > resolution of symlinks etc) would be better. It would avoid dependency > on /proc, making it possible to run dcrt1 binaries at early boot or > inside chroots, but does have different semantics that might be less > (or maybe more?) desirable. >=20 Hmmmm, so it's possible to get relative paths that way (e.g. = ./test-prog), but I suppose that might be fine? But I don't know how = useful this would really be, since dynlink.c depends on /proc for rpath = anyway. >> + // Copy the program headers into an anonymous mapping >> + new_hdr =3D mmap(0, (aux[AT_PHENT] * (aux[AT_PHNUM] + 2) + = linker_len + PAGE_SIZE - 1) & -PAGE_SIZE, PROT_READ | PROT_WRITE, = MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); >> + if (map_library_failed(new_hdr)) >> + goto error; >=20 > Can you remind us why patched program headers are needed? I think it > was absence of PT_PHDR or something... Yeah, the linker doesn't add PT_PHDR when we tell it not to set a = dynamic loader, and dynlink needs it. >=20 >> + // Point it back at the original kernel-provided base >> + new_hdr->p_type =3D PT_PHDR; >> + new_hdr->p_vaddr =3D (size_t)new_hdr - (size_t)base; >> + >> + ((Phdr*)((char*)new_hdr + aux[AT_PHENT]))->p_type =3D PT_INTERP; >> + ((Phdr*)((char*)new_hdr + aux[AT_PHENT]))->p_vaddr =3D = new_hdr->p_vaddr + aux[AT_PHENT] * (aux[AT_PHNUM] + 2); >=20 > But here you're also adding a PT_INTERP. What's the motivation for = that? It's pretty trivial to provide since we're already making a duplicate = for PHDR, and it lets the loader know its own path, which might be = useful for debug stuff at some point. Generally I'm trying to make this = act as much like a kernel-loaded ldso as possible. >=20 >> + crt_memcpy((char*)new_hdr + aux[AT_PHENT] * (aux[AT_PHNUM] + 2), = linker_path, linker_len); >> + >> + for (i =3D 0; i < aux[AT_PHNUM]; i++) { >> + Phdr *hdr =3D (void*)((char*)aux[AT_PHDR] + = aux[AT_PHENT] * i); >> + Phdr *dst =3D (void*)((char*)new_hdr + aux[AT_PHENT] * = (i + 2)); >> + if (hdr->p_type =3D=3D PT_PHDR || hdr->p_type =3D=3D = PT_INTERP) { >=20 > Isn't it impossible to have a PT_INTERP already here? Normally, yeah, I'm just covering my bases so we behave predictably even = if you do something weird. >=20 >> + // Can't have a duplicate >> + dst->p_type =3D PT_NULL; >=20 > Is adding PT_NULL headers in the middle valid, or do they get > interpreted as end of headers? It's valid; they're just skipped over. >=20 >> + } else { >> + crt_memcpy(dst, hdr, aux[AT_PHENT]); >> + } >> + } >> + >> + if (mprotect(new_hdr, aux[AT_PHENT] * (aux[AT_PHNUM] + 2) + = linker_len, PROT_READ)) >> + goto error; >> + >> + for (i=3D0; auxv[i]; i+=3D2) { >> + if (auxv[i] =3D=3D AT_BASE) >> + auxv[i + 1] =3D (size_t)loader_hdr; >> + if (auxv[i] =3D=3D AT_PHDR) >> + auxv[i + 1] =3D (size_t)new_hdr; >> + if (auxv[i] =3D=3D AT_PHNUM) >> + auxv[i + 1] +=3D 2; >> + } >> + >> + entry =3D laddr(&loader, loader_hdr->e_entry); >> + >> +#ifndef LD_FDPIC >> + /* Undo the relocations performed by dlstart */ >> + >> + if (NEED_MIPS_GOT_RELOCS) { >> + const size_t *dynv =3D _DYNAMIC; >> + size_t local_cnt =3D 0; >> + size_t *got =3D (void *)(base + dyn[DT_PLTGOT]); >> + for (i=3D0; dynv[i]; i+=3D2) if = (dynv[i]=3D=3DDT_MIPS_LOCAL_GOTNO) >> + local_cnt =3D dynv[i+1]; >> + for (i=3D0; i> + } >> + >> + size_t *rel =3D (void *)((size_t)base+dyn[DT_REL]); >> + size_t rel_size =3D dyn[DT_RELSZ]; >> + for (; rel_size; rel+=3D2, rel_size-=3D2*sizeof(size_t)) { >> + if (!IS_RELATIVE(rel[1], 0)) continue; >> + size_t *rel_addr =3D (void *)((size_t)base + rel[0]); >> + *rel_addr -=3D (size_t)base; >> + } >> +#endif >> + >> + CRTJMP(entry, argv - 1); >> + >> +error: >> + for(;;) a_crash(); >> +} >> diff --git a/ldso/dlstart.c b/ldso/dlstart.c >> index 20d50f2..7ff79d6 100644 >> --- a/ldso/dlstart.c >> +++ b/ldso/dlstart.c >> @@ -21,7 +21,7 @@ >> hidden void _dlstart_c(size_t *sp, size_t *dynv) >> { >> size_t i, aux[AUX_CNT], dyn[DYN_CNT]; >> - size_t *rel, rel_size, base; >> + size_t *rel, rel_size, base, loader_phdr; >>=20 >> int argc =3D *sp; >> char **argv =3D (void *)(sp+1); >> @@ -41,6 +41,13 @@ hidden void _dlstart_c(size_t *sp, size_t *dynv) >> * space and moving the extra fdpic arguments to the = stack >> * vector where they are easily accessible from C. */ >> segs =3D ((struct fdpic_loadmap *)(sp[-1] ? sp[-1] : = sp[-2]))->segs; >> + if (aux[AT_BASE]) { >> + Ehdr *eh =3D (void*)aux[AT_BASE]; >> + for (i =3D 0; eh->e_phoff - segs[i].p_vaddr >=3D = segs[i].p_memsz; i++); >> + loader_phdr =3D (eh->e_phoff - segs[i].p_vaddr + = segs[i].addr); >> + } else { >> + loader_phdr =3D aux[AT_PHDR]; >> + } >> } else { >> /* If dynv is null, the entry point was started from = loader >> * that is not fdpic-aware. We can assume normal fixed- >> @@ -55,6 +62,7 @@ hidden void _dlstart_c(size_t *sp, size_t *dynv) >> segs[0].p_memsz =3D -1; >> Ehdr *eh =3D (void *)base; >> Phdr *ph =3D (void *)(base + eh->e_phoff); >> + loader_phdr =3D (size_t)ph; >> size_t phnum =3D eh->e_phnum; >> size_t phent =3D eh->e_phentsize; >> while (phnum-- && ph->p_type !=3D PT_DYNAMIC) >> @@ -69,13 +77,38 @@ hidden void _dlstart_c(size_t *sp, size_t *dynv) >>=20 >> #if DL_FDPIC >> for (i=3D0; i> - if (i=3D=3DDT_RELASZ || i=3D=3DDT_RELSZ) continue; >> + if (i=3D=3DDT_RELASZ || i=3D=3DDT_RELSZ || i=3D=3DDT_RPATH= || i=3D=3DDT_RUNPATH) continue; >> if (!dyn[i]) continue; >> for (j=3D0; dyn[i]-segs[j].p_vaddr >=3D segs[j].p_memsz; = j++); >> dyn[i] +=3D segs[j].addr - segs[j].p_vaddr; >> } >> base =3D 0; >> +#else >> + /* If the dynamic linker is invoked as a command, its load >> + * address is not available in the aux vector. Instead, compute >> + * the load address as the difference between &_DYNAMIC and the >> + * virtual address in the PT_DYNAMIC program header. */ >> + base =3D aux[AT_BASE]; >> + if (!base) { >> + size_t phnum =3D aux[AT_PHNUM]; >> + size_t phentsize =3D aux[AT_PHENT]; >> + Phdr *ph =3D (void *)aux[AT_PHDR]; >> + for (i=3Dphnum; i--; ph =3D (void *)((char *)ph + = phentsize)) { >> + if (ph->p_type =3D=3D PT_DYNAMIC) { >> + base =3D (size_t)dynv - ph->p_vaddr; >> + break; >> + } >> + } >> + } >> + loader_phdr =3D base + ((Ehdr*)base)->e_phoff; >> +#endif >>=20 >> +#ifdef DLSTART_PROLOGUE >> + if (aux[AT_PHDR] !=3D loader_phdr) >> + DLSTART_PROLOGUE >> +#endif >=20 > As mentioned before, this does not work. It puts a symbol reference > into a function that is required to be pure PIC that can run prior to > relocations. Having the reference in a branch that's not taken does > not help. Ah, alright. >=20 > Instead it should be something like (pseudocode): >=20 > if (relocs_already_done) goto done: > // ... > done: > stage2_func dls2; > GETFUNCSYM(&dls2, __dls2, base+dyn[DT_PLTGOT]); > dls2((void *)base, sp); > =09 > This does not require any DLSTART_PROLOGUE macro that's specific to > dcrt1; the exact same thing works in rcrt1 (static pie) and will also > make it safe to invoke static pie programs via ldso (seems pointless, > but useful when you don't realize the binary is static). Alright, simple enough; just added a static int to __dls2 that I set to = 1 before CRTJMP-ing to ldso, and had this same aux[AT_PHDR] check in = dlstart goto past the relocs. It still needs an ifdef, since otherwise = it'd trigger in the loader itself. >=20 > The dcrt1 code in dls2 (which can vary between ldso, rcrt1, and dcrt1) > then has to do its own test to tell which code path it needs to take, > but that's fine. General principle here: repeating small amounts of > work is better than increasing dependency between stages/layers. Same > applies to passing dyn into dls2 -- doing so prevents the call from > being a tail call, and keeps _dlstart_c's stack frame around despite > being done with it. Re-parsing dynv is simple enough, but I need to get at it somehow (this = function gets it from _start). I suppose I could read the _DYNAMIC = symbol in C if you prefer? The stack frame sticking around doesn't really matter, since we're going = to CRTJMP out of this anyway. >=20 >> + /* For convenience in dcrt1, we relocate STRTAB here (as with = FDPIC) */ >> + dyn[DT_STRTAB] +=3D base; >=20 > Just do it where you need it. It's simple enough in the non-FDPIC case; I just did this here for = consistency with FDPIC, where this is significantly more complex. I can = drop it if we want to handle FDPIC differently long-term. >=20 >> /* MIPS uses an ugly packed form for GOT relocations. Since we >> * can't make function calls yet and the code is tiny anyway, >> @@ -144,5 +163,5 @@ hidden void _dlstart_c(size_t *sp, size_t *dynv) >>=20 >> stage2_func dls2; >> GETFUNCSYM(&dls2, __dls2, base+dyn[DT_PLTGOT]); >> - dls2((void *)base, sp); >> + dls2((void *)base, sp, dyn); >=20 > See above. I'll hold off on re-sending this series until we've decided what to do = about the questions above; once that's done, I'll apply whatever we = decide on, and also write up some more useful commit messages. Most of your comments from your other mail are addressed (or at least = discussed) above; the one about rewriting auxv/phdrs is not: - Both linux and freebsd's linux emu layer always insert entries for all = the auxv entries we care about (their values just might be zero), so we = don't need to worry about them potentially being missing (unless there's = some other implementation we care about that I need to check; maybe = Microsoft's?). The aux vector is on the stack, so we don't need to worry = about it being read-only or something like that. glibc actually already = rewrites values in there (when invoked directly), so there's precedent = for doing so. - We make an anonymous mapping in which to rewrite the program headers, = rather than doing it in-place. The only particularly odd bit is that we = take the difference between the effectively-random anonymous mapping = address and the actual kernel-mapped base address, which may be negative = (or very large). In practice, this works fine with musl's loader as-is = (also happens to work with glibc's), and both do an unsigned subtract = with no bounds check, so I think we're fine here as well.