Date: Wed, 31 Jan 2018 15:20:46 +0000 From: Roger Pau =?iso-8859-1?Q?Monn=E9?= <royger@FreeBSD.org> To: Wojciech Macek <wma@FreeBSD.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r328536 - in head/stand: common powerpc/kboot Message-ID: <20180131151823.fwigjbd5uubhshpj@MacBook-Pro-de-Roger.local>
next in thread | raw e-mail | index | archive | help
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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20180131151823.fwigjbd5uubhshpj>