[PATCH 0/2] arm64: use Image header fields in EFI stub

[PATCH 0/2] arm64: use Image header fields in EFI stub

[Ard Biesheuvel]

This is a followup on the RFC series I sent a week ago that changes the EFI
stub Image loader to stop using linker arithmetic and build time defines and
use data obtained at runtime instead.

This series is now rebased on top of Catalin's arm64 for-next/core branch, which
contains a relevant set of patches by Mark. This means I could drop my former
patch #1 against Documentation/booting.txt.

Patch #1 adds <asm/image_hdr.h>. I incorporated Geoff's feedback to improve the
comments and make the header suitable for sharing with userland.

Patch #2 contains the changes to the actual stub loader itself. This patch
depends on the stub loader bug fix patch I sent out today. It drops all
references to linker symbols and uses text_offset and image_size from the Image
header, and uses the loaded Image size as reported by EFI. This patch also fixes
the corner case where Image happens to be loaded at exactly the right offset,
but the allocation is actually too small to satisfy the requirement imposed by
image_size as set in the header.

Ard Biesheuvel (2):
  arm64: add C struct definition for Image header
  arm64/efi: efistub: get text offset and image size from the Image
    header

 arch/arm64/include/asm/image_hdr.h | 75 ++++++++++++++++++++++++++++++++++++++
 arch/arm64/kernel/Makefile         |  2 -
 arch/arm64/kernel/efi-stub.c       | 29 ++++++++-------
 3 files changed, 91 insertions(+), 15 deletions(-)
 create mode 100644 arch/arm64/include/asm/image_hdr.h

-- 
1.8.3.2

[PATCH 1/2] arm64: add C struct definition for Image header

[Ard Biesheuvel]

In order to be able to interpret the Image header from C code, we need a
struct definition that reflects the specification for Image headers as laid
out in Documentation/arm64/booting.txt.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/include/asm/image_hdr.h | 75 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100644 arch/arm64/include/asm/image_hdr.h

diff --git a/arch/arm64/include/asm/image_hdr.h b/arch/arm64/include/asm/image_hdr.h
new file mode 100644
index 000000000000..9dc74b672124
--- /dev/null
+++ b/arch/arm64/include/asm/image_hdr.h
@@ -0,0 +1,75 @@
+/*
+ * image_hdr.h - C struct definition of the arm64 Image header format
+ *
+ * Copyright (C) 2014 Linaro Ltd
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __ASM_IMAGE_HDR_H
+#define __ASM_IMAGE_HDR_H
+
+#ifdef __KERNEL__
+#include <linux/types.h>
+#else
+#include <stdint.h>
+#endif
+
+/**
+ * struct arm64_image_hdr - arm64 kernel Image binary header format
+ * @code0:		entry point, first instruction to jump to when booting
+ *			the Image
+ * @code1:		second instruction of entry point
+ * @text_offset:	mandatory offset (little endian) beyond a 2 megabyte
+ * 			aligned boundary that needs to be taken into account
+ * 			when deciding where to load the image
+ * @image_size:		total size (little endian) of the memory area (counted
+ *			from the offset where the image was loaded) that may be
+ *			used by the booting kernel before memory reservations
+ *			can be honoured
+ * @flags:		little endian bit field
+ *			Bit 0:		Kernel endianness.  1 if BE, 0 if LE.
+ *			Bits 1-63:	Reserved.
+ * @res2:		reserved, must be 0
+ * @res3:		reserved, must be 0
+ * @res4:		reserved, must be 0
+ * @magic:		Magic number (little endian): "ARM\x64"
+ * @res5:		reserved (used for PE COFF offset)
+ *
+ * This definition reflects the definition in Documentation/arm64/booting.txt in
+ * the Linux source tree.
+ */
+struct arm64_image_hdr {
+	uint32_t code0;
+	uint32_t code1;
+	uint64_t text_offset;
+	uint64_t image_size;
+	uint64_t flags;
+	uint64_t res2;
+	uint64_t res3;
+	uint64_t res4;
+	uint32_t magic;
+	uint32_t res5;
+};
+
+static const union {
+	uint8_t		le_val[4];
+	uint32_t	cpu_val;
+} arm64_image_hdr_magic = {
+	.le_val		= "ARM\x64"
+};
+
+/**
+ * int arm64_image_hdr_check() - check whether hdr points to an arm64 Image
+ * @hdr:	pointer to an arm64 Image
+ *
+ * Return: 1 if check is successful, 0 otherwise
+ */
+static inline int arm64_image_hdr_check(struct arm64_image_hdr const *hdr)
+{
+	return hdr->magic == arm64_image_hdr_magic.cpu_val;
+}
+
+#endif
-- 
1.8.3.2

[PATCH 2/2] arm64/efi: efistub: get text offset and image size from the Image header

[Ard Biesheuvel]

The EFI stub for arm64 needs to behave like an ordinary bootloader in the sense
that it needs to use the EFI environment and the Image header at runtime and
not rely on the linker or preprocessor to produce values for text offset,
image size and kernel size. This patch also fixes the corner case where Image
happens to be loaded at exactly the right offset, but the allocation is
actually too small to satisfy the requirement imposed by image_size as set in
the header.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/Makefile   |  2 --
 arch/arm64/kernel/efi-stub.c | 29 ++++++++++++++++-------------
 2 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index cdaedad3afe5..99b676eeeb0f 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -4,8 +4,6 @@
 
 CPPFLAGS_vmlinux.lds	:= -DTEXT_OFFSET=$(TEXT_OFFSET)
 AFLAGS_head.o		:= -DTEXT_OFFSET=$(TEXT_OFFSET)
-CFLAGS_efi-stub.o 	:= -DTEXT_OFFSET=$(TEXT_OFFSET) \
-			   -I$(src)/../../../scripts/dtc/libfdt
 
 CFLAGS_REMOVE_ftrace.o = -pg
 CFLAGS_REMOVE_insn.o = -pg
diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c
index 9b61d66e2d20..4ba90b2ef677 100644
--- a/arch/arm64/kernel/efi-stub.c
+++ b/arch/arm64/kernel/efi-stub.c
@@ -11,8 +11,7 @@
  */
 #include <linux/efi.h>
 #include <asm/efi.h>
