[PATCH 0/3] kexec-tools: multiboot2: Accept x86-64 images

[PATCH 0/3] kexec-tools: multiboot2: Accept x86-64 images

[Zhaofeng Li]

Hi Simon,

This patch series adds support for loading x86-64 ELF images with
the multiboot2 header and fixes a couple of related issues.

Tested by loading a custom research OS kernel. Thank you for your
review!

Zhaofeng

Zhaofeng Li (3):
  multiboot2: Correct MBI size calculation
  multiboot2: Use rel_min and rel_max for mbi destination
  multiboot2: Accept x86-64 images

 kexec/arch/i386/kexec-mb2-x86.c  | 51 +++++++++++++++++++++++++++-----
 kexec/arch/x86_64/kexec-x86_64.c |  4 +--
 2 files changed, 46 insertions(+), 9 deletions(-)

-- 
2.32.0


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

[PATCH 1/3] multiboot2: Correct MBI size calculation

[Zhaofeng Li]

tag_load_base_addr is dependent on rel_tag, and tag_framebuffer was
not accounted for.

Signed-off-by: Zhaofeng Li <hello@zhaofeng.li>
---
 kexec/arch/i386/kexec-mb2-x86.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/kexec/arch/i386/kexec-mb2-x86.c b/kexec/arch/i386/kexec-mb2-x86.c
index b4996bc..c10c0ef 100644
--- a/kexec/arch/i386/kexec-mb2-x86.c
+++ b/kexec/arch/i386/kexec-mb2-x86.c
@@ -115,17 +115,24 @@ void multiboot2_x86_usage(void)
 static size_t
 multiboot2_get_mbi_size(int ranges, int cmdline_size, int modcount, int modcmd_size)
 {
-	return (2 * sizeof (uint32_t) + sizeof (struct multiboot_tag)
-		+ sizeof (struct multiboot_tag)
+	size_t mbi_size = (2 * sizeof (uint32_t) /* u32 total_size, u32 reserved */
 		+ ALIGN_UP (sizeof (struct multiboot_tag_basic_meminfo), MULTIBOOT_TAG_ALIGN)
 		+ ALIGN_UP ((sizeof (struct multiboot_tag_mmap)
 			+ ranges * sizeof (struct multiboot_mmap_entry)), MULTIBOOT_TAG_ALIGN)
-		+ ALIGN_UP (sizeof (struct multiboot_tag_load_base_addr), MULTIBOOT_TAG_ALIGN)
 		+ (sizeof (struct multiboot_tag_string)
 			+ ALIGN_UP (cmdline_size, MULTIBOOT_TAG_ALIGN))
 		+ (sizeof (struct multiboot_tag_string)
 			+ ALIGN_UP (strlen(BOOTLOADER " " BOOTLOADER_VERSION) + 1, MULTIBOOT_TAG_ALIGN))
-		+ (modcount * sizeof (struct multiboot_tag_module) + modcmd_size));
+		+ (modcount * sizeof (struct multiboot_tag_module) + modcmd_size))
+		+ sizeof (struct multiboot_tag); /* end tag */
+
+	if (mhi.rel_tag) mbi_size +=
+		  ALIGN_UP (sizeof (struct multiboot_tag_load_base_addr), MULTIBOOT_TAG_ALIGN);
+
+	if (mhi.fb_tag) mbi_size +=
+		  ALIGN_UP (sizeof (struct multiboot_tag_framebuffer), MULTIBOOT_TAG_ALIGN);
+
+	return mbi_size;
 }
 
 static void multiboot2_read_header_tags(void)
-- 
2.32.0


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

[PATCH 2/3] multiboot2: Use rel_min and rel_max for mbi destination

[Zhaofeng Li]

This would segfault if mhi.rel_tag didn't exist.

Signed-off-by: Zhaofeng Li <hello@zhaofeng.li>
---
 kexec/arch/i386/kexec-mb2-x86.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kexec/arch/i386/kexec-mb2-x86.c b/kexec/arch/i386/kexec-mb2-x86.c
