[RFC PATCH 1/3] arm64: clarify Image header requirement for EFI booting

[RFC PATCH 1/3] arm64: clarify Image header requirement for EFI booting

[Ard Biesheuvel]

The 'res5' field in the Image header is defined as 'reserved, should be 0',
while it serves a specific purpose when booting via the EFI stub, in which
case it should contain the offset of the PE header. So update the doc to
reflect this.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 Documentation/arm64/booting.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/arm64/booting.txt b/Documentation/arm64/booting.txt
index 37fc4f632176..52a07a8d2cfa 100644
--- a/Documentation/arm64/booting.txt
+++ b/Documentation/arm64/booting.txt
@@ -79,15 +79,15 @@ The decompressed kernel image contains a 64-byte header as follows:
   u64 res3	= 0;		/* reserved */
   u64 res4	= 0;		/* reserved */
   u32 magic	= 0x644d5241;	/* Magic number, little endian, "ARM\x64" */
-  u32 res5 = 0;      		/* reserved */
+  u32 pehdr_offset;		/* PE header offset, only used by EFI */
 
 
 Header notes:
 
 - code0/code1 are responsible for branching to stext.
 - when booting through EFI, code0/code1 are initially skipped.
-  res5 is an offset to the PE header and the PE header has the EFI
-  entry point (efi_stub_entry). When the stub has done its work, it
+  pehdr_offset is an offset to the PE header and the PE header has the
+  EFI entry point (efi_stub_entry). When the stub has done its work, it
   jumps to code0 to resume the normal boot process.
 
 The image must be placed at the specified offset (currently 0x80000)
-- 
1.8.3.2

[RFC PATCH 2/3] 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 | 53 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 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..16d32600fb18
--- /dev/null
+++ b/arch/arm64/include/asm/image_hdr.h
@@ -0,0 +1,53 @@
+/*
+ * image_hdr.h - C struct definition of the Image header format
+ *
+ * Copyright (C) 2014 Linaro Ltd  <ard.biesheuvel@linaro.org>
+ *
+ * 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
+
+#include <linux/bug.h>
+#include <linux/byteorder/generic.h>
+#include <linux/types.h>
+
+/*
+ * As defined in Documentation/arm64/booting.txt
+ */
+#define IMAGE_HDR_SIZE		64
+
+struct image_hdr {
+	u32	code0;		/* Executable code */
+	u32	code1;		/* Executable code */
+	__le64	text_offset;	/* Image load offset */
+	u64	res0;		/* Reserved, must be 0 */
+	u64	res1;		/* Reserved, must be 0 */
+	u64	res2;		/* Reserved, must be 0 */
+	u64	res3;		/* Reserved, must be 0 */
+	u64	res4;		/* Reserved, must be 0 */
+	__le32	magic;		/* Magic number, little endian, "ARM\x64" */
+	__le32	pehdr_offset;	/* PE header offset, only used by EFI */
+};
+
+/*
+ * bool image_hdr_check() - checks the Image header for inconsistencies.
+ */
+static inline bool image_hdr_check(struct image_hdr const *hdr)
+{
+	BUILD_BUG_ON(sizeof(struct image_hdr) != IMAGE_HDR_SIZE);
+
+	if (hdr->res0 | hdr->res1 | hdr->res2 | hdr->res3 | hdr->res4)
+		return false;
+	return hdr->magic == cpu_to_le32(0x644d5241);
+}
+
+static inline u64 image_hdr_text_offset(struct image_hdr const *hdr)
+{
+	return le64_to_cpu(hdr->text_offset);
+}
+
+#endif
-- 
1.8.3.2

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

[Geoff Levand]

Hi Ard,

On Tue, 2014-07-08 at 14:50 +0200, 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.

I think the duplication of the structure definition in booting.txt
and in image_hdr.h will not be maintained, so I recommend we remove
the definition in booting.txt and replace it with something like:

-The decompressed kernel image contains a 64-byte header as follows:
...
+The decompressed kernel image contains a 64-byte header as described in
+arch/arm64/include/asm/image_hdr.h.


> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/include/asm/image_hdr.h | 53 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 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..16d32600fb18
> --- /dev/null
> +++ b/arch/arm64/include/asm/image_hdr.h
> @@ -0,0 +1,53 @@
> +/*
> + * image_hdr.h - C struct definition of the Image header format
> + *
> + * Copyright (C) 2014 Linaro Ltd  <ard.biesheuvel@linaro.org>

Are you really a copyright holder of this code?

> + *
> + * 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
> +
> +#include <linux/bug.h>
> +#include <linux/byteorder/generic.h>
> +#include <linux/types.h>
> +
> +/*
> + * As defined in Documentation/arm64/booting.txt
> + */
> +#define IMAGE_HDR_SIZE		64

I can't see any use for this IMAGE_HDR_SIZE.  We have the
structure def, so use sizeof.


Please add a comment here to document this structure and the
members, not in the structure itself.  Something like:

+/**
+ * struct arm64_image_header - arm64 kernel image header.
+ *
+ * @code0: ...

> +
> +struct image_hdr {

Since this is a global def, and possibly exported to user space,
I think arm64_image_hdr would be more appropriate.

> +	u32	code0;		/* Executable code */
> +	u32	code1;		/* Executable code */
> +	__le64	text_offset;	/* Image load offset */
> +	u64	res0;		/* Reserved, must be 0 */
> +	u64	res1;		/* Reserved, must be 0 */
> +	u64	res2;		/* Reserved, must be 0 */
> +	u64	res3;		/* Reserved, must be 0 */
> +	u64	res4;		/* Reserved, must be 0 */
> +	__le32	magic;		/* Magic number, little endian, "ARM\x64" */
> +	__le32	pehdr_offset;	/* PE header offset, only used by EFI */
> +};

These are kernel types.  If we used standard C types then this header
could also be exported for user space programs, so replace u32 with
uint32_t, etc.  We can use helper routines to like image_hdr_text_offset()
to wrap the little endian members.

To export it we would need to add this to arch/arm64/include/asm/Kbuild:

+header-y += image_hrd.h

> +
> +/*
> + * bool image_hdr_check() - checks the Image header for inconsistencies.
> + */
> +static inline bool image_hdr_check(struct image_hdr const *hdr)

> +{
> +	BUILD_BUG_ON(sizeof(struct image_hdr) != IMAGE_HDR_SIZE);
> +
> +	if (hdr->res0 | hdr->res1 | hdr->res2 | hdr->res3 | hdr->res4)
> +		return false;

I don't think we should check these reserved members, as it will limit
forward compatibility of this routine.  The magic check should be a
sufficient check.  If it is not, then we need a better magic value.
 
> +	return hdr->magic == cpu_to_le32(0x644d5241);
> +}
> +
> +static inline u64 image_hdr_text_offset(struct image_hdr const *hdr)
> +{
> +	return le64_to_cpu(hdr->text_offset);
> +}

-Geoff

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

[Ard Biesheuvel]

On 8 July 2014 21:46, Geoff Levand <geoff@infradead.org> wrote:
> Hi Ard,
>
> On Tue, 2014-07-08 at 14:50 +0200, 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.
>
> I think the duplication of the structure definition in booting.txt
> and in image_hdr.h will not be maintained, so I recommend we remove
> the definition in booting.txt and replace it with something like:
>
> -The decompressed kernel image contains a 64-byte header as follows:
> ...
> +The decompressed kernel image contains a 64-byte header as described in
> +arch/arm64/include/asm/image_hdr.h.
>
>

I see what you mean, but I will let the maintainers decide on that one.

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  arch/arm64/include/asm/image_hdr.h | 53 ++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 53 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..16d32600fb18
>> --- /dev/null
>> +++ b/arch/arm64/include/asm/image_hdr.h
>> @@ -0,0 +1,53 @@
>> +/*
>> + * image_hdr.h - C struct definition of the Image header format
>> + *
>> + * Copyright (C) 2014 Linaro Ltd  <ard.biesheuvel@linaro.org>
>
> Are you really a copyright holder of this code?
>

I am the author of this file. Will is the author of booting.txt. I am
not a lawyer so whether that makes Linaro a copyright holder, I am not
sure ...
But as I understand it, someone needs to claim copyright in order for
others to be bound to the license.

>> + *
>> + * 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
>> +
>> +#include <linux/bug.h>
>> +#include <linux/byteorder/generic.h>
>> +#include <linux/types.h>
>> +
>> +/*
>> + * As defined in Documentation/arm64/booting.txt
>> + */
>> +#define IMAGE_HDR_SIZE               64
>
> I can't see any use for this IMAGE_HDR_SIZE.  We have the
> structure def, so use sizeof.
>

