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

[Szabolcs Nagy <nsz@port70.net>]

* 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