-#include <asm/sections.h>
-
+#include <asm/image_hdr.h>
 
 efi_status_t handle_kernel_image(efi_system_table_t *sys_table,
 				 unsigned long *image_addr,
@@ -23,24 +22,28 @@ efi_status_t handle_kernel_image(efi_system_table_t *sys_table,
 				 efi_loaded_image_t *image)
 {
 	efi_status_t status;
-	unsigned long kernel_size, kernel_memsize = 0;
+	struct arm64_image_hdr *hdr = (struct arm64_image_hdr *)*image_addr;
+
+	/* make sure image_addr points to an arm64 kernel Image */
+	if (!arm64_image_hdr_check(hdr)) {
+		pr_efi_err(sys_table, "Kernel Image header check failed\n");
+		return EFI_LOAD_ERROR;
+	}
 
 	/* Relocate the image, if required. */
-	kernel_size = _edata - _text;
-	if (*image_addr != (dram_base + TEXT_OFFSET)) {
-		kernel_memsize = kernel_size + (_end - _edata) + TEXT_OFFSET;
-		status = efi_low_alloc(sys_table, kernel_memsize, SZ_2M,
+	if (*image_addr != (dram_base + hdr->text_offset) ||
+	    image->image_size < hdr->image_size) {
+		*reserve_size = hdr->text_offset + hdr->image_size;
+		status = efi_low_alloc(sys_table, *reserve_size, SZ_2M,
 				       reserve_addr);
 		if (status != EFI_SUCCESS) {
 			pr_efi_err(sys_table, "Failed to relocate kernel\n");
+			*reserve_size = 0;
 			return status;
 		}
-		memcpy((void *)*reserve_addr + TEXT_OFFSET, (void *)*image_addr,
-		       kernel_size);
-		*image_addr = *reserve_addr + TEXT_OFFSET;
-		*reserve_size = kernel_memsize;
+		memcpy((void *)*reserve_addr + hdr->text_offset,
+		       (void *)*image_addr, image->image_size);
+		*image_addr = *reserve_addr + hdr->text_offset;
 	}
-
-
 	return EFI_SUCCESS;
 }
-- 
1.8.3.2

[PATCH 2/2] arm64/efi: efistub: get text offset and image size from the Image header

[Mark Rutland]

Hi Ard,

On Mon, Jul 14, 2014 at 05:17:51PM +0100, Ard Biesheuvel wrote:
> The EFI stub for arm64 needs to behave like an ordinary bootloader in the sense
> that it needs to use the EFI environment and the Image header at runtime and
> not rely on the linker or preprocessor to produce values for text offset,
> image size and kernel size.

Could you elaborate on _why_ we can't do that, given it's linked into
the kernel Image?

Are we splitting the stub from the kernel? What's going on?

> This patch also fixes the corner case where Image happens to be loaded
> at exactly the right offset, but the allocation is actually too small
> to satisfy the requirement imposed by image_size as set in the header.

It's not really imposed by image_size. While it's described by
image_size it's imposed by the existence of the BSS section and the
initial page tables.

> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/kernel/Makefile   |  2 --
>  arch/arm64/kernel/efi-stub.c | 29 ++++++++++++++++-------------
>  2 files changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index cdaedad3afe5..99b676eeeb0f 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -4,8 +4,6 @@
>  
>  CPPFLAGS_vmlinux.lds	:= -DTEXT_OFFSET=$(TEXT_OFFSET)
>  AFLAGS_head.o		:= -DTEXT_OFFSET=$(TEXT_OFFSET)
> -CFLAGS_efi-stub.o 	:= -DTEXT_OFFSET=$(TEXT_OFFSET) \
> -			   -I$(src)/../../../scripts/dtc/libfdt
>  
>  CFLAGS_REMOVE_ftrace.o = -pg
>  CFLAGS_REMOVE_insn.o = -pg
> diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c
> index 9b61d66e2d20..4ba90b2ef677 100644
> --- a/arch/arm64/kernel/efi-stub.c
> +++ b/arch/arm64/kernel/efi-stub.c
> @@ -11,8 +11,7 @@
>   */
>  #include <linux/efi.h>
>  #include <asm/efi.h>
> -#include <asm/sections.h>
> -
> +#include <asm/image_hdr.h>
>  
>  efi_status_t handle_kernel_image(efi_system_table_t *sys_table,
>  				 unsigned long *image_addr,
> @@ -23,24 +22,28 @@ efi_status_t handle_kernel_image(efi_system_table_t *sys_table,
>  				 efi_loaded_image_t *image)
>  {
>  	efi_status_t status;
> -	unsigned long kernel_size, kernel_memsize = 0;
> +	struct arm64_image_hdr *hdr = (struct arm64_image_hdr *)*image_addr;
> +
> +	/* make sure image_addr points to an arm64 kernel Image */
> +	if (!arm64_image_hdr_check(hdr)) {
> +		pr_efi_err(sys_table, "Kernel Image header check failed\n");
> +		return EFI_LOAD_ERROR;
> +	}

Surely this should always be the case if the stub is linked into the
kernel?

It would be nice to know the rationale for this.

>  
>  	/* Relocate the image, if required. */
> -	kernel_size = _edata - _text;
> -	if (*image_addr != (dram_base + TEXT_OFFSET)) {
> -		kernel_memsize = kernel_size + (_end - _edata) + TEXT_OFFSET;
> -		status = efi_low_alloc(sys_table, kernel_memsize, SZ_2M,
> +	if (*image_addr != (dram_base + hdr->text_offset) ||
> +	    image->image_size < hdr->image_size) {

As far as I can tell the size of the Image (image->image_size) is always
going to be less than the effective run time image size
(hdr->image_size).

The SizeOfImage field in head.S which I assume image->image_size is
derived from (not having a UEFI spec in front of me) seems to cover
everything up to _edata but skips the BSS, and initial page tables, but
this is covered by the header's image_size field.

Am I missing something? Surely we _always_ expect image->image_size to
be smaller than hdr->image_size?

Cheers,
Mark.

> +		*reserve_size = hdr->text_offset + hdr->image_size;
> +		status = efi_low_alloc(sys_table, *reserve_size, SZ_2M,
>  				       reserve_addr);
>  		if (status != EFI_SUCCESS) {
>  			pr_efi_err(sys_table, "Failed to relocate kernel\n");
> +			*reserve_size = 0;
>  			return status;
>  		}
> -		memcpy((void *)*reserve_addr + TEXT_OFFSET, (void *)*image_addr,
> -		       kernel_size);
> -		*image_addr = *reserve_addr + TEXT_OFFSET;
> -		*reserve_size = kernel_memsize;
> +		memcpy((void *)*reserve_addr + hdr->text_offset,
> +		       (void *)*image_addr, image->image_size);
> +		*image_addr = *reserve_addr + hdr->text_offset;
>  	}
> -
> -
>  	return EFI_SUCCESS;
>  }
> -- 
> 1.8.3.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 1/2] arm64: add C struct definition for Image header

[Mark Rutland]

Hi Ard,

On Mon, Jul 14, 2014 at 05:17:50PM +0100, Ard Biesheuvel wrote:
> In order to be able to interpret the Image header from C code, we need a
> struct definition that reflects the specification for Image headers as laid
> out in Documentation/arm64/booting.txt.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/include/asm/image_hdr.h | 75 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
>  create mode 100644 arch/arm64/include/asm/image_hdr.h
> 
> diff --git a/arch/arm64/include/asm/image_hdr.h b/arch/arm64/include/asm/image_hdr.h
> new file mode 100644
> index 000000000000..9dc74b672124
> --- /dev/null
> +++ b/arch/arm64/include/asm/image_hdr.h
> @@ -0,0 +1,75 @@
> +/*
> + * image_hdr.h - C struct definition of the arm64 Image header format
> + *
> + * Copyright (C) 2014 Linaro Ltd
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef __ASM_IMAGE_HDR_H
> +#define __ASM_IMAGE_HDR_H
> +
> +#ifdef __KERNEL__
> +#include <linux/types.h>
> +#else
> +#include <stdint.h>
> +#endif
> +
> +/**
> + * struct arm64_image_hdr - arm64 kernel Image binary header format
> + * @code0:		entry point, first instruction to jump to when booting
> + *			the Image
> + * @code1:		second instruction of entry point
> + * @text_offset:	mandatory offset (little endian) beyond a 2 megabyte
> + * 			aligned boundary that needs to be taken into account
> + * 			when deciding where to load the image
> + * @image_size:		total size (little endian) of the memory area (counted
> + *			from the offset where the image was loaded) that may be
> + *			used by the booting kernel before memory reservations
> + *			can be honoured
> + * @flags:		little endian bit field
> + *			Bit 0:		Kernel endianness.  1 if BE, 0 if LE.
> + *			Bits 1-63:	Reserved.
> + * @res2:		reserved, must be 0
> + * @res3:		reserved, must be 0
> + * @res4:		reserved, must be 0
> + * @magic:		Magic number (little endian): "ARM\x64"
> + * @res5:		reserved (used for PE COFF offset)
> + *
> + * This definition reflects the definition in Documentation/arm64/booting.txt in
> + * the Linux source tree.
> + */

Can we not just say "See Documentation/arm64/booting.txt" rather than
duplicating the description?

> +struct arm64_image_hdr {
> +	uint32_t code0;
> +	uint32_t code1;
> +	uint64_t text_offset;
> +	uint64_t image_size;
> +	uint64_t flags;
> +	uint64_t res2;
> +	uint64_t res3;
> +	uint64_t res4;
> +	uint32_t magic;
> +	uint32_t res5;
> +};
> +
> +static const union {
> +	uint8_t		le_val[4];
> +	uint32_t	cpu_val;
> +} arm64_image_hdr_magic = {
> +	.le_val		= "ARM\x64"
> +};
> +
> +/**
> + * int arm64_image_hdr_check() - check whether hdr points to an arm64 Image
> + * @hdr:	pointer to an arm64 Image
> + *
> + * Return: 1 if check is successful, 0 otherwise
> + */
> +static inline int arm64_image_hdr_check(struct arm64_image_hdr const *hdr)
> +{
> +	return hdr->magic == arm64_image_hdr_magic.cpu_val;
> +}

Rather than the arm64_image_hdr_magic union trick above, could we not
just use the magic inline with a memcmp, e.g.

static inline int arm64_image_hdr_check(struct arm64_image_hdr const *hdr)
{
	return !memcmp(&hdr->magic, "ARM\x64", 4);
}

I'm a little hesitant to expose this to userspace in case the size of
the structure grows and userspace starts relying on it having a
particular length (though perhaps that's unfounded).

It's also a little unfortunate that we lose the nice endianness
annotations here, as it gives a wrong impression of the structure as a
set of native-endian fields.

Thanks,
Mark.

[PATCH 1/2] arm64: add C struct definition for Image header

[Ard Biesheuvel]

On 14 July 2014 18:58, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi Ard,
>
> On Mon, Jul 14, 2014 at 05:17:50PM +0100, Ard Biesheuvel wrote:
>> In order to be able to interpret the Image header from C code, we need a
>> struct definition that reflects the specification for Image headers as laid
>> out in Documentation/arm64/booting.txt.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  arch/arm64/include/asm/image_hdr.h | 75 ++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 75 insertions(+)
>>  create mode 100644 arch/arm64/include/asm/image_hdr.h
>>
>> diff --git a/arch/arm64/include/asm/image_hdr.h b/arch/arm64/include/asm/image_hdr.h
>> new file mode 100644
>> index 000000000000..9dc74b672124
>> --- /dev/null
>> +++ b/arch/arm64/include/asm/image_hdr.h
>> @@ -0,0 +1,75 @@
>> +/*
>> + * image_hdr.h - C struct definition of the arm64 Image header format
>> + *
>> + * Copyright (C) 2014 Linaro Ltd
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#ifndef __ASM_IMAGE_HDR_H
>> +#define __ASM_IMAGE_HDR_H
>> +
>> +#ifdef __KERNEL__
>> +#include <linux/types.h>
>> +#else
>> +#include <stdint.h>
>> +#endif
>> +
>> +/**
>> + * struct arm64_image_hdr - arm64 kernel Image binary header format
>> + * @code0:           entry point, first instruction to jump to when booting
>> + *                   the Image
>> + * @code1:           second instruction of entry point
>> + * @text_offset:     mandatory offset (little endian) beyond a 2 megabyte
>> + *                   aligned boundary that needs to be taken into account
>> + *                   when deciding where to load the image
>> + * @image_size:              total size (little endian) of the memory area (counted
>> + *                   from the offset where the image was loaded) that may be
>> + *                   used by the booting kernel before memory reservations
>> + *                   can be honoured
>> + * @flags:           little endian bit field
>> + *                   Bit 0:          Kernel endianness.  1 if BE, 0 if LE.
>> + *                   Bits 1-63:      Reserved.
>> + * @res2:            reserved, must be 0
>> + * @res3:            reserved, must be 0
>> + * @res4:            reserved, must be 0
>> + * @magic:           Magic number (little endian): "ARM\x64"
>> + * @res5:            reserved (used for PE COFF offset)
>> + *
>> + * This definition reflects the definition in Documentation/arm64/booting.txt in
>> + * the Linux source tree.
>> + */
>
> Can we not just say "See Documentation/arm64/booting.txt" rather than
> duplicating the description?
>

This is at the request of Geoff: he suggested it so we can use
generated documentation.
Seemed sensible to me ...

>> +struct arm64_image_hdr {
>> +     uint32_t code0;
>> +     uint32_t code1;
>> +     uint64_t text_offset;
>> +     uint64_t image_size;
>> +     uint64_t flags;
>> +     uint64_t res2;
>> +     uint64_t res3;
>> +     uint64_t res4;
>> +     uint32_t magic;
>> +     uint32_t res5;
>> +};
>> +
>> +static const union {
>> +     uint8_t         le_val[4];
>> +     uint32_t        cpu_val;
>> +} arm64_image_hdr_magic = {
>> +     .le_val         = "ARM\x64"
>> +};
>> +
>> +/**
>> + * int arm64_image_hdr_check() - check whether hdr points to an arm64 Image
>> + * @hdr:     pointer to an arm64 Image
>> + *
>> + * Return: 1 if check is successful, 0 otherwise
>> + */
>> +static inline int arm64_image_hdr_check(struct arm64_image_hdr const *hdr)
>> +{
>> +     return hdr->magic == arm64_image_hdr_magic.cpu_val;
>> +}
>
> Rather than the arm64_image_hdr_magic union trick above, could we not
> just use the magic inline with a memcmp, e.g.
>
> static inline int arm64_image_hdr_check(struct arm64_image_hdr const *hdr)
> {
>         return !memcmp(&hdr->magic, "ARM\x64", 4);
> }
>

Sure, but I don't think it is necessarily better. Strictly, memcmp()
depends on <string.h> being included, whereas this is just plain C.

> I'm a little hesitant to expose this to userspace in case the size of
> the structure grows and userspace starts relying on it having a
> particular length (though perhaps that's unfounded).
>

Well, the struct does not cover what comes right after it, so in that
sense is irrelevant.
Perhaps you would like to version the header just in case? (Not such a
bad idea anyway)

> It's also a little unfortunate that we lose the nice endianness
> annotations here, as it gives a wrong impression of the structure as a
> set of native-endian fields.
>

Yes, that is indeed unfortunate. I did entertain the idea of using
__le[32|64] in the struct, and typedef'ing them to uint[32|64]_t in
the !__KERNEL__ case.
If we then use memcmp() instead of the union to check the magic, we
can do so without triggering sparse endianness warnings.

-- 
Ard.

[PATCH 2/2] arm64/efi: efistub: get text offset and image size from the Image header

[Ard Biesheuvel]

On 14 July 2014 18:54, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi Ard,
>
> On Mon, Jul 14, 2014 at 05:17:51PM +0100, Ard Biesheuvel wrote:
>> The EFI stub for arm64 needs to behave like an ordinary bootloader in the sense
>> that it needs to use the EFI environment and the Image header at runtime and
>> not rely on the linker or preprocessor to produce values for text offset,
>> image size and kernel size.
>
> Could you elaborate on _why_ we can't do that, given it's linked into
> the kernel Image?
>
> Are we splitting the stub from the kernel? What's going on?
>

Perhaps Leif can chime in here? I was under the impression that you
were aligned in this regard.

>> This patch also fixes the corner case where Image happens to be loaded
>> at exactly the right offset, but the allocation is actually too small
>> to satisfy the requirement imposed by image_size as set in the header.
>
> It's not really imposed by image_size. While it's described by
> image_size it's imposed by the existence of the BSS section and the
> initial page tables.
>

Yes, that is true. But from stub POV, it doesn't matter /why/
image_size has a particular value.

>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  arch/arm64/kernel/Makefile   |  2 --
>>  arch/arm64/kernel/efi-stub.c | 29 ++++++++++++++++-------------
>>  2 files changed, 16 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
>> index cdaedad3afe5..99b676eeeb0f 100644
>> --- a/arch/arm64/kernel/Makefile
>> +++ b/arch/arm64/kernel/Makefile
>> @@ -4,8 +4,6 @@
>>
>>  CPPFLAGS_vmlinux.lds := -DTEXT_OFFSET=$(TEXT_OFFSET)
>>  AFLAGS_head.o                := -DTEXT_OFFSET=$(TEXT_OFFSET)
>> -CFLAGS_efi-stub.o    := -DTEXT_OFFSET=$(TEXT_OFFSET) \
>> -                        -I$(src)/../../../scripts/dtc/libfdt
>>
>>  CFLAGS_REMOVE_ftrace.o = -pg
>>  CFLAGS_REMOVE_insn.o = -pg
>> diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c
>> index 9b61d66e2d20..4ba90b2ef677 100644
>> --- a/arch/arm64/kernel/efi-stub.c
>> +++ b/arch/arm64/kernel/efi-stub.c
>> @@ -11,8 +11,7 @@
>>   */
>>  #include <linux/efi.h>
>>  #include <asm/efi.h>
>> -#include <asm/sections.h>
>> -
>> +#include <asm/image_hdr.h>
>>
>>  efi_status_t handle_kernel_image(efi_system_table_t *sys_table,
>>                                unsigned long *image_addr,
>> @@ -23,24 +22,28 @@ efi_status_t handle_kernel_image(efi_system_table_t *sys_table,
>>                                efi_loaded_image_t *image)
>>  {
>>       efi_status_t status;
>> -     unsigned long kernel_size, kernel_memsize = 0;
>> +     struct arm64_image_hdr *hdr = (struct arm64_image_hdr *)*image_addr;
>> +
>> +     /* make sure image_addr points to an arm64 kernel Image */
>> +     if (!arm64_image_hdr_check(hdr)) {
>> +             pr_efi_err(sys_table, "Kernel Image header check failed\n");
>> +             return EFI_LOAD_ERROR;
>> +     }
>
> Surely this should always be the case if the stub is linked into the
> kernel?
>
> It would be nice to know the rationale for this.
>

Well, there is an actual issue addressed by this: the PE/COFF .text
section covers everything from stext to _edata, so it does not cover
the header itself. In fact, PE/COFF does not allow the headers
themselves to be covered by a section (or at least, the Tianocore/EDK2
UEFI implementation does not). So the pointer points *outside* of the
.text section, and we are assuming that whatever is at that [negative]
offset in the file was also copied to the same offset in memory.(For
instance, there is no reason to assume that all implementations of EFI
will copy data that is not part of any section to RAM when booting an
image located in NOR flash)

>>
>>       /* Relocate the image, if required. */
>> -     kernel_size = _edata - _text;
>> -     if (*image_addr != (dram_base + TEXT_OFFSET)) {
>> -             kernel_memsize = kernel_size + (_end - _edata) + TEXT_OFFSET;
>> -             status = efi_low_alloc(sys_table, kernel_memsize, SZ_2M,
>> +     if (*image_addr != (dram_base + hdr->text_offset) ||
>> +         image->image_size < hdr->image_size) {
>
> As far as I can tell the size of the Image (image->image_size) is always
> going to be less than the effective run time image size
> (hdr->image_size).
>

Currently, yes.

> The SizeOfImage field in head.S which I assume image->image_size is
> derived from (not having a UEFI spec in front of me) seems to cover
> everything up to _edata but skips the BSS, and initial page tables, but
> this is covered by the header's image_size field.
>
> Am I missing something? Surely we _always_ expect image->image_size to
> be smaller than hdr->image_size?
>

At some point, we may extend the virtual size of .text all the way to _end.
(Without growing the actual payload, the remainder will be zero
initialized by the loader)


-- 
Ard.


> Cheers,
> Mark.
>
>> +             *reserve_size = hdr->text_offset + hdr->image_size;
>> +             status = efi_low_alloc(sys_table, *reserve_size, SZ_2M,
>>                                      reserve_addr);
>>               if (status != EFI_SUCCESS) {
>>                       pr_efi_err(sys_table, "Failed to relocate kernel\n");
>> +                     *reserve_size = 0;
>>                       return status;
>>               }
>> -             memcpy((void *)*reserve_addr + TEXT_OFFSET, (void *)*image_addr,
>> -                    kernel_size);
>> -             *image_addr = *reserve_addr + TEXT_OFFSET;
>> -             *reserve_size = kernel_memsize;
>> +             memcpy((void *)*reserve_addr + hdr->text_offset,
>> +                    (void *)*image_addr, image->image_size);
>> +             *image_addr = *reserve_addr + hdr->text_offset;
>>       }
>> -
>> -
>>       return EFI_SUCCESS;
>>  }
>> --
>> 1.8.3.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>

[PATCH 2/2] arm64/efi: efistub: get text offset and image size from the Image header

[Mark Rutland]

On Mon, Jul 14, 2014 at 06:35:32PM +0100, Ard Biesheuvel wrote:
> On 14 July 2014 18:54, Mark Rutland <mark.rutland@arm.com> wrote:
> > Hi Ard,
> >
> > On Mon, Jul 14, 2014 at 05:17:51PM +0100, Ard Biesheuvel wrote:
> >> The EFI stub for arm64 needs to behave like an ordinary bootloader in the sense
> >> that it needs to use the EFI environment and the Image header at runtime and
> >> not rely on the linker or preprocessor to produce values for text offset,
> >> image size and kernel size.
> >
> > Could you elaborate on _why_ we can't do that, given it's linked into
> > the kernel Image?
> >
> > Are we splitting the stub from the kernel? What's going on?
> >
> 
> Perhaps Leif can chime in here? I was under the impression that you
> were aligned in this regard.

Perhaps we were, and I can see reasons for doing this (e.g. as a step
towards making it possible to boot a BE kernel using UEFI), but the
commit message doesn't mention any such reason. For the sake of my poor
recollection and that of others, some words in the commit message would
be helpful.

> >> This patch also fixes the corner case where Image happens to be loaded
> >> at exactly the right offset, but the allocation is actually too small
> >> to satisfy the requirement imposed by image_size as set in the header.
> >
> > It's not really imposed by image_size. While it's described by
> > image_size it's imposed by the existence of the BSS section and the
> > initial page tables.
> >
> 
> Yes, that is true. But from stub POV, it doesn't matter /why/
> image_size has a particular value.

Sure, the stub is a piece of software with no particular knowledge of
anything. However, those of us reading the commit message are not, and
for our sake it would be nice to have a description in case we care as
to /why/ image_size has a particular value and why that value happens to
be useful here.

As far as I can see, we needed to allocate memory for the BSS even
before the image_size field was introduced, so this isn't a new
requirement.

All that's needed is a change to the wording of the commit message to
explain that we're trying to allocate space for (runtime-initialised)
data between _edata and _end rather than how we find out how much space
we need for that (reading the image_size field).

> >>
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >>  arch/arm64/kernel/Makefile   |  2 --
> >>  arch/arm64/kernel/efi-stub.c | 29 ++++++++++++++++-------------
> >>  2 files changed, 16 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> >> index cdaedad3afe5..99b676eeeb0f 100644
> >> --- a/arch/arm64/kernel/Makefile
> >> +++ b/arch/arm64/kernel/Makefile
> >> @@ -4,8 +4,6 @@
> >>
> >>  CPPFLAGS_vmlinux.lds := -DTEXT_OFFSET=$(TEXT_OFFSET)
> >>  AFLAGS_head.o                := -DTEXT_OFFSET=$(TEXT_OFFSET)
> >> -CFLAGS_efi-stub.o    := -DTEXT_OFFSET=$(TEXT_OFFSET) \
> >> -                        -I$(src)/../../../scripts/dtc/libfdt
> >>
> >>  CFLAGS_REMOVE_ftrace.o = -pg
> >>  CFLAGS_REMOVE_insn.o = -pg
> >> diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c
> >> index 9b61d66e2d20..4ba90b2ef677 100644
> >> --- a/arch/arm64/kernel/efi-stub.c
> >> +++ b/arch/arm64/kernel/efi-stub.c
> >> @@ -11,8 +11,7 @@
> >>   */
> >>  #include <linux/efi.h>
> >>  #include <asm/efi.h>
> >> -#include <asm/sections.h>
> >> -
> >> +#include <asm/image_hdr.h>
> >>
> >>  efi_status_t handle_kernel_image(efi_system_table_t *sys_table,
> >>                                unsigned long *image_addr,
> >> @@ -23,24 +22,28 @@ efi_status_t handle_kernel_image(efi_system_table_t *sys_table,
> >>                                efi_loaded_image_t *image)
> >>  {
> >>       efi_status_t status;
> >> -     unsigned long kernel_size, kernel_memsize = 0;
> >> +     struct arm64_image_hdr *hdr = (struct arm64_image_hdr *)*image_addr;
> >> +
> >> +     /* make sure image_addr points to an arm64 kernel Image */
> >> +     if (!arm64_image_hdr_check(hdr)) {
> >> +             pr_efi_err(sys_table, "Kernel Image header check failed\n");
> >> +             return EFI_LOAD_ERROR;
> >> +     }
> >
> > Surely this should always be the case if the stub is linked into the
> > kernel?
> >
> > It would be nice to know the rationale for this.
> >
> 
> Well, there is an actual issue addressed by this: the PE/COFF .text
> section covers everything from stext to _edata, so it does not cover
> the header itself. In fact, PE/COFF does not allow the headers
> themselves to be covered by a section (or at least, the Tianocore/EDK2
> UEFI implementation does not). So the pointer points *outside* of the
> .text section, and we are assuming that whatever is at that [negative]
> offset in the file was also copied to the same offset in memory.(For
> instance, there is no reason to assume that all implementations of EFI
> will copy data that is not part of any section to RAM when booting an
> image located in NOR flash)

If we are making assumptions which aren't always valid, then why the
hell are we making them in the first place, and then trying to validate
them? How does reading an address that we have absolutely no guarantee
as to the contents of solve that problem?

This sounds amazingly fragile, and as far as I can tell the header check
doesn't address the issue so much as paper over it with a failure that's
not even guaranteed to be graceful (e.g. if no memory exists at the
negative offset we're accessing we will simply explode).

Is this the first step towards splitting the stub form the kernel
proper such that we can rely on the image header being present?

If that's the case then please put that in the commit message and cover,
because it's not obvious and there's no point wasting time arguing over
whether to put a piece of useful context into the commit message.

> 
> >>
> >>       /* Relocate the image, if required. */
> >> -     kernel_size = _edata - _text;
> >> -     if (*image_addr != (dram_base + TEXT_OFFSET)) {
> >> -             kernel_memsize = kernel_size + (_end - _edata) + TEXT_OFFSET;
> >> -             status = efi_low_alloc(sys_table, kernel_memsize, SZ_2M,
> >> +     if (*image_addr != (dram_base + hdr->text_offset) ||
> >> +         image->image_size < hdr->image_size) {
> >
> > As far as I can tell the size of the Image (image->image_size) is always
> > going to be less than the effective run time image size
> > (hdr->image_size).
> >
> 
> Currently, yes.
> 
> > The SizeOfImage field in head.S which I assume image->image_size is
> > derived from (not having a UEFI spec in front of me) seems to cover
> > everything up to _edata but skips the BSS, and initial page tables, but
> > this is covered by the header's image_size field.
> >
> > Am I missing something? Surely we _always_ expect image->image_size to
> > be smaller than hdr->image_size?
> >
> 
> At some point, we may extend the virtual size of .text all the way to _end.
> (Without growing the actual payload, the remainder will be zero
> initialized by the loader)

Surely either way we know which is going to be the case? Either it's
smaller (as it is now) or we've padded it (which we have not done).

Why not either assume it's smaller or change the virtual size of .text?

Cheers,
Mark.

[PATCH 2/2] arm64/efi: efistub: get text offset and image size from the Image header

[Ard Biesheuvel]

On 14 July 2014 20:29, Mark Rutland <mark.rutland@arm.com> wrote:
> On Mon, Jul 14, 2014 at 06:35:32PM +0100, Ard Biesheuvel wrote:
>> On 14 July 2014 18:54, Mark Rutland <mark.rutland@arm.com> wrote:
>> > Hi Ard,
>> >
>> > On Mon, Jul 14, 2014 at 05:17:51PM +0100, Ard Biesheuvel wrote:
>> >> The EFI stub for arm64 needs to behave like an ordinary bootloader in the sense
>> >> that it needs to use the EFI environment and the Image header at runtime and
>> >> not rely on the linker or preprocessor to produce values for text offset,
>> >> image size and kernel size.
>> >
>> > Could you elaborate on _why_ we can't do that, given it's linked into
>> > the kernel Image?
>> >
>> > Are we splitting the stub from the kernel? What's going on?
>> >
>>
>> Perhaps Leif can chime in here? I was under the impression that you
>> were aligned in this regard.
>
> Perhaps we were, and I can see reasons for doing this (e.g. as a step
> towards making it possible to boot a BE kernel using UEFI), but the
> commit message doesn't mention any such reason. For the sake of my poor
> recollection and that of others, some words in the commit message would
> be helpful.
>

OK

>> >> This patch also fixes the corner case where Image happens to be loaded
>> >> at exactly the right offset, but the allocation is actually too small
>> >> to satisfy the requirement imposed by image_size as set in the header.
>> >
>> > It's not really imposed by image_size. While it's described by
>> > image_size it's imposed by the existence of the BSS section and the
>> > initial page tables.
>> >
>>
>> Yes, that is true. But from stub POV, it doesn't matter /why/
>> image_size has a particular value.
>
> Sure, the stub is a piece of software with no particular knowledge of
> anything. However, those of us reading the commit message are not, and
> for our sake it would be nice to have a description in case we care as
> to /why/ image_size has a particular value and why that value happens to
> be useful here.
>
> As far as I can see, we needed to allocate memory for the BSS even
> before the image_size field was introduced, so this isn't a new
> requirement.
>
> All that's needed is a change to the wording of the commit message to
> explain that we're trying to allocate space for (runtime-initialised)
> data between _edata and _end rather than how we find out how much space
> we need for that (reading the image_size field).
>

OK

>> >>
>> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >> ---
>> >>  arch/arm64/kernel/Makefile   |  2 --
>> >>  arch/arm64/kernel/efi-stub.c | 29 ++++++++++++++++-------------
>> >>  2 files changed, 16 insertions(+), 15 deletions(-)
>> >>
>> >> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
>> >> index cdaedad3afe5..99b676eeeb0f 100644
>> >> --- a/arch/arm64/kernel/Makefile
>> >> +++ b/arch/arm64/kernel/Makefile
>> >> @@ -4,8 +4,6 @@
>> >>
>> >>  CPPFLAGS_vmlinux.lds := -DTEXT_OFFSET=$(TEXT_OFFSET)
>> >>  AFLAGS_head.o                := -DTEXT_OFFSET=$(TEXT_OFFSET)
>> >> -CFLAGS_efi-stub.o    := -DTEXT_OFFSET=$(TEXT_OFFSET) \
>> >> -                        -I$(src)/../../../scripts/dtc/libfdt
>> >>
>> >>  CFLAGS_REMOVE_ftrace.o = -pg
>> >>  CFLAGS_REMOVE_insn.o = -pg
>> >> diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c
>> >> index 9b61d66e2d20..4ba90b2ef677 100644
>> >> --- a/arch/arm64/kernel/efi-stub.c
>> >> +++ b/arch/arm64/kernel/efi-stub.c
>> >> @@ -11,8 +11,7 @@
>> >>   */
>> >>  #include <linux/efi.h>
>> >>  #include <asm/efi.h>
>> >> -#include <asm/sections.h>
>> >> -
>> >> +#include <asm/image_hdr.h>
>> >>
>> >>  efi_status_t handle_kernel_image(efi_system_table_t *sys_table,
>> >>                                unsigned long *image_addr,
>> >> @@ -23,24 +22,28 @@ efi_status_t handle_kernel_image(efi_system_table_t *sys_table,
>> >>                                efi_loaded_image_t *image)
>> >>  {
>> >>       efi_status_t status;
>> >> -     unsigned long kernel_size, kernel_memsize = 0;
>> >> +     struct arm64_image_hdr *hdr = (struct arm64_image_hdr *)*image_addr;
>> >> +
>> >> +     /* make sure image_addr points to an arm64 kernel Image */
>> >> +     if (!arm64_image_hdr_check(hdr)) {
>> >> +             pr_efi_err(sys_table, "Kernel Image header check failed\n");
>> >> +             return EFI_LOAD_ERROR;
>> >> +     }
>> >
>> > Surely this should always be the case if the stub is linked into the
>> > kernel?
>> >
>> > It would be nice to know the rationale for this.
>> >
>>
>> Well, there is an actual issue addressed by this: the PE/COFF .text
>> section covers everything from stext to _edata, so it does not cover
>> the header itself. In fact, PE/COFF does not allow the headers
>> themselves to be covered by a section (or at least, the Tianocore/EDK2
>> UEFI implementation does not). So the pointer points *outside* of the
>> .text section, and we are assuming that whatever is at that [negative]
>> offset in the file was also copied to the same offset in memory.(For
>> instance, there is no reason to assume that all implementations of EFI
>> will copy data that is not part of any section to RAM when booting an
>> image located in NOR flash)
>
> If we are making assumptions which aren't always valid, then why the
> hell are we making them in the first place, and then trying to validate
> them? How does reading an address that we have absolutely no guarantee
> as to the contents of solve that problem?
>

Isn't that the whole point of a magic number?

> This sounds amazingly fragile, and as far as I can tell the header check
> doesn't address the issue so much as paper over it with a failure that's
> not even guaranteed to be graceful (e.g. if no memory exists at the
> negative offset we're accessing we will simply explode).
>

The memory is guaranteed to exist, only the header is not guaranteed
to have been copied into it.

> Is this the first step towards splitting the stub form the kernel
> proper such that we can rely on the image header being present?
>
> If that's the case then please put that in the commit message and cover,
> because it's not obvious and there's no point wasting time arguing over
> whether to put a piece of useful context into the commit message.
>

Well, the more I think of it, the more I am leaning towards dropping
this patch and taking the opposite approach.
Even if we build the stub LE and link it into a BE kernel, we should
be able to poke the right values into the right places at build time
rather than rely on a header of which we can't be even sure that it
exists in the expected place.

>>
>> >>
>> >>       /* Relocate the image, if required. */
>> >> -     kernel_size = _edata - _text;
>> >> -     if (*image_addr != (dram_base + TEXT_OFFSET)) {
>> >> -             kernel_memsize = kernel_size + (_end - _edata) + TEXT_OFFSET;
>> >> -             status = efi_low_alloc(sys_table, kernel_memsize, SZ_2M,
>> >> +     if (*image_addr != (dram_base + hdr->text_offset) ||
>> >> +         image->image_size < hdr->image_size) {
>> >
>> > As far as I can tell the size of the Image (image->image_size) is always
>> > going to be less than the effective run time image size
>> > (hdr->image_size).
>> >
>>
>> Currently, yes.
>>
>> > The SizeOfImage field in head.S which I assume image->image_size is
>> > derived from (not having a UEFI spec in front of me) seems to cover
>> > everything up to _edata but skips the BSS, and initial page tables, but
>> > this is covered by the header's image_size field.
>> >
>> > Am I missing something? Surely we _always_ expect image->image_size to
>> > be smaller than hdr->image_size?
>> >
>>
>> At some point, we may extend the virtual size of .text all the way to _end.
>> (Without growing the actual payload, the remainder will be zero
>> initialized by the loader)
>
> Surely either way we know which is going to be the case? Either it's
> smaller (as it is now) or we've padded it (which we have not done).
>
> Why not either assume it's smaller or change the virtual size of .text?
>

Well, the trouble is that we use more memory by padding the .text
section, leaving less room for the reallocation which we need to do in
99% of the cases. This is somewhat of a waste if EFI loaded the Image
close to base of DRAM.

But after seeing Mark Salter's stub patch for APM Mustang, this needs
major rework anyway, but my position is now that we should not touch
the header *at all* from the stub.

-- 
Ard.

[PATCH 2/2] arm64/efi: efistub: get text offset and image size from the Image header

[Mark Rutland]

Hi Ard,

[...]

> >> >> diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c
> >> >> index 9b61d66e2d20..4ba90b2ef677 100644
> >> >> --- a/arch/arm64/kernel/efi-stub.c
> >> >> +++ b/arch/arm64/kernel/efi-stub.c
> >> >> @@ -11,8 +11,7 @@
> >> >>   */
> >> >>  #include <linux/efi.h>
> >> >>  #include <asm/efi.h>
> >> >> -#include <asm/sections.h>
> >> >> -
> >> >> +#include <asm/image_hdr.h>
> >> >>
> >> >>  efi_status_t handle_kernel_image(efi_system_table_t *sys_table,
> >> >>                                unsigned long *image_addr,
> >> >> @@ -23,24 +22,28 @@ efi_status_t handle_kernel_image(efi_system_table_t *sys_table,
> >> >>                                efi_loaded_image_t *image)
> >> >>  {
> >> >>       efi_status_t status;
> >> >> -     unsigned long kernel_size, kernel_memsize = 0;
> >> >> +     struct arm64_image_hdr *hdr = (struct arm64_image_hdr *)*image_addr;
> >> >> +
> >> >> +     /* make sure image_addr points to an arm64 kernel Image */
> >> >> +     if (!arm64_image_hdr_check(hdr)) {
> >> >> +             pr_efi_err(sys_table, "Kernel Image header check failed\n");
> >> >> +             return EFI_LOAD_ERROR;
> >> >> +     }
> >> >
> >> > Surely this should always be the case if the stub is linked into the
> >> > kernel?
> >> >
> >> > It would be nice to know the rationale for this.
> >> >
> >>
> >> Well, there is an actual issue addressed by this: the PE/COFF .text
> >> section covers everything from stext to _edata, so it does not cover
> >> the header itself. In fact, PE/COFF does not allow the headers
> >> themselves to be covered by a section (or at least, the Tianocore/EDK2
> >> UEFI implementation does not). So the pointer points *outside* of the
> >> .text section, and we are assuming that whatever is at that [negative]
> >> offset in the file was also copied to the same offset in memory.(For
> >> instance, there is no reason to assume that all implementations of EFI
> >> will copy data that is not part of any section to RAM when booting an
> >> image located in NOR flash)
> >
> > If we are making assumptions which aren't always valid, then why the
> > hell are we making them in the first place, and then trying to validate
> > them? How does reading an address that we have absolutely no guarantee
> > as to the contents of solve that problem?
> >
> 
> Isn't that the whole point of a magic number?

Well...

I'd expect boot loaders and tools to use the magic number to identify an
image, knowing that the whole thing has been copied.

Using it in this way seems remarkably fragile, given that we're using it
to test that UEFI did something it's arguably not supposed to.

> > This sounds amazingly fragile, and as far as I can tell the header check
> > doesn't address the issue so much as paper over it with a failure that's
> > not even guaranteed to be graceful (e.g. if no memory exists at the
> > negative offset we're accessing we will simply explode).
> >
> 
> The memory is guaranteed to exist, only the header is not guaranteed
> to have been copied into it.

Ok. 

> > Is this the first step towards splitting the stub form the kernel
> > proper such that we can rely on the image header being present?
> >
> > If that's the case then please put that in the commit message and cover,
> > because it's not obvious and there's no point wasting time arguing over
> > whether to put a piece of useful context into the commit message.
> >
> 
> Well, the more I think of it, the more I am leaning towards dropping
> this patch and taking the opposite approach.
> Even if we build the stub LE and link it into a BE kernel, we should
> be able to poke the right values into the right places at build time
> rather than rely on a header of which we can't be even sure that it
> exists in the expected place.

That sounds like a better solution to me. We should try to drop all
reliance on the header if we can't expect it to be present.

> >>
> >> >>
> >> >>       /* Relocate the image, if required. */
> >> >> -     kernel_size = _edata - _text;
> >> >> -     if (*image_addr != (dram_base + TEXT_OFFSET)) {
> >> >> -             kernel_memsize = kernel_size + (_end - _edata) + TEXT_OFFSET;
> >> >> -             status = efi_low_alloc(sys_table, kernel_memsize, SZ_2M,
> >> >> +     if (*image_addr != (dram_base + hdr->text_offset) ||
> >> >> +         image->image_size < hdr->image_size) {
> >> >
> >> > As far as I can tell the size of the Image (image->image_size) is always
> >> > going to be less than the effective run time image size
> >> > (hdr->image_size).
> >> >
> >>
> >> Currently, yes.
> >>
> >> > The SizeOfImage field in head.S which I assume image->image_size is
> >> > derived from (not having a UEFI spec in front of me) seems to cover
> >> > everything up to _edata but skips the BSS, and initial page tables, but
> >> > this is covered by the header's image_size field.
> >> >
> >> > Am I missing something? Surely we _always_ expect image->image_size to
> >> > be smaller than hdr->image_size?
> >> >
> >>
> >> At some point, we may extend the virtual size of .text all the way to _end.
> >> (Without growing the actual payload, the remainder will be zero
> >> initialized by the loader)
> >
> > Surely either way we know which is going to be the case? Either it's
> > smaller (as it is now) or we've padded it (which we have not done).
> >
> > Why not either assume it's smaller or change the virtual size of .text?
> >
> 
> Well, the trouble is that we use more memory by padding the .text
> section, leaving less room for the reallocation which we need to do in
> 99% of the cases. This is somewhat of a waste if EFI loaded the Image
> close to base of DRAM.

But if we don't pad then we know it must be smaller, no?

> But after seeing Mark Salter's stub patch for APM Mustang, this needs
> major rework anyway, but my position is now that we should not touch
> the header *at all* from the stub.

Ok. If we're not going to touch the header that sounds fine to me.

Thanks,
Mark.