svn commit: r328536 - in head/stand: common powerpc/kboot

[Roger Pau Monné]

On Mon, Jan 29, 2018 at 09:24:28AM +0000, Wojciech Macek wrote:
> Modified: head/stand/common/load_elf.c
> ==============================================================================
> --- head/stand/common/load_elf.c	Mon Jan 29 09:21:08 2018	(r328535)
> +++ head/stand/common/load_elf.c	Mon Jan 29 09:24:28 2018	(r328536)
> @@ -29,6 +29,7 @@
>  __FBSDID("$FreeBSD$");
>  
>  #include <sys/param.h>
> +#include <sys/endian.h>
>  #include <sys/exec.h>
>  #include <sys/linker.h>
>  #include <sys/module.h>
> @@ -118,15 +119,72 @@ __elfN(load_elf_header)(char *filename, elf_file_t ef)
>  		err = EFTYPE;
>  		goto error;
>  	}
> +
>  	if (ehdr->e_ident[EI_CLASS] != ELF_TARG_CLASS || /* Layout ? */
>  	    ehdr->e_ident[EI_DATA] != ELF_TARG_DATA ||

So here you force EI_DATA == ELF_TARG_DATA in order to continue...

> -	    ehdr->e_ident[EI_VERSION] != EV_CURRENT || /* Version ? */
> -	    ehdr->e_version != EV_CURRENT ||
> -	    ehdr->e_machine != ELF_TARG_MACH) { /* Machine ? */
> +	    ehdr->e_ident[EI_VERSION] != EV_CURRENT) /* Version ? */ {
>  		err = EFTYPE;
>  		goto error;
>  	}
>  
> +	/*
> +	 * Fixup ELF endianness.
> +	 *
> +	 * The Xhdr structure was loaded using block read call to
> +	 * optimize file accesses. It might happen, that the endianness
> +	 * of the system memory is different that endianness of
> +	 * the ELF header.
> +	 * Swap fields here to guarantee that Xhdr always contain
> +	 * valid data regardless of architecture.
> +	 */
> +	if (ehdr->e_ident[EI_DATA] == ELFDATA2MSB) {
> +		ehdr->e_type = be16toh(ehdr->e_type);

... yet here you check for EI_DATA == ELFDATA2MSB which AFAICT it's not
possible given the check above, so the following if branch is dead
code.

> +		ehdr->e_machine = be16toh(ehdr->e_machine);
> +		ehdr->e_version = be32toh(ehdr->e_version);
> +		if (ehdr->e_ident[EI_CLASS] == ELFCLASS64) {
> +			ehdr->e_entry = be64toh(ehdr->e_entry);
> +			ehdr->e_phoff = be64toh(ehdr->e_phoff);
> +			ehdr->e_shoff = be64toh(ehdr->e_shoff);
> +		} else {
> +			ehdr->e_entry = be32toh(ehdr->e_entry);
> +			ehdr->e_phoff = be32toh(ehdr->e_phoff);
> +			ehdr->e_shoff = be32toh(ehdr->e_shoff);
> +		}
> +		ehdr->e_flags = be32toh(ehdr->e_flags);
> +		ehdr->e_ehsize = be16toh(ehdr->e_ehsize);
> +		ehdr->e_phentsize = be16toh(ehdr->e_phentsize);
> +		ehdr->e_phnum = be16toh(ehdr->e_phnum);
> +		ehdr->e_shentsize = be16toh(ehdr->e_shentsize);
> +		ehdr->e_shnum = be16toh(ehdr->e_shnum);
> +		ehdr->e_shstrndx = be16toh(ehdr->e_shstrndx);
> +
> +	} else {
> +		ehdr->e_type = le16toh(ehdr->e_type);
> +		ehdr->e_machine = le16toh(ehdr->e_machine);
> +		ehdr->e_version = le32toh(ehdr->e_version);
> +		if (ehdr->e_ident[EI_CLASS] == ELFCLASS64) {
> +			ehdr->e_entry = le64toh(ehdr->e_entry);
> +			ehdr->e_phoff = le64toh(ehdr->e_phoff);
> +			ehdr->e_shoff = le64toh(ehdr->e_shoff);
> +		} else {
> +			ehdr->e_entry = le32toh(ehdr->e_entry);
> +			ehdr->e_phoff = le32toh(ehdr->e_phoff);
> +			ehdr->e_shoff = le32toh(ehdr->e_shoff);
> +		}
> +		ehdr->e_flags = le32toh(ehdr->e_flags);
> +		ehdr->e_ehsize = le16toh(ehdr->e_ehsize);
> +		ehdr->e_phentsize = le16toh(ehdr->e_phentsize);
> +		ehdr->e_phnum = le16toh(ehdr->e_phnum);
> +		ehdr->e_shentsize = le16toh(ehdr->e_shentsize);
> +		ehdr->e_shnum = le16toh(ehdr->e_shnum);
> +		ehdr->e_shstrndx = le16toh(ehdr->e_shstrndx);
> +	}

I think this chunk (and all the similar ones below) should be put on a
macro in order to avoid such big chunks of code repetition. It's also
fairly easy to forget to change one of the branches in the future.

Roger.