index c10c0ef..49fcc2d 100644
--- a/kexec/arch/i386/kexec-mb2-x86.c
+++ b/kexec/arch/i386/kexec-mb2-x86.c
@@ -600,7 +600,7 @@ int multiboot2_x86_load(int argc, char **argv, const char *buf, off_t len,
 		return -1;
 
 	addr = add_buffer(info, mbi_buf, mbi_bytes, mbi_bytes, 4,
-			  mhi.rel_tag->min_addr, mhi.rel_tag->max_addr, 1);
+			  rel_min, rel_max, 1);
 
 	elf_rel_get_symbol(&info->rhdr, "entry32_regs", &regs, sizeof(regs));
 	regs.eax = MULTIBOOT2_BOOTLOADER_MAGIC;
-- 
2.32.0


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

[PATCH 3/3] multiboot2: Accept x86-64 images

[Zhaofeng Li]

Signed-off-by: Zhaofeng Li <hello@zhaofeng.li>
---
 kexec/arch/i386/kexec-mb2-x86.c  | 34 ++++++++++++++++++++++++++++++--
 kexec/arch/x86_64/kexec-x86_64.c |  4 ++--
 2 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/kexec/arch/i386/kexec-mb2-x86.c b/kexec/arch/i386/kexec-mb2-x86.c
