From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.3 required=5.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: from second.openwall.net (second.openwall.net [193.110.157.125]) by inbox.vuxu.org (Postfix) with SMTP id 7138E23886 for ; Tue, 23 Jul 2024 23:09:03 +0200 (CEST) Received: (qmail 26351 invoked by uid 550); 23 Jul 2024 21:08:58 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 26307 invoked from network); 23 Jul 2024 21:08:58 -0000 Date: Tue, 23 Jul 2024 23:08:49 +0200 From: Szabolcs Nagy To: Markus Mayer Cc: Musl Mailing List , Colin Cross , Colin Cross Message-ID: <20240723210849.GU3766212@port70.net> Mail-Followup-To: Markus Mayer , Musl Mailing List , Colin Cross , Colin Cross References: <20240705211232.2718532-1-mmayer@broadcom.com> <20240705211232.2718532-2-mmayer@broadcom.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240705211232.2718532-2-mmayer@broadcom.com> Subject: Re: [musl] [PATCH 1/1] Ignore incorrect elf architecture libraries * Markus Mayer [2024-07-05 14:12:28 -0700]: > From: Colin Cross > > In multilib systems LD_LIBRARY_PATH is sometimes set to a list that > contains both the lib32 and lib64 directories, for example when > running code that may execute 32-bit or 64-bit subprocesses or both. > Musl's dynamic loader aborts searching a path list when it finds any > library that is not loadable, preventing searching the other multilib > directory. > > Modify the loader to continue searching when a file is found that is > a valid elf file but for the an elf machine or class that differs from > that of the loader itself. fwiw i think the proposed feature is reasonable, i added review comments that may help inclusion into musl. i'd note that skipping wrong e_machine and e_ident[EI_CLASS] is required by the elf spec: When the dynamic linker is searching for shared objects, it is not a fatal error if an ELF file with the wrong attributes is encountered in the search. Instead, the dynamic linker shall exhaust the search of all paths before determining that a matching object could not be found. For this determination, the relevant attributes are contained in the following ELF header fields: e_ident[EI_DATA], e_ident[EI_CLASS], e_ident[EI_OSABI], e_ident[EI_ABIVERSION], e_machine, e_type, e_flags and e_version. if we do this change i think e_ident[EI_DATA] should be checked too for le/be and e_flags for abi variants like arm hf. e_type, e_version, EI_OSABI and EI_ABIVERSION are unlikely to be useful (the latter may be used in the future to bump musl abi) i don't have a strong opinion about these. https://www.sco.com/developers/gabi/latest/ch5.dynamic.html#shobj_dependencies > --- > ldso/dynlink.c | 77 +++++++++++++++++++++++++++++++++++++++----------- > 1 file changed, 61 insertions(+), 16 deletions(-) > > diff --git a/ldso/dynlink.c b/ldso/dynlink.c > index 324aa85919f0..2f1ac7e9d089 100644 > --- a/ldso/dynlink.c > +++ b/ldso/dynlink.c > @@ -68,6 +68,8 @@ struct dso { > size_t *dynv; > struct dso *next, *prev; > > + int elfmachine; > + int elfclass; i don't think we need this in every dso. i'd make them global e.g. ldso_ehdr (or just the ldso_e_machine and ldso_e_class) > Phdr *phdr; > int phnum; > size_t phentsize; > @@ -684,6 +686,19 @@ static void unmap_library(struct dso *dso) > } > } > > +static int verify_elf_magic(const Ehdr* eh) { > + return eh->e_ident[0] == ELFMAG0 && > + eh->e_ident[1] == ELFMAG1 && > + eh->e_ident[2] == ELFMAG2 && > + eh->e_ident[3] == ELFMAG3; > +} > + > +/* Verifies that an elf header's machine and class match the loader */ > +static int verify_elf_arch(const Ehdr* eh) { > + return eh->e_machine == ldso.elfmachine && > + eh->e_ident[EI_CLASS] == ldso.elfclass; > +} i'd do more checks and drop the comment. > + > static void *map_library(int fd, struct dso *dso) > { > Ehdr buf[(896+sizeof(Ehdr))/sizeof(Ehdr)]; > @@ -706,6 +721,10 @@ static void *map_library(int fd, struct dso *dso) > if (l<0) return 0; > if (le_type != ET_DYN && eh->e_type != ET_EXEC)) > goto noexec; > + if (!verify_elf_magic(eh)) goto noexec; > + if (!verify_elf_arch(eh)) goto noexec; ok. > + dso->elfmachine = eh->e_machine; > + dso->elfclass = eh->e_ident[EI_CLASS]; i don't think this is needed. > phsize = eh->e_phentsize * eh->e_phnum; > if (phsize > sizeof buf - sizeof *eh) { > allocated_buf = malloc(phsize); > @@ -870,29 +889,53 @@ error: > return 0; > } > > -static int path_open(const char *name, const char *s, char *buf, size_t buf_size) > +static int path_open_library(const char *name, const char *s, char *buf, size_t buf_size) no need to rename. > { > size_t l; > int fd; > + const char *p; > for (;;) { > s += strspn(s, ":\n"); > + p = s; > l = strcspn(s, ":\n"); i would keep the orig logic and add new stuff to if ((fd = ...) >= 0) { // here } else switch (errno) { // this is orig logic } then the diff is easier to review. > if (l-1 >= INT_MAX) return -1; > - if (snprintf(buf, buf_size, "%.*s/%s", (int)l, s, name) < buf_size) { > - if ((fd = open(buf, O_RDONLY|O_CLOEXEC))>=0) return fd; > - switch (errno) { > - case ENOENT: > - case ENOTDIR: > - case EACCES: > - case ENAMETOOLONG: > - break; > - default: > - /* Any negative value but -1 will inhibit > - * futher path search. */ > + s += l; > + if (snprintf(buf, buf_size, "%.*s/%s", (int)l, p, name) < buf_size) { > + fd = open(buf, O_RDONLY|O_CLOEXEC); > + if (fd < 0) { > + switch (errno) { > + case ENOENT: > + case ENOTDIR: > + case EACCES: > + case ENAMETOOLONG: > + /* Keep searching in path list. */ > + continue; > + default: > + /* Any negative value but -1 will > + * inhibit further path search in > + * load_library. */ > + return -2; > + } > + } > + Ehdr eh; > + ssize_t n = pread(fd, &eh, sizeof(eh), 0); i'd use read instead of pread this adds an extra syscall per dso that uses path lookup. this is the main cost of the patch, so i'd note this in the commit msg. > + /* If the elf file is invalid return -2 to inhibit > + * further path search in load_library. */ > + if (n < 0 || > + n != sizeof eh || > + !verify_elf_magic(&eh)) { > + close(fd); > return -2; > } ok. > + /* If the elf file has a valid header but is for the > + * wrong architecture ignore it and keep searching the > + * path list. */ > + if (!verify_elf_arch(&eh)) { > + close(fd); > + continue; > + } > + return fd; that's a weird way to end a loop. i'd do if (verify_elf_arch(&eh)) return fd; close(fd); > } > - s += l; > } > } > > @@ -1116,12 +1159,12 @@ static struct dso *load_library(const char *name, struct dso *needed_by) > } > if (strlen(name) > NAME_MAX) return 0; > fd = -1; > - if (env_path) fd = path_open(name, env_path, buf, sizeof buf); > + if (env_path) fd = path_open_library(name, env_path, buf, sizeof buf); > for (p=needed_by; fd == -1 && p; p=p->needed_by) { > if (fixup_rpath(p, buf, sizeof buf) < 0) > fd = -2; /* Inhibit further search. */ > if (p->rpath) > - fd = path_open(name, p->rpath, buf, sizeof buf); > + fd = path_open_library(name, p->rpath, buf, sizeof buf); > } > if (fd == -1) { > if (!sys_path) { > @@ -1160,7 +1203,7 @@ static struct dso *load_library(const char *name, struct dso *needed_by) > } > } > if (!sys_path) sys_path = "/lib:/usr/local/lib:/usr/lib"; > - fd = path_open(name, sys_path, buf, sizeof buf); > + fd = path_open_library(name, sys_path, buf, sizeof buf); > } > pathname = buf; > } > @@ -1732,6 +1775,8 @@ hidden void __dls2(unsigned char *base, size_t *sp) > ldso.phnum = ehdr->e_phnum; > ldso.phdr = laddr(&ldso, ehdr->e_phoff); > ldso.phentsize = ehdr->e_phentsize; > + ldso.elfmachine = ehdr->e_machine; > + ldso.elfclass = ehdr->e_ident[EI_CLASS]; > search_vec(auxv, &ldso_page_size, AT_PAGESZ); > kernel_mapped_dso(&ldso); > decode_dyn(&ldso); > -- > 2.45.2