I am using it in the BUILD_BUG_ON() below. Do you think that is overkill?

>
> Please add a comment here to document this structure and the
> members, not in the structure itself.  Something like:
>
> +/**
> + * struct arm64_image_header - arm64 kernel image header.
> + *
> + * @code0: ...
>

Is this coding style documented somewhere? Documentation/CodingStyle
does not seem to cover it ...

>> +
>> +struct image_hdr {
>
> Since this is a global def, and possibly exported to user space,
> I think arm64_image_hdr would be more appropriate.
>

Agreed.

>> +     u32     code0;          /* Executable code */
>> +     u32     code1;          /* Executable code */
>> +     __le64  text_offset;    /* Image load offset */
>> +     u64     res0;           /* Reserved, must be 0 */
>> +     u64     res1;           /* Reserved, must be 0 */
>> +     u64     res2;           /* Reserved, must be 0 */
>> +     u64     res3;           /* Reserved, must be 0 */
>> +     u64     res4;           /* Reserved, must be 0 */
>> +     __le32  magic;          /* Magic number, little endian, "ARM\x64" */
>> +     __le32  pehdr_offset;   /* PE header offset, only used by EFI */
>> +};
>
> These are kernel types.  If we used standard C types then this header
> could also be exported for user space programs, so replace u32 with
> uint32_t, etc.  We can use helper routines to like image_hdr_text_offset()
> to wrap the little endian members.
>

I see how that would be useful.

> To export it we would need to add this to arch/arm64/include/asm/Kbuild:
>
> +header-y += image_hrd.h
>
>> +
>> +/*
>> + * bool image_hdr_check() - checks the Image header for inconsistencies.
>> + */
>> +static inline bool image_hdr_check(struct image_hdr const *hdr)
>
>> +{
>> +     BUILD_BUG_ON(sizeof(struct image_hdr) != IMAGE_HDR_SIZE);
>> +
>> +     if (hdr->res0 | hdr->res1 | hdr->res2 | hdr->res3 | hdr->res4)
>> +             return false;
>
> I don't think we should check these reserved members, as it will limit
> forward compatibility of this routine.  The magic check should be a
> sufficient check.  If it is not, then we need a better magic value.
>

If exporting to user space, I agree.

>> +     return hdr->magic == cpu_to_le32(0x644d5241);
>> +}

Perhaps I should use a define here ...

>> +
>> +static inline u64 image_hdr_text_offset(struct image_hdr const *hdr)
>> +{
>> +     return le64_to_cpu(hdr->text_offset);
>> +}
>

Thanks Geoff.

-- 
Ard.

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

[Geoff Levand]

Hi,

On Tue, 2014-07-08 at 22:59 +0200, Ard Biesheuvel wrote:
> On 8 July 2014 21:46, Geoff Levand <geoff@infradead.org> wrote:
> > On Tue, 2014-07-08 at 14:50 +0200, Ard Biesheuvel wrote:
> >
> > I think the duplication of the structure definition in booting.txt
> > and in image_hdr.h will not be maintained, so I recommend we remove
> > the definition in booting.txt and replace it with something like:
> >
> > -The decompressed kernel image contains a 64-byte header as follows:
> > ...
> > +The decompressed kernel image contains a 64-byte header as described in
> > +arch/arm64/include/asm/image_hdr.h.
> >
> >
> 
> I see what you mean, but I will let the maintainers decide on that one.

Why don't you make a separate patch that does it, then their
decision on that won't effect this patch. 