index 49fcc2d..31b4ce1 100644
--- a/kexec/arch/i386/kexec-mb2-x86.c
+++ b/kexec/arch/i386/kexec-mb2-x86.c
@@ -72,12 +72,42 @@ struct multiboot2_header_info {
 #define ALIGN_UP(addr, align) \
 	((addr + (typeof (addr)) align - 1) & ~((typeof (addr)) align - 1))
 
+static const int probe_debug = 0;
+
+static int elf_x86_or_x64_probe(const char *buf, off_t len)
+{
+	struct mem_ehdr ehdr;
+	int result;
+	result = build_elf_exec_info(buf, len, &ehdr, 0);
+	if (result < 0) {
+		if (probe_debug) {
+			fprintf(stderr, "Not an ELF executable\n");
+		}
+		goto out;
+	}
+
+	/* Verify the architecuture specific bits */
+	if ((ehdr.e_machine != EM_386) && (ehdr.e_machine != EM_486) && (ehdr.e_machine != EM_X86_64)) {
+		/* for a different architecture */
+		if (probe_debug) {
+			fprintf(stderr, "Not i386 or x86_64 ELF executable\n");
+		}
+		result = -1;
+		goto out;
+	}
+	result = 0;
+ out:
+	free_elf_info(&ehdr);
+	return result;
+}
+
 int multiboot2_x86_probe(const char *buf, off_t buf_len)
 /* Is it a good idea to try booting this file? */
 {
 	int i, len;
-	/* First of all, check that this is an ELF file */
-	if ((i=elf_x86_probe(buf, buf_len)) < 0)
+
+	/* First of all, check that this is an ELF file for either x86 or x86-64 */
+	if ((i=elf_x86_or_x64_probe(buf, buf_len)) < 0)
 		return i;
 
 	/* Now look for a multiboot header. */
diff --git a/kexec/arch/x86_64/kexec-x86_64.c b/kexec/arch/x86_64/kexec-x86_64.c
index 394cfca..ffd84f0 100644
--- a/kexec/arch/x86_64/kexec-x86_64.c
+++ b/kexec/arch/x86_64/kexec-x86_64.c
@@ -33,11 +33,11 @@
 #include <arch/options.h>
 
 struct file_type file_type[] = {
+	{ "multiboot2-x86", multiboot2_x86_probe, multiboot2_x86_load,
+	  multiboot2_x86_usage },
 	{ "elf-x86_64", elf_x86_64_probe, elf_x86_64_load, elf_x86_64_usage },
 	{ "multiboot-x86", multiboot_x86_probe, multiboot_x86_load,
 	  multiboot_x86_usage },
-	{ "multiboot2-x86", multiboot2_x86_probe, multiboot2_x86_load,
-	  multiboot2_x86_usage },
 	{ "elf-x86", elf_x86_probe, elf_x86_load, elf_x86_usage },
 	{ "bzImage64", bzImage64_probe, bzImage64_load, bzImage64_usage },
 	{ "bzImage", bzImage_probe, bzImage_load, bzImage_usage },
-- 
2.32.0


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

Re: [PATCH 2/3] multiboot2: Use rel_min and rel_max for mbi destination

[Simon Horman]

On Fri, Sep 10, 2021 at 07:48:59PM -0700, Zhaofeng Li wrote:
> This would segfault if mhi.rel_tag didn't exist.
> 
> Signed-off-by: Zhaofeng Li <hello@zhaofeng.li>

Does the same problem exist a little further up, around line 574, also
in multiboot2_x86_load() ?

                        /* Pick the next aligned spot to load it in. Always page align. */
                        addr = add_buffer(info, buf, mod_size, mod_size, getpagesize(),
                                          mhi.rel_tag->min_addr, mhi.rel_tag->max_addr, 1);

> ---
>  kexec/arch/i386/kexec-mb2-x86.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kexec/arch/i386/kexec-mb2-x86.c b/kexec/arch/i386/kexec-mb2-x86.c
> index c10c0ef..49fcc2d 100644
> --- a/kexec/arch/i386/kexec-mb2-x86.c
> +++ b/kexec/arch/i386/kexec-mb2-x86.c
> @@ -600,7 +600,7 @@ int multiboot2_x86_load(int argc, char **argv, const char *buf, off_t len,
>  		return -1;
>  
>  	addr = add_buffer(info, mbi_buf, mbi_bytes, mbi_bytes, 4,
> -			  mhi.rel_tag->min_addr, mhi.rel_tag->max_addr, 1);
> +			  rel_min, rel_max, 1);
>  
>  	elf_rel_get_symbol(&info->rhdr, "entry32_regs", &regs, sizeof(regs));
>  	regs.eax = MULTIBOOT2_BOOTLOADER_MAGIC;
> -- 
> 2.32.0
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
> 

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

Re: [PATCH 3/3] multiboot2: Accept x86-64 images

[Simon Horman]

On Fri, Sep 10, 2021 at 07:49:00PM -0700, Zhaofeng Li wrote:
> Signed-off-by: Zhaofeng Li <hello@zhaofeng.li>
> ---
>  kexec/arch/i386/kexec-mb2-x86.c  | 34 ++++++++++++++++++++++++++++++--
>  kexec/arch/x86_64/kexec-x86_64.c |  4 ++--
>  2 files changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/kexec/arch/i386/kexec-mb2-x86.c b/kexec/arch/i386/kexec-mb2-x86.c
> index 49fcc2d..31b4ce1 100644
> --- a/kexec/arch/i386/kexec-mb2-x86.c
> +++ b/kexec/arch/i386/kexec-mb2-x86.c
> @@ -72,12 +72,42 @@ struct multiboot2_header_info {
>  #define ALIGN_UP(addr, align) \
>  	((addr + (typeof (addr)) align - 1) & ~((typeof (addr)) align - 1))
>  
> +static const int probe_debug = 0;
> +
> +static int elf_x86_or_x64_probe(const char *buf, off_t len)

There is a lot of duplication between at least this function,
elf_x86_probe and elf_x86_64_probe. I wonder if there is any value in
reconciling that.

> +{
> +	struct mem_ehdr ehdr;
> +	int result;
> +	result = build_elf_exec_info(buf, len, &ehdr, 0);
> +	if (result < 0) {
> +		if (probe_debug) {
> +			fprintf(stderr, "Not an ELF executable\n");
> +		}
> +		goto out;
> +	}
> +
> +	/* Verify the architecuture specific bits */
> +	if ((ehdr.e_machine != EM_386) && (ehdr.e_machine != EM_486) && (ehdr.e_machine != EM_X86_64)) {

nit: line-wrap please

> +		/* for a different architecture */
> +		if (probe_debug) {
> +			fprintf(stderr, "Not i386 or x86_64 ELF executable\n");
> +		}
> +		result = -1;
> +		goto out;
> +	}
> +	result = 0;
> + out:
> +	free_elf_info(&ehdr);
> +	return result;
> +}
> +
>  int multiboot2_x86_probe(const char *buf, off_t buf_len)
>  /* Is it a good idea to try booting this file? */
>  {
>  	int i, len;
> -	/* First of all, check that this is an ELF file */
> -	if ((i=elf_x86_probe(buf, buf_len)) < 0)
> +
> +	/* First of all, check that this is an ELF file for either x86 or x86-64 */
> +	if ((i=elf_x86_or_x64_probe(buf, buf_len)) < 0)

nit: as we are touching this line could we split the assignment and test of i?

	i = ...;
	if (i < 0)

>  		return i;
>  
>  	/* Now look for a multiboot header. */
> diff --git a/kexec/arch/x86_64/kexec-x86_64.c b/kexec/arch/x86_64/kexec-x86_64.c
> index 394cfca..ffd84f0 100644
> --- a/kexec/arch/x86_64/kexec-x86_64.c
> +++ b/kexec/arch/x86_64/kexec-x86_64.c
> @@ -33,11 +33,11 @@
>  #include <arch/options.h>
>  
>  struct file_type file_type[] = {
> +	{ "multiboot2-x86", multiboot2_x86_probe, multiboot2_x86_load,
> +	  multiboot2_x86_usage },
>  	{ "elf-x86_64", elf_x86_64_probe, elf_x86_64_load, elf_x86_64_usage },
>  	{ "multiboot-x86", multiboot_x86_probe, multiboot_x86_load,
>  	  multiboot_x86_usage },
> -	{ "multiboot2-x86", multiboot2_x86_probe, multiboot2_x86_load,
> -	  multiboot2_x86_usage },
>  	{ "elf-x86", elf_x86_probe, elf_x86_load, elf_x86_usage },
>  	{ "bzImage64", bzImage64_probe, bzImage64_load, bzImage64_usage },
>  	{ "bzImage", bzImage_probe, bzImage_load, bzImage_usage },
> -- 
> 2.32.0
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
> 

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

Re: [PATCH 1/3] multiboot2: Correct MBI size calculation

[Simon Horman]

On Fri, Sep 10, 2021 at 07:48:58PM -0700, Zhaofeng Li wrote:
> tag_load_base_addr is dependent on rel_tag, and tag_framebuffer was
> not accounted for.
> 
> Signed-off-by: Zhaofeng Li <hello@zhaofeng.li>
> ---
>  kexec/arch/i386/kexec-mb2-x86.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/kexec/arch/i386/kexec-mb2-x86.c b/kexec/arch/i386/kexec-mb2-x86.c
> index b4996bc..c10c0ef 100644
> --- a/kexec/arch/i386/kexec-mb2-x86.c
> +++ b/kexec/arch/i386/kexec-mb2-x86.c
> @@ -115,17 +115,24 @@ void multiboot2_x86_usage(void)
>  static size_t
>  multiboot2_get_mbi_size(int ranges, int cmdline_size, int modcount, int modcmd_size)
>  {
> -	return (2 * sizeof (uint32_t) + sizeof (struct multiboot_tag)
> -		+ sizeof (struct multiboot_tag)
> +	size_t mbi_size = (2 * sizeof (uint32_t) /* u32 total_size, u32 reserved */

nit: as this is multi-line, please declare  mbi_size and then use it.

	size_t mbi_size;

	mbi_size = ...

>  		+ ALIGN_UP (sizeof (struct multiboot_tag_basic_meminfo), MULTIBOOT_TAG_ALIGN)
>  		+ ALIGN_UP ((sizeof (struct multiboot_tag_mmap)
>  			+ ranges * sizeof (struct multiboot_mmap_entry)), MULTIBOOT_TAG_ALIGN)
> -		+ ALIGN_UP (sizeof (struct multiboot_tag_load_base_addr), MULTIBOOT_TAG_ALIGN)
>  		+ (sizeof (struct multiboot_tag_string)
>  			+ ALIGN_UP (cmdline_size, MULTIBOOT_TAG_ALIGN))
>  		+ (sizeof (struct multiboot_tag_string)
>  			+ ALIGN_UP (strlen(BOOTLOADER " " BOOTLOADER_VERSION) + 1, MULTIBOOT_TAG_ALIGN))
> -		+ (modcount * sizeof (struct multiboot_tag_module) + modcmd_size));
> +		+ (modcount * sizeof (struct multiboot_tag_module) + modcmd_size))
> +		+ sizeof (struct multiboot_tag); /* end tag */
> +
> +	if (mhi.rel_tag) mbi_size +=
> +		  ALIGN_UP (sizeof (struct multiboot_tag_load_base_addr), MULTIBOOT_TAG_ALIGN);

nit: please use the following coding style:

	if (mhi.rel_tag)
		mbi_size += ALIGN_UP (sizeof (struct multiboot_tag_load_base_addr), MULTIBOOT_TAG_ALIGN);

> +
> +	if (mhi.fb_tag) mbi_size +=
> +		  ALIGN_UP (sizeof (struct multiboot_tag_framebuffer), MULTIBOOT_TAG_ALIGN);
> +
> +	return mbi_size;
>  }
>  
>  static void multiboot2_read_header_tags(void)

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