[PATCH 0/6] elf: add better bootm support

[PATCH 0/6] elf: add better bootm support

[Clement Leger]

Currently, when booting an elf file using bootm /dev/mtdx, bootm will
simply pass the file to the bootm and the read done on it will read the
entire flash partition. This series starts by some cleanup and then add an
elf_open function to load the elf size only based on the elf header.
A special handling for the elf file is also added in bootm data to allow
using directly the elf file structure. Finally the mips bootm is modified
to use this new elf member.

Clement Leger (6):
  common: elf: add computation of elf boundaries
  common: elf: fix warning on 32 bits architectures
  common: elf: split init to be reused from other function
  common: elf: add elf_open and elf_close
  common: bootm: add support for elf file loading
  mips: lib: bootm: use new data->elf member

 arch/mips/lib/bootm.c |  13 +-----
 common/bootm.c        |  27 +++++++++++
 common/elf.c          | 105 ++++++++++++++++++++++++++++++++++++++++--
 include/bootm.h       |   3 ++
 include/elf.h         |   9 ++++
 5 files changed, 141 insertions(+), 16 deletions(-)

-- 
2.17.1


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

[PATCH 1/6] common: elf: add computation of elf boundaries

[Clement Leger]

In order to correctly load an initrd or a device tree after an elf file,
we need to know its boundaries. This commit adds support for that and
allow the bootm implementations to use it for memory loading.

Signed-off-by: Clement Leger <cleger@kalray.eu>
---
 common/elf.c  | 7 +++++++
 include/elf.h | 2 ++
 2 files changed, 9 insertions(+)

diff --git a/common/elf.c b/common/elf.c
index 4733accb0..d64de401c 100644
--- a/common/elf.c
+++ b/common/elf.c
@@ -59,6 +59,11 @@ static int load_elf_phdr_segment(struct elf_image *elf, void *src,
 	if (!p_filesz)
 		return 0;
 
+	if (dst < elf->low_addr)
+		elf->low_addr = dst;
+	if (dst + p_memsz > elf->high_addr)
+		elf->high_addr = dst + p_memsz;
+
 	pr_debug("Loading phdr to 0x%p (%llu bytes)\n", dst, p_filesz);
 
 	ret = elf_request_region(elf, (resource_size_t)dst, p_filesz);
@@ -124,6 +129,8 @@ struct elf_image *elf_load_image(void *buf)
 	INIT_LIST_HEAD(&elf->list);
 
 	elf->buf = buf;
+	elf->low_addr = (void *) (unsigned long) -1;
+	elf->high_addr = 0;
 
 	ret = elf_check_image(elf);
 	if (ret)
diff --git a/include/elf.h b/include/elf.h
index 113728f08..e4577f63c 100644
--- a/include/elf.h
+++ b/include/elf.h
@@ -403,6 +403,8 @@ struct elf_image {
 	struct list_head list;
 	u8 class;
 	u64 entry;
+	void *low_addr;
+	void *high_addr;
 	void *buf;
 };
 
-- 
2.17.1


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

[PATCH 2/6] common: elf: fix warning on 32 bits architectures

[Clement Leger]

When pointers are 32 bits wide and we cast a potentially 64 bits
value in it, the compiler will yield an error. Cast that value first
into a phys_addr_t to match the architecture pointer size and then
in a void *.

Signed-off-by: Clement Leger <cleger@kalray.eu>
---
 common/elf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/elf.c b/common/elf.c
index d64de401c..55f5bc645 100644
--- a/common/elf.c
+++ b/common/elf.c
@@ -47,7 +47,7 @@ static void elf_release_regions(struct elf_image *elf)
 static int load_elf_phdr_segment(struct elf_image *elf, void *src,
 				 void *phdr)
 {
-	void *dst = (void *) elf_phdr_p_paddr(elf, phdr);
+	void *dst = (void *) (phys_addr_t) 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);
-- 
2.17.1


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

[PATCH 3/6] common: elf: split init to be reused from other function

[Clement Leger]

New elf_open function will also need to initialize an elf file. Split
this to avoid missing members initialization.

Signed-off-by: Clement Leger <cleger@kalray.eu>
---
 common/elf.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/common/elf.c b/common/elf.c
index 55f5bc645..5534632b2 100644
--- a/common/elf.c
+++ b/common/elf.c
@@ -119,6 +119,15 @@ static int elf_check_image(struct elf_image *elf)
 	return 0;
 }
 
