[musl] [PATCH 0/1] Ignore incorrect elf architecture libraries

[musl] [PATCH 0/1] Ignore incorrect elf architecture libraries

[Markus Mayer]

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

[musl] [PATCH 1/1] Ignore incorrect elf architecture libraries

[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

Re: [musl] [PATCH 1/1] Ignore incorrect elf architecture libraries

[Szabolcs Nagy]

* Markus Mayer <mmayer@broadcom.com> [2024-07-05 14:12:28 -0700]:
> 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.

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 (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;

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

Re: [musl] [PATCH 1/1] Ignore incorrect elf architecture libraries

[Rich Felker]

On Tue, Jul 23, 2024 at 11:08:49PM +0200, Szabolcs Nagy wrote:
> * Markus Mayer <mmayer@broadcom.com> [2024-07-05 14:12:28 -0700]:
> > 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.
> 
> 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

FWIW we don't necessarily follow everything in this spec (it's not one
of the ones we claim conformance to), but yes it can be a good model
for how to do things.

> > ---
> >  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)

Yes, let's avoid size creep for struct dso.

> >  	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.

IIRC at one point it was my proposal to rename it *and* change the
signature to make it load the start of the file into a caller-provided
buffer that map_library would then reuse...

> >  		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.

...so that there is not an extra syscall per DSO loaded.

Re: [musl] [PATCH 1/1] Ignore incorrect elf architecture libraries

[Markus Mayer]

On Tue, 23 Jul 2024 at 14:08, Szabolcs Nagy <nsz@port70.net> wrote:

Thanks for reviewing the proposed changes.

It's been a little while since the submission. I found some time to
work on this again, and I was able to incorporate most of the proposed
changes. Everything is still working as intended.

Also, Colin reached out to me following my re-submission of his
proposal. He told me he wasn't going to have time to work on it for
the foreseeable future and that I was welcome to continue the work and
to take over authorship in that case.

Here's a link to the current state of affairs.[1]

I am not posting the new patch yet and just posting a link, because
work isn't quite finished. Also, this makes it easier to continue the
already existing thread. I am providing my thoughts and responses
below.

> > 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:
>
[...]
>
> 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.

I agree with regards to EI_DATA. It makes a lot of sense checking that
as well while we are at it, and it is now already implemented in the
current revision of this proposed change.

Checking the e_flags also sounds very sensible, especially in the
context of ARM. However, I am not sure if it is feasible. The
specification says that e_flags are architecture-specific, so
interpreting and checking the flags would become a per-architecture
endeavour. That would mean the loader has to know a lot about the
different architectures and then decide at runtime which check to run.
Ideally, we would probably need conditional compilation to only build
in the checks for the architecture in question. Otherwise the code
gets bloated with routines that will never be called. It sounds rather
involved and a little messy. Also, there doesn't seem to be a
precedent for architecture-specific code in the loader.

To get an idea, see [2] for what readelf is getting up to in order to
parse the ELF flags for ARM. (Spoiler alert, it's over 200 lines of
code.)

That being said, if there is a non-messy way to implement checks for
ARM soft- and hard-float, I am all for it.

> 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.

Too many checks might also lead to false positives, i.e. binaries
being rejected that would have run just fine. All it would take is for
some obscure ELF header fields to be set improperly at build time and
nobody notices, because the fields are ignored by almost everyone.
Alternatively, our checks could end up being too strict even when the
fields are all set correctly.

> 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)

Yes, I did that. At first I created individual, global variables, but
ultimately I decided to encapsulate them in a struct to make the code
a little cleaner and introduce only one global variable rather than
four. The struct now looks a bit like it is duplicating Ehdr, but
there are far fewer fields inside.

> >       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.

Done.

> > +
> >  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;
>
> ok.
>
> > +     dso->elfmachine = eh->e_machine;
> > +     dso->elfclass = eh->e_ident[EI_CLASS];
>
> i don't think this is needed.

Indeed. Removed.

> >       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.

Done.

> >  {
> >       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.

Done. However, there's an indentation change nonetheless, so the diff
still looks quite large. The unchanged "else" branch shows up in the
diff because of whitespace. Using "git diff -w" for this section helps
a bit.

> >               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

I tried. It doesn't work. The reason is that read() advances the file
pointer whereas pread() does not. In other words, pread() doesn't
affect subsequent read() calls, whereas using read() here throws off a
subsequent read that was expecting to read the beginning of the file.

I didn't spend the time to narrow down where exactly it is tripping up
when using read() here. However, I tried read() followed by lseek() to
reset the file pointer to the beginning to verify my theory. Doing
this works, and therefore proves that the advanced file pointer is
indeed the problem.

To summarize:

- using pread() works
- using read() fails
- using read() followed by lseek(fd, 0, SEEK_SET) works, but adds an
extra syscall

As a result, the code is continuing to use 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.

Done. For now, at least. I still need to address Rich's feedback about
the caller-provided buffer.

> > +                     /* 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);

Done.

Regards,
-Markus

[1] https://github.com/mmayer/musl-libc/compare/master...ld_so-squashed
[2] https://sourceware.org/git?p=binutils-gdb.git;a=blob;f=binutils/readelf.c;h=0f8dc1b9716ed5c0ba13ececfc012ed59f8ba270;hb=HEAD#l3511