> >> + *
> >> + * Copyright (C) 2014 Linaro Ltd  <ard.biesheuvel@linaro.org>
> >
> > Are you really a copyright holder of this code?
> >
> 
> I am the author of this file. Will is the author of booting.txt. I am
> not a lawyer so whether that makes Linaro a copyright holder, I am not
> sure ...
> But as I understand it, someone needs to claim copyright in order for
> others to be bound to the license.

Well, authorship and ownership (copyright holder) are different
things.  My guess is that your employment agreement makes everything
you create the property of Linaro, so they are the copyright holder.
You are just an author, so you shouldn't put yourself in the copyright
notice.  Probably just add another line like:

+ Author: <ard.biesheuvel@linaro.org>

> >> +#define IMAGE_HDR_SIZE               64
> >
> > I can't see any use for this IMAGE_HDR_SIZE.  We have the
> > structure def, so use sizeof.
> >
> 
> I am using it in the BUILD_BUG_ON() below. Do you think that is overkill?

Yes.  Even if someone changed something to make the size incorrect,
what really matters is the offset of things.

> > Please add a comment here to document this structure and the
> > members, not in the structure itself.  Something like:
> >
> > +/**
> > + * struct arm64_image_header - arm64 kernel image header.
> > + *
> > + * @code0: ...
> >
> 
> Is this coding style documented somewhere? Documentation/CodingStyle
> does not seem to cover it ...

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/kernel-doc-nano-HOWTO.txt


> >> +     return hdr->magic == cpu_to_le32(0x644d5241);
> >> +}
> 
> Perhaps I should use a define here ...

Or a constant, which would have a type.  Something like this?

+static const uint32_t magic_le = 0x644d5241U;

-Geoff

[RFC PATCH 3/3] arm64/efi: efistub: get TEXT_OFFSET from the Image header at runtime

[Ard Biesheuvel]

The EFI stub for arm64 needs to behave like an ordinary bootloader in the sense
that it needs to inspect the Image header at runtime and not rely on the linker
or preprocessor to produce a value for TEXT_OFFSET.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/Makefile   |  2 --
 arch/arm64/kernel/efi-stub.c | 19 +++++++++++++++----
 2 files changed, 15 insertions(+), 6 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 12456a7d3fa2..01581d332858 100644
--- a/arch/arm64/kernel/efi-stub.c
+++ b/arch/arm64/kernel/efi-stub.c
@@ -11,6 +11,7 @@
  */
 #include <linux/efi.h>
 #include <asm/efi.h>
+#include <asm/image_hdr.h>
 #include <asm/sections.h>
 
 
@@ -24,20 +25,30 @@ efi_status_t handle_kernel_image(efi_system_table_t *sys_table,
 {
 	efi_status_t status;
 	unsigned long kernel_size, kernel_memsize = 0;
+	struct image_hdr *hdr = (struct image_hdr *)*image_addr;
+	unsigned long image_base;
+
+	/* make sure image_addr points to an arm64 kernel Image */
+	if (!image_hdr_check(hdr)) {
+		pr_efi_err(sys_table, "Kernel Image header corrupt\n");
+		return EFI_LOAD_ERROR;
+	}
+
+	/* put the image at the offset specified in the Image header */
+	image_base = dram_base + image_hdr_text_offset(hdr);
 
 	/* Relocate the image, if required. */
 	kernel_size = _edata - _text;
-	if (*image_addr != (dram_base + TEXT_OFFSET)) {
+	if (*image_addr != image_base) {
 		kernel_memsize = kernel_size + (_end - _edata);
 		status = efi_relocate_kernel(sys_table, image_addr,
 					     kernel_size, kernel_memsize,
-					     dram_base + TEXT_OFFSET,
-					     PAGE_SIZE);
+					     image_base, PAGE_SIZE);
 		if (status != EFI_SUCCESS) {
 			pr_efi_err(sys_table, "Failed to relocate kernel\n");
 			return status;
 		}
-		if (*image_addr != (dram_base + TEXT_OFFSET)) {
+		if (*image_addr != image_base) {
 			pr_efi_err(sys_table, "Failed to alloc kernel memory\n");
 			efi_free(sys_table, kernel_memsize, *image_addr);
 			return EFI_LOAD_ERROR;
-- 
1.8.3.2