+static int elf_check_init(struct elf_image *elf, void *buf)
+{
+	elf->buf = buf;
+	elf->low_addr = (void *) (unsigned long) -1;
+	elf->high_addr = 0;
+
+	return elf_check_image(elf);
+}
+
 struct elf_image *elf_load_image(void *buf)
 {
 	struct elf_image *elf;
@@ -128,13 +137,11 @@ struct elf_image *elf_load_image(void *buf)
 
 	INIT_LIST_HEAD(&elf->list);
 
-	elf->buf = buf;
-	elf->low_addr = (void *) (unsigned long) -1;
-	elf->high_addr = 0;
-
-	ret = elf_check_image(elf);
-	if (ret)
+	ret = elf_check_init(elf, buf);
+	if (ret) {
+		free(elf);
 		return ERR_PTR(ret);
+	}
 
 	ret = load_elf_image_phdr(elf);
 	if (ret) {
-- 
2.17.1


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

[PATCH 4/6] common: elf: add elf_open and elf_close

[Clement Leger]

When loading an elf file from a mtd device, this allows to parse the
header and load only the needed data according to the elf size. Without
that support, loading a elf file from a /dev/mtd would try to read the
entire partition.

Signed-off-by: Clement Leger <cleger@kalray.eu>
---
 common/elf.c  | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/elf.h |  7 +++++
 2 files changed, 94 insertions(+)

diff --git a/common/elf.c b/common/elf.c
index 5534632b2..8b36c2073 100644
--- a/common/elf.c
+++ b/common/elf.c
@@ -5,7 +5,12 @@
 
 #include <common.h>
 #include <elf.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <libfile.h>
 #include <memory.h>
+#include <unistd.h>
+#include <linux/fs.h>
 
 struct elf_section {
 	struct list_head list;
@@ -158,3 +163,85 @@ void elf_release_image(struct elf_image *elf)
 
 	free(elf);
 }
+
+static u64 elf_get_size(struct elf_image *elf)
+{
+	u64 sh_size = elf_hdr_e_shentsize(elf, elf->buf) *
+		      elf_hdr_e_shnum(elf, elf->buf);
+
+	/*
+	 * The section header table is located at the end of the elf file thus
+	 * we can take the offset and add the size of this table to obtain the
+	 * file size.
+	 */
+	return elf_hdr_e_shoff(elf, elf->buf) + sh_size;
+}
+
+struct elf_image *elf_open(const char *filename)
+{
+	int fd, ret;
+	u64 size;
+	struct elf64_hdr hdr;
+	struct elf_image *elf;
+	ssize_t read_ret;
+
+	fd = open(filename, O_RDONLY);
+	if (fd < 0) {
+		printf("could not open: %s\n", errno_str());
+		return ERR_PTR(-errno);
+	}
+
+	if (read(fd, &hdr, sizeof(hdr)) < 0) {
+		printf("could not read elf header: %s\n", errno_str());
+		ret = -errno;
+		goto err_close_fd;
+	}
+
+	elf = xzalloc(sizeof(*elf));
+
+	ret = elf_check_init(elf, &hdr);
+	if (ret) {
+		ret = -errno;
+		goto err_free_elf;
+	}
+
+	size = elf_get_size(elf);
+
+	elf->buf = xzalloc(size);
+
+	lseek(fd, 0, SEEK_SET);
+
+	read_ret = read(fd, elf->buf, size);
+	if (read_ret < 0) {
+		printf("could not read elf file: %s\n", errno_str());
+		ret = -errno;
+		goto err_free_buf;
+	} else if (read_ret != size) {
+		printf("could not read entire elf file\n");
+		ret = -EIO;
+		goto err_free_buf;
+	}
+
+	ret = load_elf_image_phdr(elf);
+	if (ret)
+		goto err_release_elf;
+
+	return elf;
+
+err_release_elf:
+	elf_release_regions(elf);
+err_free_buf:
+	free(elf->buf);
+err_free_elf:
+	free(elf);
+err_close_fd:
+	close(fd);
+
+	return ERR_PTR(ret);
+}
+
+void elf_close(struct elf_image *elf)
+{
+	free(elf->buf);
+	elf_release_image(elf);
+}
diff --git a/include/elf.h b/include/elf.h
index e4577f63c..3721bf6d0 100644
--- a/include/elf.h
+++ b/include/elf.h
@@ -411,6 +411,9 @@ struct elf_image {
 struct elf_image *elf_load_image(void *buf);
 void elf_release_image(struct elf_image *elf);
 
+struct elf_image *elf_open(const char *filename);
+void elf_close(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) \
@@ -422,6 +425,10 @@ static inline __type elf_##__s##_##__field(struct elf_image *elf, void *arg) { \
 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_shoff, u64)
+ELF_GET_FIELD(hdr, e_shentsize, u16)
+ELF_GET_FIELD(hdr, e_machine, u16)
+ELF_GET_FIELD(hdr, e_shnum, u16)
 ELF_GET_FIELD(hdr, e_type, u16)
 ELF_GET_FIELD(phdr, p_paddr, u64)
 ELF_GET_FIELD(phdr, p_filesz, u64)
-- 
2.17.1


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

Re: [PATCH 4/6] common: elf: add elf_open and elf_close

[Sascha Hauer]

On Wed, Apr 22, 2020 at 01:21:17PM +0200, Clement Leger wrote:
> When loading an elf file from a mtd device, this allows to parse the
> header and load only the needed data according to the elf size. Without
> that support, loading a elf file from a /dev/mtd would try to read the
> entire partition.
> 
> Signed-off-by: Clement Leger <cleger@kalray.eu>
> ---
>  common/elf.c  | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/elf.h |  7 +++++
>  2 files changed, 94 insertions(+)
> 
> diff --git a/common/elf.c b/common/elf.c
> index 5534632b2..8b36c2073 100644
> --- a/common/elf.c
> +++ b/common/elf.c
> @@ -5,7 +5,12 @@
>  
>  #include <common.h>
>  #include <elf.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <libfile.h>
>  #include <memory.h>
> +#include <unistd.h>
> +#include <linux/fs.h>
>  
>  struct elf_section {
>  	struct list_head list;
> @@ -158,3 +163,85 @@ void elf_release_image(struct elf_image *elf)
>  
>  	free(elf);
>  }
> +
> +static u64 elf_get_size(struct elf_image *elf)
> +{
> +	u64 sh_size = elf_hdr_e_shentsize(elf, elf->buf) *
> +		      elf_hdr_e_shnum(elf, elf->buf);
> +
> +	/*
> +	 * The section header table is located at the end of the elf file thus
> +	 * we can take the offset and add the size of this table to obtain the
> +	 * file size.
> +	 */
> +	return elf_hdr_e_shoff(elf, elf->buf) + sh_size;
> +}
> +
> +struct elf_image *elf_open(const char *filename)
> +{
> +	int fd, ret;
> +	u64 size;
> +	struct elf64_hdr hdr;
> +	struct elf_image *elf;
> +	ssize_t read_ret;
> +
> +	fd = open(filename, O_RDONLY);
> +	if (fd < 0) {
> +		printf("could not open: %s\n", errno_str());
> +		return ERR_PTR(-errno);
> +	}
> +
> +	if (read(fd, &hdr, sizeof(hdr)) < 0) {
> +		printf("could not read elf header: %s\n", errno_str());
> +		ret = -errno;
> +		goto err_close_fd;
> +	}
> +
> +	elf = xzalloc(sizeof(*elf));
> +
> +	ret = elf_check_init(elf, &hdr);
> +	if (ret) {
> +		ret = -errno;
> +		goto err_free_elf;
> +	}
> +
> +	size = elf_get_size(elf);
> +
> +	elf->buf = xzalloc(size);

You are allocating a potentially unbounded amount of memory. Please use
malloc rather than xzalloc here.

> +
> +	lseek(fd, 0, SEEK_SET);
> +
> +	read_ret = read(fd, elf->buf, size);
> +	if (read_ret < 0) {
> +		printf("could not read elf file: %s\n", errno_str());
> +		ret = -errno;
> +		goto err_free_buf;
> +	} else if (read_ret != size) {
> +		printf("could not read entire elf file\n");
> +		ret = -EIO;
> +		goto err_free_buf;
> +	}

read_full handles this case.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
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

[PATCH 5/6] common: bootm: add support for elf file loading

[Clement Leger]

This will allows elf loader to directly have an elf file available. Thus
filetype_elf bootm handlers will be able to use this elf file directly.

Signed-off-by: Clement Leger <cleger@kalray.eu>
---
 common/bootm.c  | 27 +++++++++++++++++++++++++++
 include/bootm.h |  3 +++
 2 files changed, 30 insertions(+)

diff --git a/common/bootm.c b/common/bootm.c
index 366f31455..015d90f3c 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -455,6 +455,8 @@ int bootm_get_os_size(struct image_data *data)
 {
 	int ret;
 
+	if (data->elf)
+		return data->elf->high_addr - data->elf->low_addr;
 	if (data->os)
 		return uimage_get_size(data->os, uimage_part_num(data->os_part));
 	if (data->os_fit)
@@ -502,6 +504,20 @@ static int bootm_open_os_uimage(struct image_data *data)
 	return 0;
 }
 
+static int bootm_open_elf(struct image_data *data)
+{
+
+	data->elf = elf_open(data->os_file);
+	if (IS_ERR(data->elf))
+		return PTR_ERR(data->elf);
+
+	printf("Entry Point:  %08llx\n", data->elf->entry);
+
+	data->os_address = data->elf->entry;
+
+	return 0;
+}
+
 static void bootm_print_info(struct image_data *data)
 {
 	if (data->os_res)
@@ -636,6 +652,15 @@ int bootm_boot(struct bootm_data *bootm_data)
 		}
 	}
 
+	if (os_type == filetype_elf) {
+		ret = bootm_open_elf(data);
+		if (ret) {
+			printf("Loading ELF image failed with: %s\n",
+					strerror(-ret));
+			goto err_out;
+		}
+	}
+
 	if (bootm_data->appendroot) {
 		char *rootarg;
 
@@ -705,6 +730,8 @@ err_out:
 		uimage_close(data->initrd);
 	if (data->os)
 		uimage_close(data->os);
+	if (data->elf)
+		elf_close(data->elf);
 	if (IS_ENABLED(CONFIG_FITIMAGE) && data->os_fit)
 		fit_close(data->os_fit);
 	if (data->of_root_node && data->of_root_node != of_get_root_node())
diff --git a/include/bootm.h b/include/bootm.h
index 7782de7a4..ef5148f31 100644
--- a/include/bootm.h
+++ b/include/bootm.h
@@ -46,6 +46,9 @@ struct image_data {
 	/* if os is an uImage this will be provided */
 	struct uimage_handle *os;
 
+	/* if os is an elf file this will be provided */
+	struct elf_image *elf;
+
 	/* if os is a FIT image this will be provided */
 	struct fit_handle *os_fit;
 
-- 
2.17.1


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

Re: [PATCH 5/6] common: bootm: add support for elf file loading

[Clément Leger]

----- On 22 Apr, 2020, at 13:21, Clément Leger cleger@kalray.eu wrote:

> This will allows elf loader to directly have an elf file available. Thus
> filetype_elf bootm handlers will be able to use this elf file directly.
> 
> Signed-off-by: Clement Leger <cleger@kalray.eu>
> ---
> common/bootm.c  | 27 +++++++++++++++++++++++++++
> include/bootm.h |  3 +++
> 2 files changed, 30 insertions(+)
> 
> diff --git a/common/bootm.c b/common/bootm.c
> index 366f31455..015d90f3c 100644
> --- a/common/bootm.c
> +++ b/common/bootm.c
> @@ -455,6 +455,8 @@ int bootm_get_os_size(struct image_data *data)
> {
> 	int ret;
> 
> +	if (data->elf)
> +		return data->elf->high_addr - data->elf->low_addr;
> 	if (data->os)
> 		return uimage_get_size(data->os, uimage_part_num(data->os_part));
> 	if (data->os_fit)
> @@ -502,6 +504,20 @@ static int bootm_open_os_uimage(struct image_data *data)
> 	return 0;
> }
> 
> +static int bootm_open_elf(struct image_data *data)
> +{
> +
> +	data->elf = elf_open(data->os_file);
> +	if (IS_ERR(data->elf))
> +		return PTR_ERR(data->elf);

I just noticed that data->elf should be set to NULL here on error because on
error, the free is done if data->elf != NULL. AFAIK, bootm_open_os_uimage
suffers of the same flaw. Maybe the best place to reset the elf/os members
is in the bootm_boot command in case of error.

> +
> +	printf("Entry Point:  %08llx\n", data->elf->entry);
> +
> +	data->os_address = data->elf->entry;
> +
> +	return 0;
> +}
> +
> static void bootm_print_info(struct image_data *data)
> {
> 	if (data->os_res)
> @@ -636,6 +652,15 @@ int bootm_boot(struct bootm_data *bootm_data)
> 		}
> 	}
> 
> +	if (os_type == filetype_elf) {
> +		ret = bootm_open_elf(data);
> +		if (ret) {
> +			printf("Loading ELF image failed with: %s\n",
> +					strerror(-ret));
> +			goto err_out;
> +		}
> +	}
> +
> 	if (bootm_data->appendroot) {
> 		char *rootarg;
> 
> @@ -705,6 +730,8 @@ err_out:
> 		uimage_close(data->initrd);
> 	if (data->os)
> 		uimage_close(data->os);
> +	if (data->elf)
> +		elf_close(data->elf);
> 	if (IS_ENABLED(CONFIG_FITIMAGE) && data->os_fit)
> 		fit_close(data->os_fit);
> 	if (data->of_root_node && data->of_root_node != of_get_root_node())
> diff --git a/include/bootm.h b/include/bootm.h
> index 7782de7a4..ef5148f31 100644
> --- a/include/bootm.h
> +++ b/include/bootm.h
> @@ -46,6 +46,9 @@ struct image_data {
> 	/* if os is an uImage this will be provided */
> 	struct uimage_handle *os;
> 
> +	/* if os is an elf file this will be provided */
> +	struct elf_image *elf;
> +
> 	/* if os is a FIT image this will be provided */
> 	struct fit_handle *os_fit;
> 
> --
> 2.17.1

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

Re: [PATCH 5/6] common: bootm: add support for elf file loading

[Sascha Hauer]

On Wed, Apr 22, 2020 at 01:21:18PM +0200, Clement Leger wrote:
> This will allows elf loader to directly have an elf file available. Thus
> filetype_elf bootm handlers will be able to use this elf file directly.
> 
> Signed-off-by: Clement Leger <cleger@kalray.eu>
> ---
>  common/bootm.c  | 27 +++++++++++++++++++++++++++
>  include/bootm.h |  3 +++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/common/bootm.c b/common/bootm.c
> index 366f31455..015d90f3c 100644
> --- a/common/bootm.c
> +++ b/common/bootm.c
> @@ -455,6 +455,8 @@ int bootm_get_os_size(struct image_data *data)
>  {
>  	int ret;
>  
> +	if (data->elf)
> +		return data->elf->high_addr - data->elf->low_addr;
>  	if (data->os)
>  		return uimage_get_size(data->os, uimage_part_num(data->os_part));
>  	if (data->os_fit)
> @@ -502,6 +504,20 @@ static int bootm_open_os_uimage(struct image_data *data)
>  	return 0;
>  }
>  
> +static int bootm_open_elf(struct image_data *data)
> +{
> +
> +	data->elf = elf_open(data->os_file);
> +	if (IS_ERR(data->elf))
> +		return PTR_ERR(data->elf);
> +
> +	printf("Entry Point:  %08llx\n", data->elf->entry);
> +
> +	data->os_address = data->elf->entry;
> +
> +	return 0;
> +}
> +

This will give you linker errors when elf support is not compiled in.

	if (!IS_ENABLED(CONFIG_ELF))
		return -ENOSYS;

might be enough already, plus maybe an bootm kconfig option which
explicitly selects CONFIG_ELF when desired.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
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

[PATCH 6/6] mips: lib: bootm: use new data->elf member

[Clement Leger]

Now that the elf file is loaded by the bootm core, use this field
directly instead of manually loading the elf file.

Signed-off-by: Clement Leger <cleger@kalray.eu>
---
 arch/mips/lib/bootm.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/arch/mips/lib/bootm.c b/arch/mips/lib/bootm.c
index 5bb09cc2d..1e5fdd4ae 100644
--- a/arch/mips/lib/bootm.c
+++ b/arch/mips/lib/bootm.c
@@ -46,18 +46,10 @@ static struct binfmt_hook binfmt_barebox_hook = {
 static int do_bootm_elf(struct image_data *data)
 {
 	void (*entry)(int, void *);
-	struct elf_image *elf;
-	void *fdt, *buf;
+	struct elf_image *elf = data->elf;
+	void *fdt;
 	int ret = 0;
 
-	buf = read_file(data->os_file, NULL);
-	if (!buf)
-		return -EINVAL;
-
-	elf = elf_load_image(buf);
-	if (IS_ERR(elf))
-		return PTR_ERR(elf);
-
 	fdt = bootm_get_devicetree(data);
 	if (IS_ERR(fdt)) {
 		ret = PTR_ERR(fdt);
@@ -82,7 +74,6 @@ static int do_bootm_elf(struct image_data *data)
 bootm_elf_done:
 	elf_release_image(elf);
 	free(fdt);
-	free(buf);
 
 	return ret;
 }
-- 
2.17.1


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