Re: [RFC PATCH 3/3] Suport for bi-endianess in elf file

[Andrei Borzenkov <arvidjaar@gmail.com>]

On Thu, Aug 28, 2014 at 10:56 PM, Paulo Flabiano Smorigo
<pfsmorigo@linux.vnet.ibm.com> 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 <tbberry@us.ibm.com>
> ---
>  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