From owner-svn-src-all@freebsd.org Wed Jan 31 15:23:56 2018 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id CF2D1ED1478; Wed, 31 Jan 2018 15:23:56 +0000 (UTC) (envelope-from prvs=56287adec=roger.pau@citrix.com) Received: from SMTP.EU.CITRIX.COM (smtp.eu.citrix.com [185.25.65.24]) (using TLSv1.2 with cipher RC4-SHA (128/128 bits)) (Client CN "mail.citrix.com", Issuer "DigiCert SHA2 Secure Server CA" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id EA46368991; Wed, 31 Jan 2018 15:23:55 +0000 (UTC) (envelope-from prvs=56287adec=roger.pau@citrix.com) X-IronPort-AV: E=Sophos;i="5.46,440,1511827200"; d="scan'208";a="66995469" Date: Wed, 31 Jan 2018 15:09:55 +0000 From: Roger Pau =?iso-8859-1?Q?Monn=E9?= To: Wojciech Macek CC: , , Subject: Re: svn commit: r328536 - in head/stand: common powerpc/kboot Message-ID: <20180131150955.trc5tkkykgxuwf4f@MacBook-Pro-de-Roger.local> References: <201801290924.w0T9OSix008403@repo.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <201801290924.w0T9OSix008403@repo.freebsd.org> User-Agent: NeoMutt/20171208 X-ClientProxiedBy: AMSPEX02CAS01.citrite.net (10.69.22.112) To AMSPEX02CL02.citrite.net (10.69.22.126) X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 31 Jan 2018 15:23:57 -0000 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 > +#include > #include > #include > #include > @@ -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.