From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1Z4tyu-000299-Jh for mharc-grub-devel@gnu.org; Tue, 16 Jun 2015 12:45:00 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36753) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z4tyq-000234-OB for grub-devel@gnu.org; Tue, 16 Jun 2015 12:44:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z4tyn-0006QH-Cn for grub-devel@gnu.org; Tue, 16 Jun 2015 12:44:56 -0400 Received: from mail-la0-x236.google.com ([2a00:1450:4010:c03::236]:35545) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z4tyn-0006QB-0s for grub-devel@gnu.org; Tue, 16 Jun 2015 12:44:53 -0400 Received: by labko7 with SMTP id ko7so15458626lab.2 for ; Tue, 16 Jun 2015 09:44:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:subject:message-id:in-reply-to:references:mime-version :content-type:content-transfer-encoding; bh=kk0FT3GQob0/1taVkhM41vuv+fbiEZtv28lB5MjgVv4=; b=0Y6ZJ6WTDBfTshUKLZ9ginmiTRq1B1eP6Up2anbCxBvtZPyc/cFdK+NbjxaxyADrAN FTKYttGhqG09GpEYvhjVB2mN0+kL56M/at3DHJ1h513G6E49h8UFzfgiWdC1P5dO15i4 x0rtBY8k3EDZA04ET6g4daDpZHRCPs7Jd6GTdibNKrDTiF/slA0/1TWSshde+EM2RL3Z J8WeJhNlsQilQbQhm/262qaVb9L8dSCzXLuayLgGPl2T/q4wXDijbWMIfsDhO6iNLXyD dnvPTw4xw5dkFXLBPYH9rNM+RmYQjJN/EhlFCSfp8ein1z71TQpQm+eHqZifylxo4k8Y euOQ== X-Received: by 10.112.160.73 with SMTP id xi9mr2381976lbb.92.1434473092005; Tue, 16 Jun 2015 09:44:52 -0700 (PDT) Received: from opensuse.site (ppp91-76-14-38.pppoe.mtu-net.ru. [91.76.14.38]) by mx.google.com with ESMTPSA id ap9sm353879lbd.21.2015.06.16.09.44.51 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 16 Jun 2015 09:44:51 -0700 (PDT) Date: Tue, 16 Jun 2015 19:44:49 +0300 From: Andrei Borzenkov To: The development of GNU GRUB Subject: Re: [RFC PATCH 3/3] Suport for bi-endianess in elf file Message-ID: <20150616194449.02d3eca6@opensuse.site> In-Reply-To: <1409255765-3209-4-git-send-email-pfsmorigo@linux.vnet.ibm.com> References: <1409255765-3209-1-git-send-email-pfsmorigo@linux.vnet.ibm.com> <1409255765-3209-4-git-send-email-pfsmorigo@linux.vnet.ibm.com> X-Mailer: Claws Mail 3.11.0 (GTK+ 2.24.28; x86_64-suse-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2a00:1450:4010:c03::236 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 16 Jun 2015 16:44:58 -0000 On Thu, Aug 28, 2014 at 10:56 PM, Paulo Flabiano Smorigo wrote: > * grub-core/kern/elf.c: check and switch endianess with grub_{be,le}_to > cpu functions. > * grub-core/kern/elfXX.c: Likewise. > As a general comment - currently we need it for one platform only so let's wrap additional code in #ifdef __powerpc__ until any more users emerge, to avoid dead code on other platforms (especially size-critical like i386-pc). > Also-by: Tomohiro B Berry > --- > grub-core/kern/elf.c | 60 +++++++++++++++++++++++++++++++++++++++-- > grub-core/kern/elfXX.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 131 insertions(+), 2 deletions(-) > > diff --git a/grub-core/kern/elf.c b/grub-core/kern/elf.c > index 5f99c43..de90811 100644 > --- a/grub-core/kern/elf.c > +++ b/grub-core/kern/elf.c > @@ -28,6 +28,11 @@ > > GRUB_MOD_LICENSE ("GPLv3+"); > > +void grub_elf32_check_endianess (grub_elf_t elf); > +void grub_elf64_check_endianess (grub_elf_t elf); > +grub_err_t grub_elf32_check_version (grub_elf_t elf); > +grub_err_t grub_elf64_check_version (grub_elf_t elf); > + > /* Check if EHDR is a valid ELF header. */ > static grub_err_t > grub_elf_check_header (grub_elf_t elf) > @@ -38,10 +43,22 @@ grub_elf_check_header (grub_elf_t elf) > || e->e_ident[EI_MAG1] != ELFMAG1 > || e->e_ident[EI_MAG2] != ELFMAG2 > || e->e_ident[EI_MAG3] != ELFMAG3 > - || e->e_ident[EI_VERSION] != EV_CURRENT > - || e->e_version != EV_CURRENT) > + || e->e_ident[EI_VERSION] != EV_CURRENT) > return grub_error (GRUB_ERR_BAD_OS, N_("invalid arch-independent ELF magic")); > > + if (grub_elf_is_elf32 (elf)) > + { > + grub_elf32_check_endianess (elf); > + grub_elf32_check_version (elf); > + } > + else if (grub_elf_is_elf64 (elf)) > + { > + grub_elf64_check_endianess (elf); > + grub_elf64_check_version (elf); > + } This ignores any errors these functions may return. I would suggest #if defined(__powerpc__) if ((grub_elf_is_elf32 (elf)) grub_elf32_check_endianess (elf); ... > + else > + return grub_error (GRUB_ERR_BAD_OS, N_("invalid arch-dependent ELF magic")); > + It probably should be "Uknown ELF class" here. #endif /* __powerpc__ */ if (e->e_version != EV_CURRENT) return grub_error (GRUB_ERR_BAD_OS, N_("invalid arch-independent ELF magic")); > return GRUB_ERR_NONE; > } > > @@ -127,7 +144,20 @@ grub_elf_open (const char *name) > #define grub_elf_is_elfXX grub_elf_is_elf32 > #define grub_elfXX_load_phdrs grub_elf32_load_phdrs > #define ElfXX_Phdr Elf32_Phdr > +#define ElfXX_Ehdr Elf32_Ehdr > #define grub_uintXX_t grub_uint32_t > +#define grub_be_to_halfXX grub_be_to_cpu16 > +#define grub_be_to_wordXX grub_be_to_cpu32 > +#define grub_be_to_addrXX grub_be_to_cpu32 > +#define grub_be_to_offXX grub_be_to_cpu32 > +#define grub_be_to_XwordXX grub_be_to_wordXX Only addr and off are different between 32/64. Do we really need to define others? > +#define grub_le_to_halfXX grub_le_to_cpu16 > +#define grub_le_to_wordXX grub_le_to_cpu32 > +#define grub_le_to_addrXX grub_le_to_cpu32 > +#define grub_le_to_offXX grub_le_to_cpu32 > +#define grub_le_to_XwordXX grub_le_to_wordXX > +#define grub_elfXX_check_endianess grub_elf32_check_endianess > +#define grub_elfXX_check_version grub_elf32_check_version > > #include "elfXX.c" > > @@ -140,7 +170,20 @@ grub_elf_open (const char *name) > #undef grub_elf_is_elfXX > #undef grub_elfXX_load_phdrs > #undef ElfXX_Phdr > +#undef ElfXX_Ehdr > #undef grub_uintXX_t > +#undef grub_be_to_halfXX > +#undef grub_be_to_wordXX > +#undef grub_be_to_addrXX > +#undef grub_be_to_offXX > +#undef grub_be_to_XwordXX > +#undef grub_le_to_halfXX > +#undef grub_le_to_wordXX > +#undef grub_le_to_addrXX > +#undef grub_le_to_offXX > +#undef grub_le_to_XwordXX > +#undef grub_elfXX_check_endianess > +#undef grub_elfXX_check_version > > > /* 64-bit */ > @@ -153,6 +196,19 @@ grub_elf_open (const char *name) > #define grub_elf_is_elfXX grub_elf_is_elf64 > #define grub_elfXX_load_phdrs grub_elf64_load_phdrs > #define ElfXX_Phdr Elf64_Phdr > +#define ElfXX_Ehdr Elf64_Ehdr > #define grub_uintXX_t grub_uint64_t > +#define grub_be_to_halfXX grub_be_to_cpu16 > +#define grub_be_to_wordXX grub_be_to_cpu32 > +#define grub_be_to_addrXX grub_be_to_cpu64 > +#define grub_be_to_offXX grub_be_to_cpu64 > +#define grub_be_to_XwordXX grub_be_to_cpu64 > +#define grub_le_to_halfXX grub_le_to_cpu16 > +#define grub_le_to_wordXX grub_le_to_cpu32 > +#define grub_le_to_addrXX grub_le_to_cpu64 > +#define grub_le_to_offXX grub_le_to_cpu64 > +#define grub_le_to_XwordXX grub_le_to_cpu64 > +#define grub_elfXX_check_endianess grub_elf64_check_endianess > +#define grub_elfXX_check_version grub_elf64_check_version > > #include "elfXX.c" > diff --git a/grub-core/kern/elfXX.c b/grub-core/kern/elfXX.c > index 1d09971..ecf9df6 100644 > --- a/grub-core/kern/elfXX.c > +++ b/grub-core/kern/elfXX.c > @@ -154,3 +154,76 @@ grub_elfXX_load (grub_elf_t elf, const char *filename, > > return grub_errno; > } > + > +void > +grub_elfXX_check_endianess (grub_elf_t elf) > +{ > + ElfXX_Ehdr *e = &(elf->ehdr.ehdrXX); > + ElfXX_Phdr *phdr; > + This block is not needed on BE systems; and as of this writing there is no LE system that needs it at all, so I suggest to leave it in place but effectively ifdef out like #if !defined(GRUB_CPU_WORDS_BIGENDIAN) && 0 > + if (e->e_ident[EI_DATA] == ELFDATA2MSB) > + { > + e->e_type = grub_be_to_halfXX (e->e_type); > + e->e_machine = grub_be_to_halfXX (e->e_machine); > + e->e_version = grub_be_to_wordXX (e->e_version); > + e->e_entry = grub_be_to_addrXX (e->e_entry); > + e->e_phoff = grub_be_to_offXX (e->e_phoff); > + e->e_shoff = grub_be_to_offXX (e->e_shoff); > + e->e_flags = grub_be_to_wordXX (e->e_flags); > + e->e_ehsize = grub_be_to_halfXX (e->e_ehsize); > + e->e_phentsize = grub_be_to_halfXX (e->e_phentsize); > + e->e_phnum = grub_be_to_halfXX (e->e_phnum); > + e->e_shentsize = grub_be_to_halfXX (e->e_shentsize); > + e->e_shnum = grub_be_to_halfXX (e->e_shnum); > + e->e_shstrndx = grub_be_to_halfXX (e->e_shstrndx); > + > + FOR_ELFXX_PHDRS (elf,phdr) > + { I suggest split in two parts - first one to byteswap ELF headers and second one for p-sections in grub_elfXX_load_phdrs. This makes error handling easier - byteswap is always possible, and at the point it will be called in grub_elfXX_load_phdrs all error checks are done. Also it is not needed until version is validated and we actually decide to load sections. And please wrap them under #ifdef __powerpc__ as well. > + phdr->p_type = grub_be_to_wordXX (phdr->p_type); > + phdr->p_flags = grub_be_to_wordXX (phdr->p_flags); > + phdr->p_offset = grub_be_to_offXX (phdr->p_offset); > + phdr->p_vaddr = grub_be_to_addrXX (phdr->p_vaddr); > + phdr->p_paddr = grub_be_to_addrXX (phdr->p_paddr); > + phdr->p_filesz = grub_be_to_XwordXX (phdr->p_filesz); > + phdr->p_memsz = grub_be_to_XwordXX (phdr->p_memsz); > + phdr->p_align = grub_be_to_XwordXX (phdr->p_align); > + } > + } #endif #if defined(GRUB_CPU_WORDS_BIGENDIAN) && defined(__powerpc__) (or whatever is needed to detect it) By the same logic code is needed on BE platforms only and so far only on PPC. > + else if (e->e_ident[EI_DATA] == ELFDATA2LSB) > + { > + e->e_type = grub_le_to_halfXX (e->e_type); > + e->e_machine = grub_le_to_halfXX (e->e_machine); > + e->e_version = grub_le_to_wordXX (e->e_version); > + e->e_entry = grub_le_to_addrXX (e->e_entry); > + e->e_phoff = grub_le_to_offXX (e->e_phoff); > + e->e_shoff = grub_le_to_offXX (e->e_shoff); > + e->e_flags = grub_le_to_wordXX (e->e_flags); > + e->e_ehsize = grub_le_to_halfXX (e->e_ehsize); > + e->e_phentsize = grub_le_to_halfXX (e->e_phentsize); > + e->e_phnum = grub_le_to_halfXX (e->e_phnum); > + e->e_shentsize = grub_le_to_halfXX (e->e_shentsize); > + e->e_shnum = grub_le_to_halfXX (e->e_shnum); > + e->e_shstrndx = grub_le_to_halfXX (e->e_shstrndx); > + > + FOR_ELFXX_PHDRS (elf,phdr) > + { > + phdr->p_type = grub_le_to_wordXX (phdr->p_type); > + phdr->p_flags = grub_le_to_wordXX (phdr->p_flags); > + phdr->p_offset = grub_le_to_offXX (phdr->p_offset); > + phdr->p_vaddr = grub_le_to_addrXX (phdr->p_vaddr); > + phdr->p_paddr = grub_le_to_addrXX (phdr->p_paddr); > + phdr->p_filesz = grub_le_to_XwordXX (phdr->p_filesz); > + phdr->p_memsz = grub_le_to_XwordXX (phdr->p_memsz); > + phdr->p_align = grub_le_to_XwordXX (phdr->p_align); > + } > + } > +} > + > +grub_err_t > +grub_elfXX_check_version (grub_elf_t elf) > +{ > + if (elf->ehdr.ehdrXX.e_version != EV_CURRENT) > + return grub_error (GRUB_ERR_BAD_OS, N_("invalid arch-independent ELF magic")); > + > + return GRUB_ERR_NONE; > +} This becomes redundant with suggested reorder. > -- > 1.8.1.4 > > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel