Re: [PATCH] elf: add 64 bits elf support

[Sascha Hauer <s.hauer@pengutronix.de>]

Hi Clément,

I guess this version looks good. Could you resend with a Signed-off-by
tag?

Sascha

On Wed, Mar 20, 2019 at 05:28:44PM +0100, Clément Leger wrote:
> Here is a V2 which uses correct type for elf header access macros
> (instead of simply unsigned long). Moreover types used are now
> of fixed size type (u64 instead of unsigned long). This could
> potentially allow a 32bit barebox to load a 64bit elf using some custom
> hardware which support 64bit addressing (DMA or such thing).
> 
> 
> This patch add elf64 loading support to the elf loader. Since
> elf32 and elf64 uses completely different types, to avoid copying all
> the code and simply replace elf32 with elf64, use a macro which will
> return the appropriate field for each type of header. This macro
> generates getter for elf structures according to the class of the loaded
> elf.
> All direct elf struct dereference are then replaced by call to generated
> functions. This allows to keep a common loader code even if types are
> different.
> ---
>  common/elf.c  | 45 +++++++++++++++++++++++----------------------
>  include/elf.h | 29 ++++++++++++++++++++++++++++-
>  2 files changed, 51 insertions(+), 23 deletions(-)
> 
> diff --git a/common/elf.c b/common/elf.c
> index 8edf38856..4733accb0 100644
> --- a/common/elf.c
> +++ b/common/elf.c
> @@ -45,29 +45,31 @@ static void elf_release_regions(struct elf_image *elf)
>  
>  
>  static int load_elf_phdr_segment(struct elf_image *elf, void *src,
> -				 Elf32_Phdr *phdr)
> +				 void *phdr)
>  {
> -	void *dst = (void *)phdr->p_paddr;
> +	void *dst = (void *) elf_phdr_p_paddr(elf, phdr);
>  	int ret;
> +	u64 p_filesz = elf_phdr_p_filesz(elf, phdr);
> +	u64 p_memsz = elf_phdr_p_memsz(elf, phdr);
>  
>  	/* we care only about PT_LOAD segments */
> -	if (phdr->p_type != PT_LOAD)
> +	if (elf_phdr_p_type(elf, phdr) != PT_LOAD)
>  		return 0;
>  
> -	if (!phdr->p_filesz)
> +	if (!p_filesz)
>  		return 0;
>  
> -	pr_debug("Loading phdr to 0x%p (%i bytes)\n", dst, phdr->p_filesz);
> +	pr_debug("Loading phdr to 0x%p (%llu bytes)\n", dst, p_filesz);
>  
> -	ret = elf_request_region(elf, (resource_size_t)dst, phdr->p_filesz);
> +	ret = elf_request_region(elf, (resource_size_t)dst, p_filesz);
>  	if (ret)
>  		return ret;
>  
> -	memcpy(dst, src, phdr->p_filesz);
> +	memcpy(dst, src, p_filesz);
>  
> -	if (phdr->p_filesz < phdr->p_memsz)
> -		memset(dst + phdr->p_filesz, 0x00,
> -		       phdr->p_memsz - phdr->p_filesz);
> +	if (p_filesz < p_memsz)
> +		memset(dst + p_filesz, 0x00,
> +		       p_memsz - p_filesz);
>  
>  	return 0;
>  }
> @@ -75,14 +77,13 @@ static int load_elf_phdr_segment(struct elf_image *elf, void *src,
>  static int load_elf_image_phdr(struct elf_image *elf)
>  {
>  	void *buf = elf->buf;
> -	Elf32_Ehdr *ehdr = buf;
> -	Elf32_Phdr *phdr = (Elf32_Phdr *)(buf + ehdr->e_phoff);
> +	void *phdr = (void *) (buf + elf_hdr_e_phoff(elf, buf));
>  	int i, ret;
>  
> -	elf->entry = ehdr->e_entry;
> +	elf->entry = elf_hdr_e_entry(elf, buf);
>  
> -	for (i = 0; i < ehdr->e_phnum; ++i) {
> -		void *src = buf + phdr->p_offset;
> +	for (i = 0; i < elf_hdr_e_phnum(elf, buf) ; ++i) {
> +		void *src = buf + elf_phdr_p_offset(elf, phdr);
>  
>  		ret = load_elf_phdr_segment(elf, src, phdr);
>  		/* in case of error elf_load_image() caller should clean up and
> @@ -90,22 +91,22 @@ static int load_elf_image_phdr(struct elf_image *elf)
>  		if (ret)
>  			return ret;
>  
> -		++phdr;
> +		phdr += elf_size_of_phdr(elf);
>  	}
>  
>  	return 0;
>  }
>  
> -static int elf_check_image(void *buf)
> +static int elf_check_image(struct elf_image *elf)
>  {
> -	Elf32_Ehdr *ehdr = (Elf32_Ehdr *)buf;
> -
> -	if (strncmp(buf, ELFMAG, SELFMAG)) {
> +	if (strncmp(elf->buf, ELFMAG, SELFMAG)) {
>  		pr_err("ELF magic not found.\n");
>  		return -EINVAL;
>  	}
>  
> -	if (ehdr->e_type != ET_EXEC) {
> +	elf->class = ((char *) elf->buf)[EI_CLASS];
> +
> +	if (elf_hdr_e_type(elf, elf->buf) != ET_EXEC) {
>  		pr_err("Non EXEC ELF image.\n");
>  		return -ENOEXEC;
>  	}
> @@ -124,7 +125,7 @@ struct elf_image *elf_load_image(void *buf)
>  
>  	elf->buf = buf;
>  
> -	ret = elf_check_image(buf);
> +	ret = elf_check_image(elf);
>  	if (ret)
>  		return ERR_PTR(ret);
>  
> diff --git a/include/elf.h b/include/elf.h
> index 92c8d9c12..633f4992d 100644
> --- a/include/elf.h
> +++ b/include/elf.h
> @@ -400,11 +400,38 @@ static inline void arch_write_notes(struct file *file) { }
>  
>  struct elf_image {
>  	struct list_head list;
> -	unsigned long entry;
> +	u8 class;
> +	u64 entry;
>  	void *buf;
>  };
>  
>  struct elf_image *elf_load_image(void *buf);
>  void elf_release_image(struct elf_image *elf);
>  
> +#define ELF_GET_FIELD(__s, __field, __type) \
> +static inline __type elf_##__s##_##__field(struct elf_image *elf, void *arg) { \
> +	if (elf->class == ELFCLASS32) \
> +		return (__type) ((struct elf32_##__s *) arg)->__field; \
> +	else \
> +		return (__type) ((struct elf64_##__s *) arg)->__field; \
> +}
> +
> +ELF_GET_FIELD(hdr, e_entry, u64)
> +ELF_GET_FIELD(hdr, e_phnum, u16)
> +ELF_GET_FIELD(hdr, e_phoff, u64)
> +ELF_GET_FIELD(hdr, e_type, u16)
> +ELF_GET_FIELD(phdr, p_paddr, u64)
> +ELF_GET_FIELD(phdr, p_filesz, u64)
> +ELF_GET_FIELD(phdr, p_memsz, u64)
> +ELF_GET_FIELD(phdr, p_type, u32)
> +ELF_GET_FIELD(phdr, p_offset, u64)
> +
> +static inline unsigned long elf_size_of_phdr(struct elf_image *elf)
> +{
> +	if (elf->class == ELFCLASS32)
> +		return sizeof(Elf32_Phdr);
> +	else
> +		return sizeof(Elf64_Phdr);
> +}
> +
>  #endif /* _LINUX_ELF_H */
> -- 
> 2.15.0.276.g89ea799
> 
> 
> 
> 
> > Hi Sascha,
> > 
> >> Hi Clément,
> >> 
> >> On Mon, Mar 18, 2019 at 08:16:47PM +0100, Clément Leger wrote:
> >>> This patch add elf64 loading support to the elf loader. Since
> >>> elf32 and elf64 uses completely different types, to avoid copying all
> >>> the code and simply replace elf32 with elf64, use a macro which will
> >>> return the appropriate field for each type of header. This macro
> >>> generates getter for elf structures according to the class of the loaded
> >>> elf.
> >>> All direct elf struct dereference are then replaced by call to generated
> >>> functions. This allows to keep a common loader code even if types are
> >>> different.
> >>> 
> >>> Signed-off-by: Clément Léger <clement.leger@kalray.eu>
> >>> ---
> >>>  common/elf.c  | 46 +++++++++++++++++++++++-----------------------
> >>>  include/elf.h | 27 +++++++++++++++++++++++++++
> >>>  2 files changed, 50 insertions(+), 23 deletions(-)
> >>> 
> >>> diff --git a/common/elf.c b/common/elf.c
> >>> index 8edf38856..bfb878954 100644
> >>> --- a/common/elf.c
> >>> +++ b/common/elf.c
> >>> @@ -43,31 +43,32 @@ static void elf_release_regions(struct elf_image *elf)
> >>>  	}
> >>>  }
> >>>  
> >>> -
> >>>  static int load_elf_phdr_segment(struct elf_image *elf, void *src,
> >>> -				 Elf32_Phdr *phdr)
> >>> +				 void *phdr)
> >>>  {
> >>> -	void *dst = (void *)phdr->p_paddr;
> >>> +	void *dst = (void *) elf_phdr_p_paddr(elf, phdr);
> >>>  	int ret;
> >>> +	unsigned long p_filesz = elf_phdr_p_filesz(elf, phdr);
> >>> +	unsigned long p_memsz = elf_phdr_p_memsz(elf, phdr);
> >>>  
> >>>  	/* we care only about PT_LOAD segments */
> >>> -	if (phdr->p_type != PT_LOAD)
> >>> +	if (elf_phdr_p_type(elf, phdr) != PT_LOAD)
> >>>  		return 0;
> >>>  
> >>> -	if (!phdr->p_filesz)
> >>> +	if (!p_filesz)
> >>>  		return 0;
> >>>  
> >>> -	pr_debug("Loading phdr to 0x%p (%i bytes)\n", dst, phdr->p_filesz);
> >>> +	pr_debug("Loading phdr to 0x%p (%ld bytes)\n", dst, p_filesz);
> >> 
> >> %lu for p_filesz?
> > 
> > Indeed, I missed this one.
> > 
> >> 
> >>> @@ -400,6 +400,7 @@ static inline void arch_write_notes(struct file *file) { }
> >>>  
> >>>  struct elf_image {
> >>>  	struct list_head list;
> >>> +	unsigned long class;
> >>>  	unsigned long entry;
> >>>  	void *buf;
> >>>  };
> >>> @@ -407,4 +408,30 @@ struct elf_image {
> >>>  struct elf_image *elf_load_image(void *buf);
> >>>  void elf_release_image(struct elf_image *elf);
> >>>  
> >>> +#define ELF_GET_FIELD(__s, __field, __type) \
> >>> +static inline __type elf_##__s##_##__field(struct elf_image *elf, void *arg) {
> >>> \
> >>> +	if (elf->class == ELFCLASS32) \
> >>> +		return (__type) ((struct elf32_##__s *) arg)->__field; \
> >>> +	else \
> >>> +		return (__type) ((struct elf64_##__s *) arg)->__field; \
> >>> +}
> >>> +
> >>> +ELF_GET_FIELD(hdr, e_entry, unsigned long)
> >>> +ELF_GET_FIELD(hdr, e_phnum, unsigned long)
> >>> +ELF_GET_FIELD(hdr, e_phoff, unsigned long)
> >>> +ELF_GET_FIELD(hdr, e_type, unsigned long)
> >>> +ELF_GET_FIELD(phdr, p_paddr, unsigned long)
> >>> +ELF_GET_FIELD(phdr, p_filesz, unsigned long)
> >>> +ELF_GET_FIELD(phdr, p_memsz, unsigned long)
> >>> +ELF_GET_FIELD(phdr, p_type, unsigned long)
> >>> +ELF_GET_FIELD(phdr, p_offset, unsigned long)
> >> 
> >> When it's always unsigned long why do we have to pass in the type as an
> >> argument?
> > 
> > Actually, some of them should not be defined as I did.
> > For instance, the e_type is an half in both elf32 and elf64 so it should
> > be defined as u16.
> > 
> > Some other approaches to handle both 64bits/32bits elf were to copy
> > the whole loading code and s/elf32/elf64. Since the code in barebox is
> > not so big, maybe I could do that.
> > 
> >> 
> >> I am undecided if this is the right approach. "unsigned long" is wrong
> >> when a ELF file for a foreign architecture is loaded. This can happen
> >> for example when code for the Cortex M4 cores is loaded from the 64bit
> >> Cortex A cores is loaded on an i.MX8 for example. Using the bigger types
> >> then is not a problem, but maybe it could happen the other way round,
> >> loading a 64bit ELF on a 32bit architecture?
> > 
> > I was thinking about this one. I tried loading 32bit and 64bit elf from
> > a 64bit core but indeed, not the other way. If so, then addresses will
> > be truncated but since the processor will not be able to access a
> > 64 bits memory space, I guess it's not possible (unless you have some
> > DMA which can access the upper memory but this will probably not be
> > handled by barebox elf loader).
> > 
> >> 
> >> I can't see a real problem here, I just wanted to note. Are there other
> >> opinions?
> >> 
> >> Sascha
> >> 
> >> --
> >> Pengutronix e.K.                           |                             |
> >> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> >> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> >> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> > 
> > _______________________________________________
> > barebox mailing list
> > barebox@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/barebox
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox