mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] [PATCH 0/1] Ignore incorrect elf architecture libraries
@ 2024-07-05 21:12 Markus Mayer
  2024-07-05 21:12 ` [musl] [PATCH 1/1] " Markus Mayer
  0 siblings, 1 reply; 2+ messages in thread
From: Markus Mayer @ 2024-07-05 21:12 UTC (permalink / raw)
  To: Musl Mailing List; +Cc: Markus Mayer, Colin Cross

Hi all,

Several months back, I reported a problem I discovered when it comes to
how musl's shared library loader is handling shared libraries of a wrong
architecture.[1] Colin Cross then responded that he had sent a patch for
the very same issue a year prior.[2]

There seemed to be some interest in my submission initially, but then
the discussion fizzled out.

The short summary of the problem is that the shared library loader will
stop looking for a suitable shared library once it finds a library of
the correct name. It won't care if it's the wrong architecture (ELF
class) and stop looking, regardless. The correct library will never be
found, even if present. This will result in binaries failing to run even
though they could have been executed without a problem.

If was finally able to get back to it and create this proposal.

I applied Colin's patch to musl 1.2.5 and experimented with it. I also
ran a bunch of tests. It seems to be doing exactly what I would expect
the shared library loader to do.

Since Colin's patch is much better than mine, I would like to re-submit
his patch and propose it for inclusion once more.

I am not entirely sure how to properly submit another person's patch to
the musl project. In kernel land, one would add Signed-off-by and
Tested-by tags to the commit message, but neither seem to be customary
in the musl project.

Please let me know what you think and if there is anything else I should
be doing to help with having this patch accepted.

To conclude the matter, I'll show the test setup I used.

* A simple test program linked against libz.so, built for aarch64.
* /usr/lib/libz.so being the aarch64 library
* /tmp/lib/libz.so being the aarch32 library

And these are the tests:

* Run "zdemo" without setting LD_LIBRARY_PATH
* Run "zdemo" with LD_LIBRARY_PATH pointing to /tmp/lib
* Repeat the steps with and without Colin's patch

$ ls -l /tmp/lib
total 100
drwxr-xr-x    2 root     root       100 Jan  7 04:18 .
drwxrwxrwt    3 root     root       120 Jan  7 04:18 ..
lrwxrwxrwx    1 root     root        13 Jul  5  2024 libz.so -> libz.so.1.3.1
lrwxrwxrwx    1 root     root        13 Jul  5  2024 libz.so.1 -> libz.so.1.3.1
-rwxr-xr-x    1 root     root    101420 Jul  5  2024 libz.so.1.3.1

$ ls -l /usr/lib/libz*
lrwxrwxrwx    1 root     root        13 Jun 28  2024 libz.so -> libz.so.1.3.1
lrwxrwxrwx    1 root     root        13 Jun 28  2024 libz.so.1 -> libz.so.1.3.1
-rwxr-xr-x    1 root     root     83664 Jun 28  2024 libz.so.1.3.1

The results look like this.

1) Unpatched libc, no LD_LIBRARY_PATH:

$ ./zdemo-arm64 /dev/null
gz=0x7fae7f4860

2) Unpatched libc, with LD_LIBRARY_PATH:

$ LD_LIBRARY_PATH=/tmp/lib ./zdemo-arm64 /dev/null
Error loading shared library libz.so.1: Exec format error (needed by ./zdemo-arm64)
Error relocating ./zdemo-arm64: gzopen: symbol not found
Error relocating ./zdemo-arm64: gzclose: symbol not found

3) Patched libc, no LD_LIBRARY_PATH:

$ ./libc.so ./zdemo-arm64 /dev/null
gz=0x7faca58860

4) Patched libc, with LD_LIBRARY_PATH:

$ LD_LIBRARY_PATH=/tmp/lib ./libc.so ./zdemo-arm64 /dev/null
gz=0x7f9002d860

4a) Patched libc, with LD_LIBRARY_PATH and debug output:

$ LD_LIBRARY_PATH=/tmp/lib ./libc.so ./zdemo-arm64 /dev/null
verify_elf_arch, 712: machine=b7 / EI_CLASS=2
path_open_library, 920: opening /tmp/lib/libz.so.1
verify_elf_arch, 712: machine=28 / EI_CLASS=1
path_open_library, 920: opening /lib/libz.so.1
    open failed -- No such file or directory
path_open_library, 920: opening /usr/local/lib/libz.so.1
    open failed -- No such file or directory
path_open_library, 920: opening /usr/lib/libz.so.1
verify_elf_arch, 712: machine=b7 / EI_CLASS=2
path_open_library, 956: returning 3 (for /usr/lib/libz.so.1)
verify_elf_arch, 712: machine=b7 / EI_CLASS=2
gz=0x7f9002d860

As we can see, with Colin's patch, it finds /tmp/lib/libz.so.1 first,
realizes it's the wrong ELF class -- and keeps looking. It then proceeds
through the regular library search path (/lib, /usr/local/lib,
/usr/lib), where it ultimately finds the proper libz.

Without Colin's patch, it bails after trying to use /tmp/lib/libz.so.1.

Regards,
-Markus

[1] https://www.openwall.com/lists/musl/2024/02/07/1
[2] https://www.openwall.com/lists/musl/2023/02/07/3

Colin Cross (1):
  Ignore incorrect elf architecture libraries

 ldso/dynlink.c | 77 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 61 insertions(+), 16 deletions(-)

-- 
2.45.2


^ permalink raw reply	[flat|nested] 2+ messages in thread

* [musl] [PATCH 1/1] Ignore incorrect elf architecture libraries
  2024-07-05 21:12 [musl] [PATCH 0/1] Ignore incorrect elf architecture libraries Markus Mayer
@ 2024-07-05 21:12 ` Markus Mayer
  0 siblings, 0 replies; 2+ messages in thread
From: Markus Mayer @ 2024-07-05 21:12 UTC (permalink / raw)
  To: Musl Mailing List; +Cc: Colin Cross, Colin Cross, Markus Mayer

From: Colin Cross <ccross@android.com>

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.
---
 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;
 	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;
+}
+
 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 (l<sizeof *eh || (eh->e_type != ET_DYN && eh->e_type != ET_EXEC))
 		goto noexec;
+	if (!verify_elf_magic(eh)) goto noexec;
+	if (!verify_elf_arch(eh)) goto noexec;
+	dso->elfmachine = eh->e_machine;
+	dso->elfclass = eh->e_ident[EI_CLASS];
 	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)
 {
 	size_t l;
 	int fd;
+	const char *p;
 	for (;;) {
 		s += strspn(s, ":\n");
+		p = s;
 		l = strcspn(s, ":\n");
 		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);
+			/* 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;
 			}
+			/* 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;
 		}
-		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


^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2024-07-05 21:13 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-07-05 21:12 [musl] [PATCH 0/1] Ignore incorrect elf architecture libraries Markus Mayer
2024-07-05 21:12 ` [musl] [PATCH 1/1] " Markus Mayer

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).