Date: Thu, 8 Feb 2018 16:06:49 +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: <20180208160649.pmkg42i63eipdgiv@MacBook-Pro-de-Roger.local> In-Reply-To: <20180131151823.fwigjbd5uubhshpj@MacBook-Pro-de-Roger.local> References: <20180131151823.fwigjbd5uubhshpj@MacBook-Pro-de-Roger.local>
next in thread | previous in thread | raw e-mail | index | archive | help
Ping?
Ed committed a band-aid, but we need to get this fixed.
On Wed, Jan 31, 2018 at 03:20:46PM +0000, Roger Pau Monné wrote:
> 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?20180208160649.pmkg42i63eipdgiv>