[PATCH 0/7] GRUB: Supporting Secure Boot of xen.gz

[PATCH 0/7] GRUB: Supporting Secure Boot of xen.gz

[Ross Lagerwall]

This patch series implements support for loading and verifying a signed
xen.gz binary. This would allow the same xen.gz binary to be used for
BIOS boot, UEFI boot, and UEFI boot with Secure Boot verification.
There is an accompanying Xen patch series.

The first two patches update the multiboot2 specification with the
necessary changes.

Patches 3 through 5 implement the spec changes.
Patches 6 and 7 are a couple of small changes to allow the Secure Boot
verification to happen.

Ross

Diff stat for 0001 and 0002 (to be applied to  the "multiboot2" branch)
Ross Lagerwall (2):
  multiboot2: Add load type header and support for the PE binary type
  multiboot2: Allow 64-bit entry tags

 doc/multiboot.texi | 71 ++++++++++++++++++++++++++++++++++------------
 doc/multiboot2.h   | 19 ++++++++++++-
 2 files changed, 71 insertions(+), 19 deletions(-)

Diff stat for 0003 to 0007 (to be applied to the "master" branch)
Ross Lagerwall (5):
  multiboot2: Add support for the load type header tag
  multiboot2: Add PE load support
  multiboot2: Add support for 64-bit entry addresses
  efi: Allow loading multiboot modules without verification
  verifiers: Verify after decompression

 grub-core/Makefile.core.def       |   1 +
 grub-core/kern/efi/sb.c           |   1 +
 grub-core/loader/multiboot_mbi2.c |  66 ++-
 grub-core/loader/multiboot_pe.c   | 694 ++++++++++++++++++++++++++++++
 include/grub/efi/pe32.h           |  64 +++
 include/grub/file.h               |   2 +-
 include/grub/multiboot2.h         |   4 +
 include/multiboot2.h              |  19 +-
 8 files changed, 844 insertions(+), 7 deletions(-)
 create mode 100644 grub-core/loader/multiboot_pe.c

-- 
2.43.0

[PATCH 0/7] GRUB: Supporting Secure Boot of xen.gz

[Ross Lagerwall via Grub-devel]

This patch series implements support for loading and verifying a signed
xen.gz binary. This would allow the same xen.gz binary to be used for
BIOS boot, UEFI boot, and UEFI boot with Secure Boot verification.
There is an accompanying Xen patch series.

The first two patches update the multiboot2 specification with the
necessary changes.

Patches 3 through 5 implement the spec changes.
Patches 6 and 7 are a couple of small changes to allow the Secure Boot
verification to happen.

Ross

Diff stat for 0001 and 0002 (to be applied to  the "multiboot2" branch)
Ross Lagerwall (2):
  multiboot2: Add load type header and support for the PE binary type
  multiboot2: Allow 64-bit entry tags

 doc/multiboot.texi | 71 ++++++++++++++++++++++++++++++++++------------
 doc/multiboot2.h   | 19 ++++++++++++-
 2 files changed, 71 insertions(+), 19 deletions(-)

Diff stat for 0003 to 0007 (to be applied to the "master" branch)
Ross Lagerwall (5):
  multiboot2: Add support for the load type header tag
  multiboot2: Add PE load support
  multiboot2: Add support for 64-bit entry addresses
  efi: Allow loading multiboot modules without verification
  verifiers: Verify after decompression

 grub-core/Makefile.core.def       |   1 +
 grub-core/kern/efi/sb.c           |   1 +
 grub-core/loader/multiboot_mbi2.c |  66 ++-
 grub-core/loader/multiboot_pe.c   | 694 ++++++++++++++++++++++++++++++
 include/grub/efi/pe32.h           |  64 +++
 include/grub/file.h               |   2 +-
 include/grub/multiboot2.h         |   4 +
 include/multiboot2.h              |  19 +-
 8 files changed, 844 insertions(+), 7 deletions(-)
 create mode 100644 grub-core/loader/multiboot_pe.c

-- 
2.43.0


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH 1/7] multiboot2: Add load type header and support for the PE binary type

[Ross Lagerwall]

Currently, multiboot2-compatible bootloaders can load ELF binaries and
a.out binaries. The presence of the address header tag determines
how the bootloader tries to interpret the binary (a.out if the address
tag is present else ELF).

Add a new load type header tag that explicitly states the type of the
binary. Bootloaders should use the binary type specified in the load
type tag. If the load type tag is not present, the bootloader should
fall back to the previous heuristics.

In addition to the existing address and ELF load types, specify a new
optional PE binary load type. This new type is a useful addition since
PE binaries can be signed and verified (i.e. used with Secure Boot).

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 doc/multiboot.texi | 39 ++++++++++++++++++++++++++++++++++-----
 doc/multiboot2.h   | 13 +++++++++++++
 2 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/doc/multiboot.texi b/doc/multiboot.texi
index df8a0d056e76..d12719c744eb 100644
--- a/doc/multiboot.texi
+++ b/doc/multiboot.texi
@@ -511,11 +511,12 @@ assumes that no bss segment is present.
 
 Note: This information does not need to be provided if the kernel image
 is in @sc{elf} format, but it must be provided if the image is in a.out
-format or in some other format. When the address tag is present it must
-be used in order to load the image, regardless of whether an @sc{elf}
-header is also present. Compliant boot loaders must be able to load
-images that are either in @sc{elf} format or contain the address tag
-embedded in the Multiboot2 header.
+format or in some other format. If the load type tag is not specified
+and the address tag is present it must be used in order to load the
+image, regardless of whether an @sc{elf} header is also present.
+Compliant boot loaders must be able to load images that are either in
+@sc{elf} format or contain the address tag embedded in the Multiboot2
+header.
 
 @subsection The entry address tag of Multiboot2 header
 
@@ -732,6 +733,34 @@ and @samp{2} means load image at highest possible address but not
 higher than max_addr.
 @end table
 
+@node Load type tag
+@subsection Load type tag
+
+@example
+@group
+        +-------------------+
+u16     | type = 11         |
+u16     | flags             |
+u32     | size = 12         |
+u32     | load_type         |
+        +-------------------+
+@end group
+@end example
+
+This tag indicates the type of the payload and how the boot loader
+should load it.
+
+The meaning of each field is as follows:
+
+@table @code
+@item load_type
+Recognized load types are @samp{0} for address (i.e. load a.out using
+the address tag), @samp{1} for ELF, and @samp{2} for PE. Compliant
+bootloaders should implement support for a.out and ELF as a minimum.  If
+this tag is not specified, the boot loader should attempt to load the
+payload using the information specified in the address tag if present,
+else it should load the payload as an ELF binary.  @end table
+
 @node Machine state
 @section MIPS machine state
 
diff --git a/doc/multiboot2.h b/doc/multiboot2.h
index b181607075b2..d4cae05706e4 100644
--- a/doc/multiboot2.h
+++ b/doc/multiboot2.h
@@ -75,6 +75,7 @@
 #define MULTIBOOT_HEADER_TAG_ENTRY_ADDRESS_EFI32  8
 #define MULTIBOOT_HEADER_TAG_ENTRY_ADDRESS_EFI64  9
 #define MULTIBOOT_HEADER_TAG_RELOCATABLE  10
+#define MULTIBOOT_HEADER_TAG_LOAD_TYPE  11
 
 #define MULTIBOOT_ARCHITECTURE_I386  0
 #define MULTIBOOT_ARCHITECTURE_MIPS32  4
@@ -179,6 +180,18 @@ struct multiboot_header_tag_relocatable
   multiboot_uint32_t preference;
 };
 
+struct multiboot_header_tag_load_type
+{
+  multiboot_uint16_t type;
+  multiboot_uint16_t flags;
+  multiboot_uint32_t size;
+#define MULTIBOOT_LOAD_TYPE_ADDRESS     0
+#define MULTIBOOT_LOAD_TYPE_ELF         1
+#define MULTIBOOT_LOAD_TYPE_PE          2
+#define MULTIBOOT_LOAD_TYPE_TOTAL       3
+  multiboot_uint32_t load_type;
+};
+
 struct multiboot_color
 {
   multiboot_uint8_t red;
-- 
2.43.0

[PATCH 1/7] multiboot2: Add load type header and support for the PE binary type

[Ross Lagerwall via Grub-devel]

Currently, multiboot2-compatible bootloaders can load ELF binaries and
a.out binaries. The presence of the address header tag determines
how the bootloader tries to interpret the binary (a.out if the address
tag is present else ELF).

Add a new load type header tag that explicitly states the type of the
binary. Bootloaders should use the binary type specified in the load
type tag. If the load type tag is not present, the bootloader should
fall back to the previous heuristics.

In addition to the existing address and ELF load types, specify a new
optional PE binary load type. This new type is a useful addition since
PE binaries can be signed and verified (i.e. used with Secure Boot).

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 doc/multiboot.texi | 39 ++++++++++++++++++++++++++++++++++-----
 doc/multiboot2.h   | 13 +++++++++++++
 2 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/doc/multiboot.texi b/doc/multiboot.texi
index df8a0d056e76..d12719c744eb 100644
--- a/doc/multiboot.texi
+++ b/doc/multiboot.texi
@@ -511,11 +511,12 @@ assumes that no bss segment is present.
 
 Note: This information does not need to be provided if the kernel image
 is in @sc{elf} format, but it must be provided if the image is in a.out
-format or in some other format. When the address tag is present it must
-be used in order to load the image, regardless of whether an @sc{elf}
-header is also present. Compliant boot loaders must be able to load
-images that are either in @sc{elf} format or contain the address tag
-embedded in the Multiboot2 header.
+format or in some other format. If the load type tag is not specified
+and the address tag is present it must be used in order to load the
+image, regardless of whether an @sc{elf} header is also present.
+Compliant boot loaders must be able to load images that are either in
+@sc{elf} format or contain the address tag embedded in the Multiboot2
+header.
 
 @subsection The entry address tag of Multiboot2 header
 
@@ -732,6 +733,34 @@ and @samp{2} means load image at highest possible address but not
 higher than max_addr.
 @end table
 
+@node Load type tag
+@subsection Load type tag
+
+@example
+@group
+        +-------------------+
+u16     | type = 11         |
+u16     | flags             |
+u32     | size = 12         |
+u32     | load_type         |
+        +-------------------+
+@end group
+@end example
+
+This tag indicates the type of the payload and how the boot loader
+should load it.
+
+The meaning of each field is as follows:
+
+@table @code
+@item load_type
+Recognized load types are @samp{0} for address (i.e. load a.out using
+the address tag), @samp{1} for ELF, and @samp{2} for PE. Compliant
+bootloaders should implement support for a.out and ELF as a minimum.  If
+this tag is not specified, the boot loader should attempt to load the
+payload using the information specified in the address tag if present,
+else it should load the payload as an ELF binary.  @end table
+
 @node Machine state
 @section MIPS machine state
 
diff --git a/doc/multiboot2.h b/doc/multiboot2.h
index b181607075b2..d4cae05706e4 100644
--- a/doc/multiboot2.h
+++ b/doc/multiboot2.h
@@ -75,6 +75,7 @@
 #define MULTIBOOT_HEADER_TAG_ENTRY_ADDRESS_EFI32  8
 #define MULTIBOOT_HEADER_TAG_ENTRY_ADDRESS_EFI64  9
 #define MULTIBOOT_HEADER_TAG_RELOCATABLE  10
+#define MULTIBOOT_HEADER_TAG_LOAD_TYPE  11
 
 #define MULTIBOOT_ARCHITECTURE_I386  0
 #define MULTIBOOT_ARCHITECTURE_MIPS32  4
@@ -179,6 +180,18 @@ struct multiboot_header_tag_relocatable
   multiboot_uint32_t preference;
 };
 
+struct multiboot_header_tag_load_type
+{
+  multiboot_uint16_t type;
+  multiboot_uint16_t flags;
+  multiboot_uint32_t size;
+#define MULTIBOOT_LOAD_TYPE_ADDRESS     0
+#define MULTIBOOT_LOAD_TYPE_ELF         1
+#define MULTIBOOT_LOAD_TYPE_PE          2
+#define MULTIBOOT_LOAD_TYPE_TOTAL       3
+  multiboot_uint32_t load_type;
+};
+
 struct multiboot_color
 {
   multiboot_uint8_t red;
-- 
2.43.0


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 1/7] multiboot2: Add load type header and support for the PE binary type

[Jan Beulich]

On 13.03.2024 16:07, Ross Lagerwall wrote:
> In addition to the existing address and ELF load types, specify a new
> optional PE binary load type. This new type is a useful addition since
> PE binaries can be signed and verified (i.e. used with Secure Boot).

And the consideration to have ELF signable (by whatever extension to
the ELF spec) went nowhere?

Jan

Re: [PATCH 1/7] multiboot2: Add load type header and support for the PE binary type

[Jan Beulich via Grub-devel]

On 13.03.2024 16:07, Ross Lagerwall wrote:
> In addition to the existing address and ELF load types, specify a new
> optional PE binary load type. This new type is a useful addition since
> PE binaries can be signed and verified (i.e. used with Secure Boot).

And the consideration to have ELF signable (by whatever extension to
the ELF spec) went nowhere?

Jan

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 1/7] multiboot2: Add load type header and support for the PE binary type

[Damien Zammit via Grub-devel]

[-- Attachment #1.1: Type: text/plain, Size: 1450 bytes --]

Hi, I saw this on the list and have concerns:

-------- Original Message --------
On 14 Mar 2024, 6:24 pm, Jan Beulich via Grub-devel < grub-devel@gnu.org> wrote:
On 13.03.2024 16:07, Ross Lagerwall wrote:
>> In addition to the existing address and ELF load types, specify a new
>> optional PE binary load type. This new type is a useful addition since
>> PE binaries can be signed and verified (i.e. used with Secure Boot).

> And the consideration to have ELF signable (by whatever extension to the ELF spec) went nowhere? Jan

If the purpose of signing binaries is to prevent their execution unless they are signed by their owner, this is MALWARE unless the end user can replace the keys with one of their choosing.
Adding a field to elf to provide this feature is IMHO asking for trouble because the key is stored elsewhere and there is nothing to prevent abuse of this field to deny users their freedom to run code, (ie by not providing them the key or a guaranteed mechanism for providing their own).

On that note, why is it such a useful feature to restrict the freedom to run code in grub? If grub selects malware to execute, the user must have chosen to run it - or grub itself is compromised?

Do you think that locking binaries down is the future for users to ensure their own security or it is acceptable for 3rd parties to hide platform keys to lock all systems down, even by binary?

I'm not convinced.

Damien Zammit
GNU/Hurd hacker

[-- Attachment #1.2: Type: text/html, Size: 1556 bytes --]

[-- Attachment #2: Type: text/plain, Size: 141 bytes --]

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 1/7] multiboot2: Add load type header and support for the PE binary type

[Damien Zammit]

[-- Attachment #1: Type: text/plain, Size: 1450 bytes --]

Hi, I saw this on the list and have concerns:

-------- Original Message --------
On 14 Mar 2024, 6:24 pm, Jan Beulich via Grub-devel < grub-devel@gnu.org> wrote:
On 13.03.2024 16:07, Ross Lagerwall wrote:
>> In addition to the existing address and ELF load types, specify a new
>> optional PE binary load type. This new type is a useful addition since
>> PE binaries can be signed and verified (i.e. used with Secure Boot).

> And the consideration to have ELF signable (by whatever extension to the ELF spec) went nowhere? Jan

If the purpose of signing binaries is to prevent their execution unless they are signed by their owner, this is MALWARE unless the end user can replace the keys with one of their choosing.
Adding a field to elf to provide this feature is IMHO asking for trouble because the key is stored elsewhere and there is nothing to prevent abuse of this field to deny users their freedom to run code, (ie by not providing them the key or a guaranteed mechanism for providing their own).

On that note, why is it such a useful feature to restrict the freedom to run code in grub? If grub selects malware to execute, the user must have chosen to run it - or grub itself is compromised?

Do you think that locking binaries down is the future for users to ensure their own security or it is acceptable for 3rd parties to hide platform keys to lock all systems down, even by binary?

I'm not convinced.

Damien Zammit
GNU/Hurd hacker

[-- Attachment #2: Type: text/html, Size: 1556 bytes --]

Re: [PATCH 1/7] multiboot2: Add load type header and support for the PE binary type

[Vladimir 'phcoder' Serbinenko]

Please take this discussion to a separate thread. In this thread it's
pure offtopic

On Thu, Mar 14, 2024 at 10:25 AM Jan Beulich via Grub-devel
<grub-devel@gnu.org> wrote:
>
> On 13.03.2024 16:07, Ross Lagerwall wrote:
> > In addition to the existing address and ELF load types, specify a new
> > optional PE binary load type. This new type is a useful addition since
> > PE binaries can be signed and verified (i.e. used with Secure Boot).
>
> And the consideration to have ELF signable (by whatever extension to
> the ELF spec) went nowhere?
>
> Jan
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel



-- 
Regards
Vladimir 'phcoder' Serbinenko

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 1/7] multiboot2: Add load type header and support for the PE binary type

[Ross Lagerwall]

On Thu, Mar 14, 2024 at 7:24 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 13.03.2024 16:07, Ross Lagerwall wrote:
> > In addition to the existing address and ELF load types, specify a new
> > optional PE binary load type. This new type is a useful addition since
> > PE binaries can be signed and verified (i.e. used with Secure Boot).
>
> And the consideration to have ELF signable (by whatever extension to
> the ELF spec) went nowhere?
>

I'm not sure if you're referring to some ongoing work to create signable
ELFs that I'm not aware of.

I didn't choose that route because:

* Signed PE binaries are the current standard for Secure Boot.

* Having signed ELF binaries would mean that code to handle them needs
to be added to Shim which contravenes its goals of being small and
simple to verify.

* I could be wrong on this but to my knowledge, the ELF format is not
being actively updated nor is the standard owned/maintained by a
specific group which makes updating it difficult.

* Tools would need to be updated/developed to add support for signing
ELF binaries and inspecting the signatures.

I am open to suggestions of course but I'm not sure what benefits there
would be to going the ELF route.

Ross

Re: [PATCH 1/7] multiboot2: Add load type header and support for the PE binary type

[Ross Lagerwall via Grub-devel]

On Thu, Mar 14, 2024 at 7:24 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 13.03.2024 16:07, Ross Lagerwall wrote:
> > In addition to the existing address and ELF load types, specify a new
> > optional PE binary load type. This new type is a useful addition since
> > PE binaries can be signed and verified (i.e. used with Secure Boot).
>
> And the consideration to have ELF signable (by whatever extension to
> the ELF spec) went nowhere?
>

I'm not sure if you're referring to some ongoing work to create signable
ELFs that I'm not aware of.

I didn't choose that route because:

* Signed PE binaries are the current standard for Secure Boot.

* Having signed ELF binaries would mean that code to handle them needs
to be added to Shim which contravenes its goals of being small and
simple to verify.

* I could be wrong on this but to my knowledge, the ELF format is not
being actively updated nor is the standard owned/maintained by a
specific group which makes updating it difficult.

* Tools would need to be updated/developed to add support for signing
ELF binaries and inspecting the signatures.

I am open to suggestions of course but I'm not sure what benefits there
would be to going the ELF route.

Ross

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 1/7] multiboot2: Add load type header and support for the PE binary type

[Jan Beulich]

On 14.03.2024 10:30, Ross Lagerwall wrote:
> On Thu, Mar 14, 2024 at 7:24 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 13.03.2024 16:07, Ross Lagerwall wrote:
>>> In addition to the existing address and ELF load types, specify a new
>>> optional PE binary load type. This new type is a useful addition since
>>> PE binaries can be signed and verified (i.e. used with Secure Boot).
>>
>> And the consideration to have ELF signable (by whatever extension to
>> the ELF spec) went nowhere?
>>
> 
> I'm not sure if you're referring to some ongoing work to create signable
> ELFs that I'm not aware of.

Something must have been invented already to make Linux modules signable.

> I didn't choose that route because:
> 
> * Signed PE binaries are the current standard for Secure Boot.
> 
> * Having signed ELF binaries would mean that code to handle them needs
> to be added to Shim which contravenes its goals of being small and
> simple to verify.

Both true, but neither goes entirely without saying, I suppose.

> * I could be wrong on this but to my knowledge, the ELF format is not
> being actively updated nor is the standard owned/maintained by a
> specific group which makes updating it difficult.

And PE/COFF isn't under control of a public entity / group afaik, which
may be viewed as no better, if not worse.

> * Tools would need to be updated/developed to add support for signing
> ELF binaries and inspecting the signatures.

As above, yes indeed.

Jan

Re: [PATCH 1/7] multiboot2: Add load type header and support for the PE binary type

[Jan Beulich via Grub-devel]

On 14.03.2024 10:30, Ross Lagerwall wrote:
> On Thu, Mar 14, 2024 at 7:24 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 13.03.2024 16:07, Ross Lagerwall wrote:
>>> In addition to the existing address and ELF load types, specify a new
>>> optional PE binary load type. This new type is a useful addition since
>>> PE binaries can be signed and verified (i.e. used with Secure Boot).
>>
>> And the consideration to have ELF signable (by whatever extension to
>> the ELF spec) went nowhere?
>>
> 
> I'm not sure if you're referring to some ongoing work to create signable
> ELFs that I'm not aware of.

Something must have been invented already to make Linux modules signable.

> I didn't choose that route because:
> 
> * Signed PE binaries are the current standard for Secure Boot.
> 
> * Having signed ELF binaries would mean that code to handle them needs
> to be added to Shim which contravenes its goals of being small and
> simple to verify.

Both true, but neither goes entirely without saying, I suppose.

> * I could be wrong on this but to my knowledge, the ELF format is not
> being actively updated nor is the standard owned/maintained by a
> specific group which makes updating it difficult.

And PE/COFF isn't under control of a public entity / group afaik, which
may be viewed as no better, if not worse.

> * Tools would need to be updated/developed to add support for signing
> ELF binaries and inspecting the signatures.

As above, yes indeed.

Jan

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 1/7] multiboot2: Add load type header and support for the PE binary type

[Ross Lagerwall]

On Thu, Mar 14, 2024 at 1:37 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 14.03.2024 10:30, Ross Lagerwall wrote:
> > On Thu, Mar 14, 2024 at 7:24 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 13.03.2024 16:07, Ross Lagerwall wrote:
> >>> In addition to the existing address and ELF load types, specify a new
> >>> optional PE binary load type. This new type is a useful addition since
> >>> PE binaries can be signed and verified (i.e. used with Secure Boot).
> >>
> >> And the consideration to have ELF signable (by whatever extension to
> >> the ELF spec) went nowhere?
> >>
> >
> > I'm not sure if you're referring to some ongoing work to create signable
> > ELFs that I'm not aware of.
>
> Something must have been invented already to make Linux modules signable.

Linux module signatures operate outside of the ELF container. In fact,
AFAIK the actual signed content could be anything. The file format is:

* Content (i.e. ELF binary)
* Signature of content in PKCS7 format
* Signature info, including signature length
* Magic marker: "~Module signature appended~\n"

This kind of arrangement does indeed work but it is fragile. Since the
signature is on the entire contents and tools that understand ELF don't
parse the signature, any transformation of the binary (e.g. to
strip out debuginfo) will cause the signature to be lost / invalidated.

Nevertheless, this could still be an option for Xen if this is
deemed to be a preferred solution by others. It would be good to hear
some opinions on this.

>
> > I didn't choose that route because:
> >
> > * Signed PE binaries are the current standard for Secure Boot.
> >
> > * Having signed ELF binaries would mean that code to handle them needs
> > to be added to Shim which contravenes its goals of being small and
> > simple to verify.
>
> Both true, but neither goes entirely without saying, I suppose.
>
> > * I could be wrong on this but to my knowledge, the ELF format is not
> > being actively updated nor is the standard owned/maintained by a
> > specific group which makes updating it difficult.
>
> And PE/COFF isn't under control of a public entity / group afaik, which
> may be viewed as no better, if not worse.

Agreed, I guess my point was that PE/COFF doesn't need to be updated to
support signing because it already supports it.

Thanks,
Ross

Re: [PATCH 1/7] multiboot2: Add load type header and support for the PE binary type

[Ross Lagerwall via Grub-devel]

On Thu, Mar 14, 2024 at 1:37 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 14.03.2024 10:30, Ross Lagerwall wrote:
> > On Thu, Mar 14, 2024 at 7:24 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 13.03.2024 16:07, Ross Lagerwall wrote:
> >>> In addition to the existing address and ELF load types, specify a new
> >>> optional PE binary load type. This new type is a useful addition since
> >>> PE binaries can be signed and verified (i.e. used with Secure Boot).
> >>
> >> And the consideration to have ELF signable (by whatever extension to
> >> the ELF spec) went nowhere?
> >>
> >
> > I'm not sure if you're referring to some ongoing work to create signable
> > ELFs that I'm not aware of.
>
> Something must have been invented already to make Linux modules signable.

Linux module signatures operate outside of the ELF container. In fact,
AFAIK the actual signed content could be anything. The file format is:

* Content (i.e. ELF binary)
* Signature of content in PKCS7 format
* Signature info, including signature length
* Magic marker: "~Module signature appended~\n"

This kind of arrangement does indeed work but it is fragile. Since the
signature is on the entire contents and tools that understand ELF don't
parse the signature, any transformation of the binary (e.g. to
strip out debuginfo) will cause the signature to be lost / invalidated.

Nevertheless, this could still be an option for Xen if this is
deemed to be a preferred solution by others. It would be good to hear
some opinions on this.

>
> > I didn't choose that route because:
> >
> > * Signed PE binaries are the current standard for Secure Boot.
> >
> > * Having signed ELF binaries would mean that code to handle them needs
> > to be added to Shim which contravenes its goals of being small and
> > simple to verify.
>
> Both true, but neither goes entirely without saying, I suppose.
>
> > * I could be wrong on this but to my knowledge, the ELF format is not
> > being actively updated nor is the standard owned/maintained by a
> > specific group which makes updating it difficult.
>
> And PE/COFF isn't under control of a public entity / group afaik, which
> may be viewed as no better, if not worse.

Agreed, I guess my point was that PE/COFF doesn't need to be updated to
support signing because it already supports it.

Thanks,
Ross

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 1/7] multiboot2: Add load type header and support for the PE binary type

[Jan Beulich]

On 14.03.2024 15:24, Ross Lagerwall wrote:
> On Thu, Mar 14, 2024 at 1:37 PM Jan Beulich <jbeulich@suse.com> wrote:
>> On 14.03.2024 10:30, Ross Lagerwall wrote:
>>> On Thu, Mar 14, 2024 at 7:24 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 13.03.2024 16:07, Ross Lagerwall wrote:
>>>>> In addition to the existing address and ELF load types, specify a new
>>>>> optional PE binary load type. This new type is a useful addition since
>>>>> PE binaries can be signed and verified (i.e. used with Secure Boot).
>>>>
>>>> And the consideration to have ELF signable (by whatever extension to
>>>> the ELF spec) went nowhere?
>>>
>>> I'm not sure if you're referring to some ongoing work to create signable
>>> ELFs that I'm not aware of.
>>
>> Something must have been invented already to make Linux modules signable.
> 
> Linux module signatures operate outside of the ELF container. In fact,
> AFAIK the actual signed content could be anything. The file format is:
> 
> * Content (i.e. ELF binary)
> * Signature of content in PKCS7 format
> * Signature info, including signature length
> * Magic marker: "~Module signature appended~\n"
> 
> This kind of arrangement does indeed work but it is fragile. Since the
> signature is on the entire contents and tools that understand ELF don't
> parse the signature, any transformation of the binary (e.g. to
> strip out debuginfo) will cause the signature to be lost / invalidated.

This looks extremely poor to me, so ...

> Nevertheless, this could still be an option for Xen if this is
> deemed to be a preferred solution by others. It would be good to hear
> some opinions on this.

... I'd rather not see this used for Xen.

Jan

Re: [PATCH 1/7] multiboot2: Add load type header and support for the PE binary type

[Jan Beulich via Grub-devel]

On 14.03.2024 15:24, Ross Lagerwall wrote:
> On Thu, Mar 14, 2024 at 1:37 PM Jan Beulich <jbeulich@suse.com> wrote:
>> On 14.03.2024 10:30, Ross Lagerwall wrote:
>>> On Thu, Mar 14, 2024 at 7:24 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 13.03.2024 16:07, Ross Lagerwall wrote:
>>>>> In addition to the existing address and ELF load types, specify a new
>>>>> optional PE binary load type. This new type is a useful addition since
>>>>> PE binaries can be signed and verified (i.e. used with Secure Boot).
>>>>
>>>> And the consideration to have ELF signable (by whatever extension to
>>>> the ELF spec) went nowhere?
>>>
>>> I'm not sure if you're referring to some ongoing work to create signable
>>> ELFs that I'm not aware of.
>>
>> Something must have been invented already to make Linux modules signable.
> 
> Linux module signatures operate outside of the ELF container. In fact,
> AFAIK the actual signed content could be anything. The file format is:
> 
> * Content (i.e. ELF binary)
> * Signature of content in PKCS7 format
> * Signature info, including signature length
> * Magic marker: "~Module signature appended~\n"
> 
> This kind of arrangement does indeed work but it is fragile. Since the
> signature is on the entire contents and tools that understand ELF don't
> parse the signature, any transformation of the binary (e.g. to
> strip out debuginfo) will cause the signature to be lost / invalidated.

This looks extremely poor to me, so ...

> Nevertheless, this could still be an option for Xen if this is
> deemed to be a preferred solution by others. It would be good to hear
> some opinions on this.

... I'd rather not see this used for Xen.

Jan

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 1/7] multiboot2: Add load type header and support for the PE binary type

[Roger Pau Monné]

On Thu, Mar 14, 2024 at 02:24:31PM +0000, Ross Lagerwall wrote:
> On Thu, Mar 14, 2024 at 1:37 PM Jan Beulich <jbeulich@suse.com> wrote:
> >
> > On 14.03.2024 10:30, Ross Lagerwall wrote:
> > > On Thu, Mar 14, 2024 at 7:24 AM Jan Beulich <jbeulich@suse.com> wrote:
> > >>
> > >> On 13.03.2024 16:07, Ross Lagerwall wrote:
> > >>> In addition to the existing address and ELF load types, specify a new
> > >>> optional PE binary load type. This new type is a useful addition since
> > >>> PE binaries can be signed and verified (i.e. used with Secure Boot).
> > >>
> > >> And the consideration to have ELF signable (by whatever extension to
> > >> the ELF spec) went nowhere?
> > >>
> > >
> > > I'm not sure if you're referring to some ongoing work to create signable
> > > ELFs that I'm not aware of.
> >
> > Something must have been invented already to make Linux modules signable.
> 
> Linux module signatures operate outside of the ELF container. In fact,
> AFAIK the actual signed content could be anything. The file format is:
> 
> * Content (i.e. ELF binary)
> * Signature of content in PKCS7 format
> * Signature info, including signature length
> * Magic marker: "~Module signature appended~\n"
> 
> This kind of arrangement does indeed work but it is fragile. Since the
> signature is on the entire contents and tools that understand ELF don't
> parse the signature, any transformation of the binary (e.g. to
> strip out debuginfo) will cause the signature to be lost / invalidated.
> 
> Nevertheless, this could still be an option for Xen if this is
> deemed to be a preferred solution by others. It would be good to hear
> some opinions on this.

No, IMO the PE route is likely the best one, as there's already all
the tooling around it, and it's what other OSes use to perform secure
boot.

It would have been nice for ELF to grow an extension to the spec for
image integrity data, but I don't see myself doing the work TBH.

Thanks, Roger.

Re: [PATCH 1/7] multiboot2: Add load type header and support for the PE binary type

[Roger Pau Monné via Grub-devel]

On Thu, Mar 14, 2024 at 02:24:31PM +0000, Ross Lagerwall wrote:
> On Thu, Mar 14, 2024 at 1:37 PM Jan Beulich <jbeulich@suse.com> wrote:
> >
> > On 14.03.2024 10:30, Ross Lagerwall wrote:
> > > On Thu, Mar 14, 2024 at 7:24 AM Jan Beulich <jbeulich@suse.com> wrote:
> > >>
> > >> On 13.03.2024 16:07, Ross Lagerwall wrote:
> > >>> In addition to the existing address and ELF load types, specify a new
> > >>> optional PE binary load type. This new type is a useful addition since
> > >>> PE binaries can be signed and verified (i.e. used with Secure Boot).
> > >>
> > >> And the consideration to have ELF signable (by whatever extension to
> > >> the ELF spec) went nowhere?
> > >>
> > >
> > > I'm not sure if you're referring to some ongoing work to create signable
> > > ELFs that I'm not aware of.
> >
> > Something must have been invented already to make Linux modules signable.
> 
> Linux module signatures operate outside of the ELF container. In fact,
> AFAIK the actual signed content could be anything. The file format is:
> 
> * Content (i.e. ELF binary)
> * Signature of content in PKCS7 format
> * Signature info, including signature length
> * Magic marker: "~Module signature appended~\n"
> 
> This kind of arrangement does indeed work but it is fragile. Since the
> signature is on the entire contents and tools that understand ELF don't
> parse the signature, any transformation of the binary (e.g. to
> strip out debuginfo) will cause the signature to be lost / invalidated.
> 
> Nevertheless, this could still be an option for Xen if this is
> deemed to be a preferred solution by others. It would be good to hear
> some opinions on this.

No, IMO the PE route is likely the best one, as there's already all
the tooling around it, and it's what other OSes use to perform secure
boot.

It would have been nice for ELF to grow an extension to the spec for
image integrity data, but I don't see myself doing the work TBH.

Thanks, Roger.

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 1/7] multiboot2: Add load type header and support for the PE binary type

[Roger Pau Monné]

On Wed, Mar 13, 2024 at 03:07:42PM +0000, Ross Lagerwall wrote:
> Currently, multiboot2-compatible bootloaders can load ELF binaries and
> a.out binaries. The presence of the address header tag determines
> how the bootloader tries to interpret the binary (a.out if the address
> tag is present else ELF).
> 
> Add a new load type header tag that explicitly states the type of the
> binary. Bootloaders should use the binary type specified in the load
> type tag. If the load type tag is not present, the bootloader should
> fall back to the previous heuristics.
> 
> In addition to the existing address and ELF load types, specify a new
> optional PE binary load type. This new type is a useful addition since
> PE binaries can be signed and verified (i.e. used with Secure Boot).
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
>  doc/multiboot.texi | 39 ++++++++++++++++++++++++++++++++++-----
>  doc/multiboot2.h   | 13 +++++++++++++
>  2 files changed, 47 insertions(+), 5 deletions(-)
> 
> diff --git a/doc/multiboot.texi b/doc/multiboot.texi
> index df8a0d056e76..d12719c744eb 100644
> --- a/doc/multiboot.texi
> +++ b/doc/multiboot.texi
> @@ -511,11 +511,12 @@ assumes that no bss segment is present.
>  
>  Note: This information does not need to be provided if the kernel image
>  is in @sc{elf} format, but it must be provided if the image is in a.out
> -format or in some other format. When the address tag is present it must
> -be used in order to load the image, regardless of whether an @sc{elf}
> -header is also present. Compliant boot loaders must be able to load
> -images that are either in @sc{elf} format or contain the address tag
> -embedded in the Multiboot2 header.
> +format or in some other format. If the load type tag is not specified
> +and the address tag is present it must be used in order to load the
> +image, regardless of whether an @sc{elf} header is also present.
> +Compliant boot loaders must be able to load images that are either in
> +@sc{elf} format or contain the address tag embedded in the Multiboot2
> +header.
>  
>  @subsection The entry address tag of Multiboot2 header
>  
> @@ -732,6 +733,34 @@ and @samp{2} means load image at highest possible address but not
>  higher than max_addr.
>  @end table
>  
> +@node Load type tag
> +@subsection Load type tag
> +
> +@example
> +@group
> +        +-------------------+
> +u16     | type = 11         |
> +u16     | flags             |
> +u32     | size = 12         |
> +u32     | load_type         |
> +        +-------------------+
> +@end group
> +@end example
> +
> +This tag indicates the type of the payload and how the boot loader
> +should load it.
> +
> +The meaning of each field is as follows:
> +
> +@table @code
> +@item load_type
> +Recognized load types are @samp{0} for address (i.e. load a.out using
> +the address tag), @samp{1} for ELF, and @samp{2} for PE. Compliant
> +bootloaders should implement support for a.out and ELF as a minimum.  If
> +this tag is not specified, the boot loader should attempt to load the
> +payload using the information specified in the address tag if present,
> +else it should load the payload as an ELF binary.  @end table

I wonder if it would be simpler to use the following order instead:

1. Address tag
2. Load type tag
3. ELF header

It's pointless to add a Loader type tag with load_type == 0, as that's
already mandated by the Address tag.  IOW: signaling the use of the
Address tag here is kind of pointless, if the fields in the Address
tag are set, that's the only signaling required for the data in the
Address tag to be used.

Or are we attempting to support images that set Address tag and Load
type tag != 0 in order to use the Address tag when the loader doesn't
recognize the Load type tag, and otherwise use a different format?

Would it be sensible for multiboot2 to use PE in preference to ELF if
present on the image?  (without requiring any signaling).  I would
think this could be troublesome if kernels are so far expecting the
ELF header to be used with multiboot2, even if they also expose a PE
header.

Thanks, Roger.

Re: [PATCH 1/7] multiboot2: Add load type header and support for the PE binary type

[Roger Pau Monné via Grub-devel]

On Wed, Mar 13, 2024 at 03:07:42PM +0000, Ross Lagerwall wrote:
> Currently, multiboot2-compatible bootloaders can load ELF binaries and
> a.out binaries. The presence of the address header tag determines
> how the bootloader tries to interpret the binary (a.out if the address
> tag is present else ELF).
> 
> Add a new load type header tag that explicitly states the type of the
> binary. Bootloaders should use the binary type specified in the load
> type tag. If the load type tag is not present, the bootloader should
> fall back to the previous heuristics.
> 
> In addition to the existing address and ELF load types, specify a new
> optional PE binary load type. This new type is a useful addition since
> PE binaries can be signed and verified (i.e. used with Secure Boot).
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
>  doc/multiboot.texi | 39 ++++++++++++++++++++++++++++++++++-----
>  doc/multiboot2.h   | 13 +++++++++++++
>  2 files changed, 47 insertions(+), 5 deletions(-)
> 
> diff --git a/doc/multiboot.texi b/doc/multiboot.texi
> index df8a0d056e76..d12719c744eb 100644
> --- a/doc/multiboot.texi
> +++ b/doc/multiboot.texi
> @@ -511,11 +511,12 @@ assumes that no bss segment is present.
>  
>  Note: This information does not need to be provided if the kernel image
>  is in @sc{elf} format, but it must be provided if the image is in a.out
> -format or in some other format. When the address tag is present it must
> -be used in order to load the image, regardless of whether an @sc{elf}
> -header is also present. Compliant boot loaders must be able to load
> -images that are either in @sc{elf} format or contain the address tag
> -embedded in the Multiboot2 header.
> +format or in some other format. If the load type tag is not specified
> +and the address tag is present it must be used in order to load the
> +image, regardless of whether an @sc{elf} header is also present.
> +Compliant boot loaders must be able to load images that are either in
> +@sc{elf} format or contain the address tag embedded in the Multiboot2
> +header.
>  
>  @subsection The entry address tag of Multiboot2 header
>  
> @@ -732,6 +733,34 @@ and @samp{2} means load image at highest possible address but not
>  higher than max_addr.
>  @end table
>  
> +@node Load type tag
> +@subsection Load type tag
> +
> +@example
> +@group
> +        +-------------------+
> +u16     | type = 11         |
> +u16     | flags             |
> +u32     | size = 12         |
> +u32     | load_type         |
> +        +-------------------+
> +@end group
> +@end example
> +
> +This tag indicates the type of the payload and how the boot loader
> +should load it.
> +
> +The meaning of each field is as follows:
> +
> +@table @code
> +@item load_type
> +Recognized load types are @samp{0} for address (i.e. load a.out using
> +the address tag), @samp{1} for ELF, and @samp{2} for PE. Compliant
> +bootloaders should implement support for a.out and ELF as a minimum.  If
> +this tag is not specified, the boot loader should attempt to load the
> +payload using the information specified in the address tag if present,
> +else it should load the payload as an ELF binary.  @end table

I wonder if it would be simpler to use the following order instead:

1. Address tag
2. Load type tag
3. ELF header

It's pointless to add a Loader type tag with load_type == 0, as that's
already mandated by the Address tag.  IOW: signaling the use of the
Address tag here is kind of pointless, if the fields in the Address
tag are set, that's the only signaling required for the data in the
Address tag to be used.

Or are we attempting to support images that set Address tag and Load
type tag != 0 in order to use the Address tag when the loader doesn't
recognize the Load type tag, and otherwise use a different format?

Would it be sensible for multiboot2 to use PE in preference to ELF if
present on the image?  (without requiring any signaling).  I would
think this could be troublesome if kernels are so far expecting the
ELF header to be used with multiboot2, even if they also expose a PE
header.

Thanks, Roger.

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 1/7] multiboot2: Add load type header and support for the PE binary type

[Ross Lagerwall]

On Tue, Mar 19, 2024 at 1:18 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
>
> On Wed, Mar 13, 2024 at 03:07:42PM +0000, Ross Lagerwall wrote:
> > Currently, multiboot2-compatible bootloaders can load ELF binaries and
> > a.out binaries. The presence of the address header tag determines
> > how the bootloader tries to interpret the binary (a.out if the address
> > tag is present else ELF).
> >
> > Add a new load type header tag that explicitly states the type of the
> > binary. Bootloaders should use the binary type specified in the load
> > type tag. If the load type tag is not present, the bootloader should
> > fall back to the previous heuristics.
> >
> > In addition to the existing address and ELF load types, specify a new
> > optional PE binary load type. This new type is a useful addition since
> > PE binaries can be signed and verified (i.e. used with Secure Boot).
> >
> > Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> > ---
> >  doc/multiboot.texi | 39 ++++++++++++++++++++++++++++++++++-----
> >  doc/multiboot2.h   | 13 +++++++++++++
> >  2 files changed, 47 insertions(+), 5 deletions(-)
> >
> > diff --git a/doc/multiboot.texi b/doc/multiboot.texi
> > index df8a0d056e76..d12719c744eb 100644
> > --- a/doc/multiboot.texi
> > +++ b/doc/multiboot.texi
> > @@ -511,11 +511,12 @@ assumes that no bss segment is present.
> >
> >  Note: This information does not need to be provided if the kernel image
> >  is in @sc{elf} format, but it must be provided if the image is in a.out
> > -format or in some other format. When the address tag is present it must
> > -be used in order to load the image, regardless of whether an @sc{elf}
> > -header is also present. Compliant boot loaders must be able to load
> > -images that are either in @sc{elf} format or contain the address tag
> > -embedded in the Multiboot2 header.
> > +format or in some other format. If the load type tag is not specified
> > +and the address tag is present it must be used in order to load the
> > +image, regardless of whether an @sc{elf} header is also present.
> > +Compliant boot loaders must be able to load images that are either in
> > +@sc{elf} format or contain the address tag embedded in the Multiboot2
> > +header.
> >
> >  @subsection The entry address tag of Multiboot2 header
> >
> > @@ -732,6 +733,34 @@ and @samp{2} means load image at highest possible address but not
> >  higher than max_addr.
> >  @end table
> >
> > +@node Load type tag
> > +@subsection Load type tag
> > +
> > +@example
> > +@group
> > +        +-------------------+
> > +u16     | type = 11         |
> > +u16     | flags             |
> > +u32     | size = 12         |
> > +u32     | load_type         |
> > +        +-------------------+
> > +@end group
> > +@end example
> > +
> > +This tag indicates the type of the payload and how the boot loader
> > +should load it.
> > +
> > +The meaning of each field is as follows:
> > +
> > +@table @code
> > +@item load_type
> > +Recognized load types are @samp{0} for address (i.e. load a.out using
> > +the address tag), @samp{1} for ELF, and @samp{2} for PE. Compliant
> > +bootloaders should implement support for a.out and ELF as a minimum.  If
> > +this tag is not specified, the boot loader should attempt to load the
> > +payload using the information specified in the address tag if present,
> > +else it should load the payload as an ELF binary.  @end table
>
> I wonder if it would be simpler to use the following order instead:
>
> 1. Address tag
> 2. Load type tag
> 3. ELF header
>
> It's pointless to add a Loader type tag with load_type == 0, as that's
> already mandated by the Address tag.  IOW: signaling the use of the
> Address tag here is kind of pointless, if the fields in the Address
> tag are set, that's the only signaling required for the data in the
> Address tag to be used.
>
> Or are we attempting to support images that set Address tag and Load
> type tag != 0 in order to use the Address tag when the loader doesn't
> recognize the Load type tag, and otherwise use a different format?

No, that wasn't my intention. My intention for the load type tag was
to have one tag that unambiguously describes the payload format and if
that is missing, fall back to the previous logic for compatibility.
It avoids more complicated heuristics if different payload types are
added in the future.

>
> Would it be sensible for multiboot2 to use PE in preference to ELF if
> present on the image?  (without requiring any signaling).  I would
> think this could be troublesome if kernels are so far expecting the
> ELF header to be used with multiboot2, even if they also expose a PE
> header.
>

AFAIK an ELF image can't also be a valid PE/COFF image since they have
different magic numbers at the start of the image. Perhaps it would
be simpler to avoid introducing the load type tag and just load the
payload based on the magic number?

Ross

Re: [PATCH 1/7] multiboot2: Add load type header and support for the PE binary type

[Ross Lagerwall via Grub-devel]

On Tue, Mar 19, 2024 at 1:18 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
>
> On Wed, Mar 13, 2024 at 03:07:42PM +0000, Ross Lagerwall wrote:
> > Currently, multiboot2-compatible bootloaders can load ELF binaries and
> > a.out binaries. The presence of the address header tag determines
> > how the bootloader tries to interpret the binary (a.out if the address
> > tag is present else ELF).
> >
> > Add a new load type header tag that explicitly states the type of the
> > binary. Bootloaders should use the binary type specified in the load
> > type tag. If the load type tag is not present, the bootloader should
> > fall back to the previous heuristics.
> >
> > In addition to the existing address and ELF load types, specify a new
> > optional PE binary load type. This new type is a useful addition since
> > PE binaries can be signed and verified (i.e. used with Secure Boot).
> >
> > Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> > ---
> >  doc/multiboot.texi | 39 ++++++++++++++++++++++++++++++++++-----
> >  doc/multiboot2.h   | 13 +++++++++++++
> >  2 files changed, 47 insertions(+), 5 deletions(-)
> >
> > diff --git a/doc/multiboot.texi b/doc/multiboot.texi
> > index df8a0d056e76..d12719c744eb 100644
> > --- a/doc/multiboot.texi
> > +++ b/doc/multiboot.texi
> > @@ -511,11 +511,12 @@ assumes that no bss segment is present.
> >
> >  Note: This information does not need to be provided if the kernel image
> >  is in @sc{elf} format, but it must be provided if the image is in a.out
> > -format or in some other format. When the address tag is present it must
> > -be used in order to load the image, regardless of whether an @sc{elf}
> > -header is also present. Compliant boot loaders must be able to load
> > -images that are either in @sc{elf} format or contain the address tag
> > -embedded in the Multiboot2 header.
> > +format or in some other format. If the load type tag is not specified
> > +and the address tag is present it must be used in order to load the
> > +image, regardless of whether an @sc{elf} header is also present.
> > +Compliant boot loaders must be able to load images that are either in
> > +@sc{elf} format or contain the address tag embedded in the Multiboot2
> > +header.
> >
> >  @subsection The entry address tag of Multiboot2 header
> >
> > @@ -732,6 +733,34 @@ and @samp{2} means load image at highest possible address but not
> >  higher than max_addr.
> >  @end table
> >
> > +@node Load type tag
> > +@subsection Load type tag
> > +
> > +@example
> > +@group
> > +        +-------------------+
> > +u16     | type = 11         |
> > +u16     | flags             |
> > +u32     | size = 12         |
> > +u32     | load_type         |
> > +        +-------------------+
> > +@end group
> > +@end example
> > +
> > +This tag indicates the type of the payload and how the boot loader
> > +should load it.
> > +
> > +The meaning of each field is as follows:
> > +
> > +@table @code
> > +@item load_type
> > +Recognized load types are @samp{0} for address (i.e. load a.out using
> > +the address tag), @samp{1} for ELF, and @samp{2} for PE. Compliant
> > +bootloaders should implement support for a.out and ELF as a minimum.  If
> > +this tag is not specified, the boot loader should attempt to load the
> > +payload using the information specified in the address tag if present,
> > +else it should load the payload as an ELF binary.  @end table
>
> I wonder if it would be simpler to use the following order instead:
>
> 1. Address tag
> 2. Load type tag
> 3. ELF header
>
> It's pointless to add a Loader type tag with load_type == 0, as that's
> already mandated by the Address tag.  IOW: signaling the use of the
> Address tag here is kind of pointless, if the fields in the Address
> tag are set, that's the only signaling required for the data in the
> Address tag to be used.
>
> Or are we attempting to support images that set Address tag and Load
> type tag != 0 in order to use the Address tag when the loader doesn't
> recognize the Load type tag, and otherwise use a different format?

No, that wasn't my intention. My intention for the load type tag was
to have one tag that unambiguously describes the payload format and if
that is missing, fall back to the previous logic for compatibility.
It avoids more complicated heuristics if different payload types are
added in the future.

>
> Would it be sensible for multiboot2 to use PE in preference to ELF if
> present on the image?  (without requiring any signaling).  I would
> think this could be troublesome if kernels are so far expecting the
> ELF header to be used with multiboot2, even if they also expose a PE
> header.
>

AFAIK an ELF image can't also be a valid PE/COFF image since they have
different magic numbers at the start of the image. Perhaps it would
be simpler to avoid introducing the load type tag and just load the
payload based on the magic number?

Ross

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 1/7] multiboot2: Add load type header and support for the PE binary type

[Roger Pau Monné]

On Tue, Mar 19, 2024 at 02:46:59PM +0000, Ross Lagerwall wrote:
> On Tue, Mar 19, 2024 at 1:18 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
> >
> > On Wed, Mar 13, 2024 at 03:07:42PM +0000, Ross Lagerwall wrote:
> > > Currently, multiboot2-compatible bootloaders can load ELF binaries and
> > > a.out binaries. The presence of the address header tag determines
> > > how the bootloader tries to interpret the binary (a.out if the address
> > > tag is present else ELF).
> > >
> > > Add a new load type header tag that explicitly states the type of the
> > > binary. Bootloaders should use the binary type specified in the load
> > > type tag. If the load type tag is not present, the bootloader should
> > > fall back to the previous heuristics.
> > >
> > > In addition to the existing address and ELF load types, specify a new
> > > optional PE binary load type. This new type is a useful addition since
> > > PE binaries can be signed and verified (i.e. used with Secure Boot).
> > >
> > > Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> > > ---
> > >  doc/multiboot.texi | 39 ++++++++++++++++++++++++++++++++++-----
> > >  doc/multiboot2.h   | 13 +++++++++++++
> > >  2 files changed, 47 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/doc/multiboot.texi b/doc/multiboot.texi
> > > index df8a0d056e76..d12719c744eb 100644
> > > --- a/doc/multiboot.texi
> > > +++ b/doc/multiboot.texi
> > > @@ -511,11 +511,12 @@ assumes that no bss segment is present.
> > >
> > >  Note: This information does not need to be provided if the kernel image
> > >  is in @sc{elf} format, but it must be provided if the image is in a.out
> > > -format or in some other format. When the address tag is present it must
> > > -be used in order to load the image, regardless of whether an @sc{elf}
> > > -header is also present. Compliant boot loaders must be able to load
> > > -images that are either in @sc{elf} format or contain the address tag
> > > -embedded in the Multiboot2 header.
> > > +format or in some other format. If the load type tag is not specified
> > > +and the address tag is present it must be used in order to load the
> > > +image, regardless of whether an @sc{elf} header is also present.
> > > +Compliant boot loaders must be able to load images that are either in
> > > +@sc{elf} format or contain the address tag embedded in the Multiboot2
> > > +header.
> > >
> > >  @subsection The entry address tag of Multiboot2 header
> > >
> > > @@ -732,6 +733,34 @@ and @samp{2} means load image at highest possible address but not
> > >  higher than max_addr.
> > >  @end table
> > >
> > > +@node Load type tag
> > > +@subsection Load type tag
> > > +
> > > +@example
> > > +@group
> > > +        +-------------------+
> > > +u16     | type = 11         |
> > > +u16     | flags             |
> > > +u32     | size = 12         |
> > > +u32     | load_type         |
> > > +        +-------------------+
> > > +@end group
> > > +@end example
> > > +
> > > +This tag indicates the type of the payload and how the boot loader
> > > +should load it.
> > > +
> > > +The meaning of each field is as follows:
> > > +
> > > +@table @code
> > > +@item load_type
> > > +Recognized load types are @samp{0} for address (i.e. load a.out using
> > > +the address tag), @samp{1} for ELF, and @samp{2} for PE. Compliant
> > > +bootloaders should implement support for a.out and ELF as a minimum.  If
> > > +this tag is not specified, the boot loader should attempt to load the
> > > +payload using the information specified in the address tag if present,
> > > +else it should load the payload as an ELF binary.  @end table
> >
> > I wonder if it would be simpler to use the following order instead:
> >
> > 1. Address tag
> > 2. Load type tag
> > 3. ELF header
> >
> > It's pointless to add a Loader type tag with load_type == 0, as that's
> > already mandated by the Address tag.  IOW: signaling the use of the
> > Address tag here is kind of pointless, if the fields in the Address
> > tag are set, that's the only signaling required for the data in the
> > Address tag to be used.
> >
> > Or are we attempting to support images that set Address tag and Load
> > type tag != 0 in order to use the Address tag when the loader doesn't
> > recognize the Load type tag, and otherwise use a different format?
> 
> No, that wasn't my intention. My intention for the load type tag was
> to have one tag that unambiguously describes the payload format and if
> that is missing, fall back to the previous logic for compatibility.
> It avoids more complicated heuristics if different payload types are
> added in the future.
> 
> >
> > Would it be sensible for multiboot2 to use PE in preference to ELF if
> > present on the image?  (without requiring any signaling).  I would
> > think this could be troublesome if kernels are so far expecting the
> > ELF header to be used with multiboot2, even if they also expose a PE
> > header.
> >
> 
> AFAIK an ELF image can't also be a valid PE/COFF image since they have
> different magic numbers at the start of the image. Perhaps it would
> be simpler to avoid introducing the load type tag and just load the
> payload based on the magic number?

I would likely be fine with just handling it like we handle ELF, if a
PE header is found use it.  As long as ELF and PE headers are mutually
exclusive.

Thanks, Roger.

Re: [PATCH 1/7] multiboot2: Add load type header and support for the PE binary type

[Roger Pau Monné via Grub-devel]

On Tue, Mar 19, 2024 at 02:46:59PM +0000, Ross Lagerwall wrote:
> On Tue, Mar 19, 2024 at 1:18 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
> >
> > On Wed, Mar 13, 2024 at 03:07:42PM +0000, Ross Lagerwall wrote:
> > > Currently, multiboot2-compatible bootloaders can load ELF binaries and
> > > a.out binaries. The presence of the address header tag determines
> > > how the bootloader tries to interpret the binary (a.out if the address
> > > tag is present else ELF).
> > >
> > > Add a new load type header tag that explicitly states the type of the
> > > binary. Bootloaders should use the binary type specified in the load
> > > type tag. If the load type tag is not present, the bootloader should
> > > fall back to the previous heuristics.
> > >
> > > In addition to the existing address and ELF load types, specify a new
> > > optional PE binary load type. This new type is a useful addition since
> > > PE binaries can be signed and verified (i.e. used with Secure Boot).
> > >
> > > Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> > > ---
> > >  doc/multiboot.texi | 39 ++++++++++++++++++++++++++++++++++-----
> > >  doc/multiboot2.h   | 13 +++++++++++++
> > >  2 files changed, 47 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/doc/multiboot.texi b/doc/multiboot.texi
> > > index df8a0d056e76..d12719c744eb 100644
> > > --- a/doc/multiboot.texi
> > > +++ b/doc/multiboot.texi
> > > @@ -511,11 +511,12 @@ assumes that no bss segment is present.
> > >
> > >  Note: This information does not need to be provided if the kernel image
> > >  is in @sc{elf} format, but it must be provided if the image is in a.out
> > > -format or in some other format. When the address tag is present it must
> > > -be used in order to load the image, regardless of whether an @sc{elf}
> > > -header is also present. Compliant boot loaders must be able to load
> > > -images that are either in @sc{elf} format or contain the address tag
> > > -embedded in the Multiboot2 header.
> > > +format or in some other format. If the load type tag is not specified
> > > +and the address tag is present it must be used in order to load the
> > > +image, regardless of whether an @sc{elf} header is also present.
> > > +Compliant boot loaders must be able to load images that are either in
> > > +@sc{elf} format or contain the address tag embedded in the Multiboot2
> > > +header.
> > >
> > >  @subsection The entry address tag of Multiboot2 header
> > >
> > > @@ -732,6 +733,34 @@ and @samp{2} means load image at highest possible address but not
> > >  higher than max_addr.
> > >  @end table
> > >
> > > +@node Load type tag
> > > +@subsection Load type tag
> > > +
> > > +@example
> > > +@group
> > > +        +-------------------+
> > > +u16     | type = 11         |
> > > +u16     | flags             |
> > > +u32     | size = 12         |
> > > +u32     | load_type         |
> > > +        +-------------------+
> > > +@end group
> > > +@end example
> > > +
> > > +This tag indicates the type of the payload and how the boot loader
> > > +should load it.
> > > +
> > > +The meaning of each field is as follows:
> > > +
> > > +@table @code
> > > +@item load_type
> > > +Recognized load types are @samp{0} for address (i.e. load a.out using
> > > +the address tag), @samp{1} for ELF, and @samp{2} for PE. Compliant
> > > +bootloaders should implement support for a.out and ELF as a minimum.  If
> > > +this tag is not specified, the boot loader should attempt to load the
> > > +payload using the information specified in the address tag if present,
> > > +else it should load the payload as an ELF binary.  @end table
> >
> > I wonder if it would be simpler to use the following order instead:
> >
> > 1. Address tag
> > 2. Load type tag
> > 3. ELF header
> >
> > It's pointless to add a Loader type tag with load_type == 0, as that's
> > already mandated by the Address tag.  IOW: signaling the use of the
> > Address tag here is kind of pointless, if the fields in the Address
> > tag are set, that's the only signaling required for the data in the
> > Address tag to be used.
> >
> > Or are we attempting to support images that set Address tag and Load
> > type tag != 0 in order to use the Address tag when the loader doesn't
> > recognize the Load type tag, and otherwise use a different format?
> 
> No, that wasn't my intention. My intention for the load type tag was
> to have one tag that unambiguously describes the payload format and if
> that is missing, fall back to the previous logic for compatibility.
> It avoids more complicated heuristics if different payload types are
> added in the future.
> 
> >
> > Would it be sensible for multiboot2 to use PE in preference to ELF if
> > present on the image?  (without requiring any signaling).  I would
> > think this could be troublesome if kernels are so far expecting the
> > ELF header to be used with multiboot2, even if they also expose a PE
> > header.
> >
> 
> AFAIK an ELF image can't also be a valid PE/COFF image since they have
> different magic numbers at the start of the image. Perhaps it would
> be simpler to avoid introducing the load type tag and just load the
> payload based on the magic number?

I would likely be fine with just handling it like we handle ELF, if a
PE header is found use it.  As long as ELF and PE headers are mutually
exclusive.

Thanks, Roger.

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH 2/7] multiboot2: Allow 64-bit entry tags

[Ross Lagerwall]

Binaries may be built with entry points above 4G. While bootloaders may
relocate them below 4G, it should be possible for the binary to specify
those entry points. Therefore, extend the multiboot2 protocol such that
64 bit addresses are allowed for entry points. The extension is done in
a backwards-compatible way.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 doc/multiboot.texi | 32 +++++++++++++++++++-------------
 doc/multiboot2.h   |  6 +++++-
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/doc/multiboot.texi b/doc/multiboot.texi
index d12719c744eb..049afab53c1f 100644
--- a/doc/multiboot.texi
+++ b/doc/multiboot.texi
@@ -522,12 +522,12 @@ header.
 
 @example
 @group
-        +-------------------+
-u16     | type = 3          |
-u16     | flags             |
-u32     | size              |
-u32     | entry_addr        |
-        +-------------------+
+          +-------------------+
+u16       | type = 3          |
+u16       | flags             |
+u32       | size              |
+u32 / u64 | entry_addr        |
+          +-------------------+
 @end group
 @end example
 
@@ -538,7 +538,10 @@ The meaning of each is as follows:
 
 @item entry_addr
 The physical address to which the boot loader should jump in order to
-start running the operating system.
+start running the operating system. @samp{entry_addr} may be specified
+either as a @samp{u32} or @samp{u64}. The bootloader should use the
+header size to determine the size of @samp{entry_addr}.
+
 @end table
 
 @subsection EFI i386 entry address tag of Multiboot2 header
@@ -573,12 +576,12 @@ tag of Multiboot2 header are ignored.
 
 @example
 @group
-        +-------------------+
-u16     | type = 9          |
-u16     | flags             |
-u32     | size              |
-u32     | entry_addr        |
-        +-------------------+
+          +-------------------+
+u16       | type = 9          |
+u16       | flags             |
+u32       | size              |
+u32 / u64 | entry_addr        |
+          +-------------------+
 @end group
 @end example
 
@@ -594,6 +597,9 @@ is as follows:
 @item entry_addr
 The physical address to which the boot loader should jump in order to
 start running EFI amd64 compatible operating system code.
+@samp{entry_addr} may be specified either as a @samp{u32} or @samp{u64}.
+The bootloader should use the header size to determine the size of
+@samp{entry_addr}.
 @end table
 
 This tag is taken into account only on EFI amd64 platforms
diff --git a/doc/multiboot2.h b/doc/multiboot2.h
index d4cae05706e4..a994a7b28b02 100644
--- a/doc/multiboot2.h
+++ b/doc/multiboot2.h
@@ -141,7 +141,11 @@ struct multiboot_header_tag_entry_address
   multiboot_uint16_t type;
   multiboot_uint16_t flags;
   multiboot_uint32_t size;
-  multiboot_uint32_t entry_addr;
+  union
+  {
+    multiboot_uint32_t entry_addr32;
+    multiboot_uint64_t entry_addr64;
+  };
 };
 
 struct multiboot_header_tag_console_flags
-- 
2.43.0

[PATCH 2/7] multiboot2: Allow 64-bit entry tags

[Ross Lagerwall via Grub-devel]

Binaries may be built with entry points above 4G. While bootloaders may
relocate them below 4G, it should be possible for the binary to specify
those entry points. Therefore, extend the multiboot2 protocol such that
64 bit addresses are allowed for entry points. The extension is done in
a backwards-compatible way.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 doc/multiboot.texi | 32 +++++++++++++++++++-------------
 doc/multiboot2.h   |  6 +++++-
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/doc/multiboot.texi b/doc/multiboot.texi
index d12719c744eb..049afab53c1f 100644
--- a/doc/multiboot.texi
+++ b/doc/multiboot.texi
@@ -522,12 +522,12 @@ header.
 
 @example
 @group
-        +-------------------+
-u16     | type = 3          |
-u16     | flags             |
-u32     | size              |
-u32     | entry_addr        |
-        +-------------------+
+          +-------------------+
+u16       | type = 3          |
+u16       | flags             |
+u32       | size              |
+u32 / u64 | entry_addr        |
+          +-------------------+
 @end group
 @end example
 
@@ -538,7 +538,10 @@ The meaning of each is as follows:
 
 @item entry_addr
 The physical address to which the boot loader should jump in order to
-start running the operating system.
+start running the operating system. @samp{entry_addr} may be specified
+either as a @samp{u32} or @samp{u64}. The bootloader should use the
+header size to determine the size of @samp{entry_addr}.
+
 @end table
 
 @subsection EFI i386 entry address tag of Multiboot2 header
@@ -573,12 +576,12 @@ tag of Multiboot2 header are ignored.
 
 @example
 @group
-        +-------------------+
-u16     | type = 9          |
-u16     | flags             |
-u32     | size              |
-u32     | entry_addr        |
-        +-------------------+
+          +-------------------+
+u16       | type = 9          |
+u16       | flags             |
+u32       | size              |
+u32 / u64 | entry_addr        |
+          +-------------------+
 @end group
 @end example
 
@@ -594,6 +597,9 @@ is as follows:
 @item entry_addr
 The physical address to which the boot loader should jump in order to
 start running EFI amd64 compatible operating system code.
+@samp{entry_addr} may be specified either as a @samp{u32} or @samp{u64}.
+The bootloader should use the header size to determine the size of
+@samp{entry_addr}.
 @end table
 
 This tag is taken into account only on EFI amd64 platforms
diff --git a/doc/multiboot2.h b/doc/multiboot2.h
index d4cae05706e4..a994a7b28b02 100644
--- a/doc/multiboot2.h
+++ b/doc/multiboot2.h
@@ -141,7 +141,11 @@ struct multiboot_header_tag_entry_address
   multiboot_uint16_t type;
   multiboot_uint16_t flags;
   multiboot_uint32_t size;
-  multiboot_uint32_t entry_addr;
+  union
+  {
+    multiboot_uint32_t entry_addr32;
+    multiboot_uint64_t entry_addr64;
+  };
 };
 
 struct multiboot_header_tag_console_flags
-- 
2.43.0


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 2/7] multiboot2: Allow 64-bit entry tags

[Roger Pau Monné]

On Wed, Mar 13, 2024 at 03:07:43PM +0000, Ross Lagerwall wrote:
> Binaries may be built with entry points above 4G. While bootloaders may
> relocate them below 4G, it should be possible for the binary to specify
> those entry points. Therefore, extend the multiboot2 protocol such that
> 64 bit addresses are allowed for entry points. The extension is done in
> a backwards-compatible way.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
>  doc/multiboot.texi | 32 +++++++++++++++++++-------------
>  doc/multiboot2.h   |  6 +++++-
>  2 files changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/doc/multiboot.texi b/doc/multiboot.texi
> index d12719c744eb..049afab53c1f 100644
> --- a/doc/multiboot.texi
> +++ b/doc/multiboot.texi
> @@ -522,12 +522,12 @@ header.
>  
>  @example
>  @group
> -        +-------------------+
> -u16     | type = 3          |
> -u16     | flags             |
> -u32     | size              |
> -u32     | entry_addr        |
> -        +-------------------+
> +          +-------------------+
> +u16       | type = 3          |
> +u16       | flags             |
> +u32       | size              |
> +u32 / u64 | entry_addr        |
> +          +-------------------+

I might be confused, but this entry point is used in 32bit protected
mode, and hence a 64bit value is simply impossible to use according to
the protocol in "3.3 I386 machine state".

Unless that section is expanded to describe other protocols that use
the entry address in a way where 64bits could be meaningful it seems
pointless to expand the field.

>  @end group
>  @end example
>  
> @@ -538,7 +538,10 @@ The meaning of each is as follows:
>  
>  @item entry_addr
>  The physical address to which the boot loader should jump in order to
> -start running the operating system.
> +start running the operating system. @samp{entry_addr} may be specified
> +either as a @samp{u32} or @samp{u64}. The bootloader should use the
> +header size to determine the size of @samp{entry_addr}.
> +
>  @end table
>  
>  @subsection EFI i386 entry address tag of Multiboot2 header
> @@ -573,12 +576,12 @@ tag of Multiboot2 header are ignored.
>  
>  @example
>  @group
> -        +-------------------+
> -u16     | type = 9          |
> -u16     | flags             |
> -u32     | size              |
> -u32     | entry_addr        |
> -        +-------------------+
> +          +-------------------+
> +u16       | type = 9          |
> +u16       | flags             |
> +u32       | size              |
> +u32 / u64 | entry_addr        |
> +          +-------------------+

This does seem sensible.

Thanks, Roger.

Re: [PATCH 2/7] multiboot2: Allow 64-bit entry tags

[Roger Pau Monné via Grub-devel]

On Wed, Mar 13, 2024 at 03:07:43PM +0000, Ross Lagerwall wrote:
> Binaries may be built with entry points above 4G. While bootloaders may
> relocate them below 4G, it should be possible for the binary to specify
> those entry points. Therefore, extend the multiboot2 protocol such that
> 64 bit addresses are allowed for entry points. The extension is done in
> a backwards-compatible way.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
>  doc/multiboot.texi | 32 +++++++++++++++++++-------------
>  doc/multiboot2.h   |  6 +++++-
>  2 files changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/doc/multiboot.texi b/doc/multiboot.texi
> index d12719c744eb..049afab53c1f 100644
> --- a/doc/multiboot.texi
> +++ b/doc/multiboot.texi
> @@ -522,12 +522,12 @@ header.
>  
>  @example
>  @group
> -        +-------------------+
> -u16     | type = 3          |
> -u16     | flags             |
> -u32     | size              |
> -u32     | entry_addr        |
> -        +-------------------+
> +          +-------------------+
> +u16       | type = 3          |
> +u16       | flags             |
> +u32       | size              |
> +u32 / u64 | entry_addr        |
> +          +-------------------+

I might be confused, but this entry point is used in 32bit protected
mode, and hence a 64bit value is simply impossible to use according to
the protocol in "3.3 I386 machine state".

Unless that section is expanded to describe other protocols that use
the entry address in a way where 64bits could be meaningful it seems
pointless to expand the field.

>  @end group
>  @end example
>  
> @@ -538,7 +538,10 @@ The meaning of each is as follows:
>  
>  @item entry_addr
>  The physical address to which the boot loader should jump in order to
> -start running the operating system.
> +start running the operating system. @samp{entry_addr} may be specified
> +either as a @samp{u32} or @samp{u64}. The bootloader should use the
> +header size to determine the size of @samp{entry_addr}.
> +
>  @end table
>  
>  @subsection EFI i386 entry address tag of Multiboot2 header
> @@ -573,12 +576,12 @@ tag of Multiboot2 header are ignored.
>  
>  @example
>  @group
> -        +-------------------+
> -u16     | type = 9          |
> -u16     | flags             |
> -u32     | size              |
> -u32     | entry_addr        |
> -        +-------------------+
> +          +-------------------+
> +u16       | type = 9          |
> +u16       | flags             |
> +u32       | size              |
> +u32 / u64 | entry_addr        |
> +          +-------------------+

This does seem sensible.

Thanks, Roger.

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 2/7] multiboot2: Allow 64-bit entry tags

[Ross Lagerwall]

On Tue, Mar 19, 2024 at 10:07 AM Roger Pau Monné <roger.pau@citrix.com> wrote:
>
> On Wed, Mar 13, 2024 at 03:07:43PM +0000, Ross Lagerwall wrote:
> > Binaries may be built with entry points above 4G. While bootloaders may
> > relocate them below 4G, it should be possible for the binary to specify
> > those entry points. Therefore, extend the multiboot2 protocol such that
> > 64 bit addresses are allowed for entry points. The extension is done in
> > a backwards-compatible way.
> >
> > Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> > ---
> >  doc/multiboot.texi | 32 +++++++++++++++++++-------------
> >  doc/multiboot2.h   |  6 +++++-
> >  2 files changed, 24 insertions(+), 14 deletions(-)
> >
> > diff --git a/doc/multiboot.texi b/doc/multiboot.texi
> > index d12719c744eb..049afab53c1f 100644
> > --- a/doc/multiboot.texi
> > +++ b/doc/multiboot.texi
> > @@ -522,12 +522,12 @@ header.
> >
> >  @example
> >  @group
> > -        +-------------------+
> > -u16     | type = 3          |
> > -u16     | flags             |
> > -u32     | size              |
> > -u32     | entry_addr        |
> > -        +-------------------+
> > +          +-------------------+
> > +u16       | type = 3          |
> > +u16       | flags             |
> > +u32       | size              |
> > +u32 / u64 | entry_addr        |
> > +          +-------------------+
>
> I might be confused, but this entry point is used in 32bit protected
> mode, and hence a 64bit value is simply impossible to use according to
> the protocol in "3.3 I386 machine state".
>
> Unless that section is expanded to describe other protocols that use
> the entry address in a way where 64bits could be meaningful it seems
> pointless to expand the field.

I changed this because the same binary is being used for both BIOS boot
and UEFI boot, therefore it may have a base address above 4 GiB.
Despite that, it is expected that GRUB would relocate the binary below
4 GiB so BIOS boot would still work.

However, on reflection this is kind of nasty. I've managed to build Xen
in such a way that this is no longer needed so I can drop this change
from the next version of this series.

Ross

Re: [PATCH 2/7] multiboot2: Allow 64-bit entry tags

[Ross Lagerwall via Grub-devel]

On Tue, Mar 19, 2024 at 10:07 AM Roger Pau Monné <roger.pau@citrix.com> wrote:
>
> On Wed, Mar 13, 2024 at 03:07:43PM +0000, Ross Lagerwall wrote:
> > Binaries may be built with entry points above 4G. While bootloaders may
> > relocate them below 4G, it should be possible for the binary to specify
> > those entry points. Therefore, extend the multiboot2 protocol such that
> > 64 bit addresses are allowed for entry points. The extension is done in
> > a backwards-compatible way.
> >
> > Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> > ---
> >  doc/multiboot.texi | 32 +++++++++++++++++++-------------
> >  doc/multiboot2.h   |  6 +++++-
> >  2 files changed, 24 insertions(+), 14 deletions(-)
> >
> > diff --git a/doc/multiboot.texi b/doc/multiboot.texi
> > index d12719c744eb..049afab53c1f 100644
> > --- a/doc/multiboot.texi
> > +++ b/doc/multiboot.texi
> > @@ -522,12 +522,12 @@ header.
> >
> >  @example
> >  @group
> > -        +-------------------+
> > -u16     | type = 3          |
> > -u16     | flags             |
> > -u32     | size              |
> > -u32     | entry_addr        |
> > -        +-------------------+
> > +          +-------------------+
> > +u16       | type = 3          |
> > +u16       | flags             |
> > +u32       | size              |
> > +u32 / u64 | entry_addr        |
> > +          +-------------------+
>
> I might be confused, but this entry point is used in 32bit protected
> mode, and hence a 64bit value is simply impossible to use according to
> the protocol in "3.3 I386 machine state".
>
> Unless that section is expanded to describe other protocols that use
> the entry address in a way where 64bits could be meaningful it seems
> pointless to expand the field.

I changed this because the same binary is being used for both BIOS boot
and UEFI boot, therefore it may have a base address above 4 GiB.
Despite that, it is expected that GRUB would relocate the binary below
4 GiB so BIOS boot would still work.

However, on reflection this is kind of nasty. I've managed to build Xen
in such a way that this is no longer needed so I can drop this change
from the next version of this series.

Ross

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 2/7] multiboot2: Allow 64-bit entry tags

[Roger Pau Monné]

On Thu, Mar 28, 2024 at 03:05:47PM +0000, Ross Lagerwall wrote:
> On Tue, Mar 19, 2024 at 10:07 AM Roger Pau Monné <roger.pau@citrix.com> wrote:
> >
> > On Wed, Mar 13, 2024 at 03:07:43PM +0000, Ross Lagerwall wrote:
> > > Binaries may be built with entry points above 4G. While bootloaders may
> > > relocate them below 4G, it should be possible for the binary to specify
> > > those entry points. Therefore, extend the multiboot2 protocol such that
> > > 64 bit addresses are allowed for entry points. The extension is done in
> > > a backwards-compatible way.
> > >
> > > Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> > > ---
> > >  doc/multiboot.texi | 32 +++++++++++++++++++-------------
> > >  doc/multiboot2.h   |  6 +++++-
> > >  2 files changed, 24 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/doc/multiboot.texi b/doc/multiboot.texi
> > > index d12719c744eb..049afab53c1f 100644
> > > --- a/doc/multiboot.texi
> > > +++ b/doc/multiboot.texi
> > > @@ -522,12 +522,12 @@ header.
> > >
> > >  @example
> > >  @group
> > > -        +-------------------+
> > > -u16     | type = 3          |
> > > -u16     | flags             |
> > > -u32     | size              |
> > > -u32     | entry_addr        |
> > > -        +-------------------+
> > > +          +-------------------+
> > > +u16       | type = 3          |
> > > +u16       | flags             |
> > > +u32       | size              |
> > > +u32 / u64 | entry_addr        |
> > > +          +-------------------+
> >
> > I might be confused, but this entry point is used in 32bit protected
> > mode, and hence a 64bit value is simply impossible to use according to
> > the protocol in "3.3 I386 machine state".
> >
> > Unless that section is expanded to describe other protocols that use
> > the entry address in a way where 64bits could be meaningful it seems
> > pointless to expand the field.
> 
> I changed this because the same binary is being used for both BIOS boot
> and UEFI boot, therefore it may have a base address above 4 GiB.
> Despite that, it is expected that GRUB would relocate the binary below
> 4 GiB so BIOS boot would still work.

Right, for UEFI boot it's possible to have entry addresses above 4GB,
because the entry point is called in long mode with identity page
tables (and hence you can put addresses in %rip past the 4GB
boundary).

However the multiboot entry point puts the CPU in 32bit protected
mode, and hence %eip can only hold a value below the 4GB boundary.

It's technically impossible to use an entry point above 4GB, unless
there's something that I'm missing that changes the initial CPU state
for the multiboot2 entry point.

Thanks, Roger.

Re: [PATCH 2/7] multiboot2: Allow 64-bit entry tags

[Roger Pau Monné via Grub-devel]

On Thu, Mar 28, 2024 at 03:05:47PM +0000, Ross Lagerwall wrote:
> On Tue, Mar 19, 2024 at 10:07 AM Roger Pau Monné <roger.pau@citrix.com> wrote:
> >
> > On Wed, Mar 13, 2024 at 03:07:43PM +0000, Ross Lagerwall wrote:
> > > Binaries may be built with entry points above 4G. While bootloaders may
> > > relocate them below 4G, it should be possible for the binary to specify
> > > those entry points. Therefore, extend the multiboot2 protocol such that
> > > 64 bit addresses are allowed for entry points. The extension is done in
> > > a backwards-compatible way.
> > >
> > > Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> > > ---
> > >  doc/multiboot.texi | 32 +++++++++++++++++++-------------
> > >  doc/multiboot2.h   |  6 +++++-
> > >  2 files changed, 24 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/doc/multiboot.texi b/doc/multiboot.texi
> > > index d12719c744eb..049afab53c1f 100644
> > > --- a/doc/multiboot.texi
> > > +++ b/doc/multiboot.texi
> > > @@ -522,12 +522,12 @@ header.
> > >
> > >  @example
> > >  @group
> > > -        +-------------------+
> > > -u16     | type = 3          |
> > > -u16     | flags             |
> > > -u32     | size              |
> > > -u32     | entry_addr        |
> > > -        +-------------------+
> > > +          +-------------------+
> > > +u16       | type = 3          |
> > > +u16       | flags             |
> > > +u32       | size              |
> > > +u32 / u64 | entry_addr        |
> > > +          +-------------------+
> >
> > I might be confused, but this entry point is used in 32bit protected
> > mode, and hence a 64bit value is simply impossible to use according to
> > the protocol in "3.3 I386 machine state".
> >
> > Unless that section is expanded to describe other protocols that use
> > the entry address in a way where 64bits could be meaningful it seems
> > pointless to expand the field.
> 
> I changed this because the same binary is being used for both BIOS boot
> and UEFI boot, therefore it may have a base address above 4 GiB.
> Despite that, it is expected that GRUB would relocate the binary below
> 4 GiB so BIOS boot would still work.

Right, for UEFI boot it's possible to have entry addresses above 4GB,
because the entry point is called in long mode with identity page
tables (and hence you can put addresses in %rip past the 4GB
boundary).

However the multiboot entry point puts the CPU in 32bit protected
mode, and hence %eip can only hold a value below the 4GB boundary.

It's technically impossible to use an entry point above 4GB, unless
there's something that I'm missing that changes the initial CPU state
for the multiboot2 entry point.

Thanks, Roger.

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH 3/7] multiboot2: Add support for the load type header tag

[Ross Lagerwall]

The binary may expose its type using the load type header tag. Implement
it according to the specification.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 grub-core/loader/multiboot_mbi2.c | 45 ++++++++++++++++++++++++++++---
 include/grub/multiboot2.h         |  1 +
 include/multiboot2.h              | 13 +++++++++
 3 files changed, 56 insertions(+), 3 deletions(-)

diff --git a/grub-core/loader/multiboot_mbi2.c b/grub-core/loader/multiboot_mbi2.c
index 00a48413c013..6ef4f265070a 100644
--- a/grub-core/loader/multiboot_mbi2.c
+++ b/grub-core/loader/multiboot_mbi2.c
@@ -119,10 +119,12 @@ grub_multiboot2_load (grub_file_t file, const char *filename)
   grub_uint32_t console_required = 0;
   struct multiboot_header_tag_framebuffer *fbtag = NULL;
   int accepted_consoles = GRUB_MULTIBOOT2_CONSOLE_EGA_TEXT;
+  struct multiboot_header_tag_load_type *load_type_tag;
   mbi_load_data_t mld;
 
   mld.mbi_ver = 2;
   mld.relocatable = 0;
+  mld.load_type = MULTIBOOT_LOAD_TYPE_TOTAL;
 
   mld.buffer = grub_malloc (MULTIBOOT_SEARCH);
   if (!mld.buffer)
@@ -258,6 +260,17 @@ grub_multiboot2_load (grub_file_t file, const char *filename)
 #endif
 	break;
 
+      case MULTIBOOT_HEADER_TAG_LOAD_TYPE:
+        load_type_tag = (struct multiboot_header_tag_load_type *) tag;
+        mld.load_type = load_type_tag->load_type;
+        if (mld.load_type >= MULTIBOOT_LOAD_TYPE_TOTAL)
+          {
+              grub_free (mld.buffer);
+              return grub_error (GRUB_ERR_UNKNOWN_OS,
+                                 "unsupported load type: %u", load_type_tag->load_type);
+          }
+        break;
+
       default:
 	if (! (tag->flags & MULTIBOOT_HEADER_TAG_OPTIONAL))
 	  {
@@ -268,14 +281,32 @@ grub_multiboot2_load (grub_file_t file, const char *filename)
 	break;
       }
 
-  if (addr_tag && !entry_specified && !(keep_bs && efi_entry_specified))
+  if (mld.load_type == MULTIBOOT_LOAD_TYPE_ADDRESS && !addr_tag)
+    {
+      grub_free (mld.buffer);
+      return grub_error (GRUB_ERR_UNKNOWN_OS,
+                         "load type address without address tag");
+    }
+  if (mld.load_type < MULTIBOOT_LOAD_TYPE_TOTAL &&
+      mld.load_type != MULTIBOOT_LOAD_TYPE_ADDRESS && addr_tag)
+    {
+      grub_free (mld.buffer);
+      return grub_error (GRUB_ERR_UNKNOWN_OS,
+                         "address tag specified but load type doesn't match");
+    }
+  if (mld.load_type == MULTIBOOT_LOAD_TYPE_TOTAL)
+    mld.load_type = addr_tag ? MULTIBOOT_LOAD_TYPE_ADDRESS : MULTIBOOT_LOAD_TYPE_ELF;
+
+  if (mld.load_type == MULTIBOOT_LOAD_TYPE_ADDRESS && !entry_specified &&
+      !(keep_bs && efi_entry_specified))
     {
       grub_free (mld.buffer);
       return grub_error (GRUB_ERR_UNKNOWN_OS,
 			 "load address tag without entry address tag");
     }
 
-  if (addr_tag)
+  switch (mld.load_type) {
+  case MULTIBOOT_LOAD_TYPE_ADDRESS:
     {
       grub_uint64_t load_addr = (addr_tag->load_addr + 1)
 	? addr_tag->load_addr : (addr_tag->header_addr
@@ -343,8 +374,9 @@ grub_multiboot2_load (grub_file_t file, const char *filename)
       if (addr_tag->bss_end_addr)
 	grub_memset ((grub_uint8_t *) source + load_size, 0,
 		     addr_tag->bss_end_addr - load_addr - load_size);
+      break;
     }
-  else
+  case MULTIBOOT_LOAD_TYPE_ELF:
     {
       mld.file = file;
       mld.filename = filename;
@@ -355,7 +387,14 @@ grub_multiboot2_load (grub_file_t file, const char *filename)
 	  grub_free (mld.buffer);
 	  return err;
 	}
+      break;
     }
+  case MULTIBOOT_LOAD_TYPE_PE:
+      grub_fatal ("Unsupported load type: %u\n", mld.load_type);
+  default:
+    /* should be impossible */
+    grub_fatal ("Unknown load type: %u\n", mld.load_type);
+  }
 
   load_base_addr = mld.load_base_addr;
 
diff --git a/include/grub/multiboot2.h b/include/grub/multiboot2.h
index b90aa6989674..1f6d4fc9e336 100644
--- a/include/grub/multiboot2.h
+++ b/include/grub/multiboot2.h
@@ -89,6 +89,7 @@ struct mbi_load_data
   grub_uint32_t link_base_addr;
   grub_uint32_t load_base_addr;
   int avoid_efi_boot_services;
+  grub_uint32_t load_type;
 };
 typedef struct mbi_load_data mbi_load_data_t;
 
diff --git a/include/multiboot2.h b/include/multiboot2.h
index a039aa0439aa..f4377bf7b394 100644
--- a/include/multiboot2.h
+++ b/include/multiboot2.h
@@ -74,6 +74,7 @@
 #define MULTIBOOT_HEADER_TAG_EFI_BS  7
 #define MULTIBOOT_HEADER_TAG_ENTRY_ADDRESS_EFI64  9
 #define MULTIBOOT_HEADER_TAG_RELOCATABLE  10
+#define MULTIBOOT_HEADER_TAG_LOAD_TYPE  11
 
 #define MULTIBOOT2_ARCHITECTURE_I386  0
 #define MULTIBOOT2_ARCHITECTURE_MIPS32  4
@@ -178,6 +179,18 @@ struct multiboot_header_tag_relocatable
   multiboot_uint32_t preference;
 };
 
+struct multiboot_header_tag_load_type
+{
+  multiboot_uint16_t type;
+  multiboot_uint16_t flags;
+  multiboot_uint32_t size;
+#define MULTIBOOT_LOAD_TYPE_ADDRESS     0
+#define MULTIBOOT_LOAD_TYPE_ELF         1
+#define MULTIBOOT_LOAD_TYPE_PE          2
+#define MULTIBOOT_LOAD_TYPE_TOTAL       3
+  multiboot_uint32_t load_type;
+};
+
 struct multiboot_color
 {
   multiboot_uint8_t red;
-- 
2.43.0

[PATCH 3/7] multiboot2: Add support for the load type header tag

[Ross Lagerwall via Grub-devel]

The binary may expose its type using the load type header tag. Implement
it according to the specification.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 grub-core/loader/multiboot_mbi2.c | 45 ++++++++++++++++++++++++++++---
 include/grub/multiboot2.h         |  1 +
 include/multiboot2.h              | 13 +++++++++
 3 files changed, 56 insertions(+), 3 deletions(-)

diff --git a/grub-core/loader/multiboot_mbi2.c b/grub-core/loader/multiboot_mbi2.c
index 00a48413c013..6ef4f265070a 100644
--- a/grub-core/loader/multiboot_mbi2.c
+++ b/grub-core/loader/multiboot_mbi2.c
@@ -119,10 +119,12 @@ grub_multiboot2_load (grub_file_t file, const char *filename)
   grub_uint32_t console_required = 0;
   struct multiboot_header_tag_framebuffer *fbtag = NULL;
   int accepted_consoles = GRUB_MULTIBOOT2_CONSOLE_EGA_TEXT;
+  struct multiboot_header_tag_load_type *load_type_tag;
   mbi_load_data_t mld;
 
   mld.mbi_ver = 2;
   mld.relocatable = 0;
+  mld.load_type = MULTIBOOT_LOAD_TYPE_TOTAL;
 
   mld.buffer = grub_malloc (MULTIBOOT_SEARCH);
   if (!mld.buffer)
@@ -258,6 +260,17 @@ grub_multiboot2_load (grub_file_t file, const char *filename)
 #endif
 	break;
 
+      case MULTIBOOT_HEADER_TAG_LOAD_TYPE:
+        load_type_tag = (struct multiboot_header_tag_load_type *) tag;
+        mld.load_type = load_type_tag->load_type;
+        if (mld.load_type >= MULTIBOOT_LOAD_TYPE_TOTAL)
+          {
+              grub_free (mld.buffer);
+              return grub_error (GRUB_ERR_UNKNOWN_OS,
+                                 "unsupported load type: %u", load_type_tag->load_type);
+          }
+        break;
+
       default:
 	if (! (tag->flags & MULTIBOOT_HEADER_TAG_OPTIONAL))
 	  {
@@ -268,14 +281,32 @@ grub_multiboot2_load (grub_file_t file, const char *filename)
 	break;
       }
 
-  if (addr_tag && !entry_specified && !(keep_bs && efi_entry_specified))
+  if (mld.load_type == MULTIBOOT_LOAD_TYPE_ADDRESS && !addr_tag)
+    {
+      grub_free (mld.buffer);
+      return grub_error (GRUB_ERR_UNKNOWN_OS,
+                         "load type address without address tag");
+    }
+  if (mld.load_type < MULTIBOOT_LOAD_TYPE_TOTAL &&
+      mld.load_type != MULTIBOOT_LOAD_TYPE_ADDRESS && addr_tag)
+    {
+      grub_free (mld.buffer);
+      return grub_error (GRUB_ERR_UNKNOWN_OS,
+                         "address tag specified but load type doesn't match");
+    }
+  if (mld.load_type == MULTIBOOT_LOAD_TYPE_TOTAL)
+    mld.load_type = addr_tag ? MULTIBOOT_LOAD_TYPE_ADDRESS : MULTIBOOT_LOAD_TYPE_ELF;
+
+  if (mld.load_type == MULTIBOOT_LOAD_TYPE_ADDRESS && !entry_specified &&
+      !(keep_bs && efi_entry_specified))
     {
       grub_free (mld.buffer);
       return grub_error (GRUB_ERR_UNKNOWN_OS,
 			 "load address tag without entry address tag");
     }
 
-  if (addr_tag)
+  switch (mld.load_type) {
+  case MULTIBOOT_LOAD_TYPE_ADDRESS:
     {
       grub_uint64_t load_addr = (addr_tag->load_addr + 1)
 	? addr_tag->load_addr : (addr_tag->header_addr
@@ -343,8 +374,9 @@ grub_multiboot2_load (grub_file_t file, const char *filename)
       if (addr_tag->bss_end_addr)
 	grub_memset ((grub_uint8_t *) source + load_size, 0,
 		     addr_tag->bss_end_addr - load_addr - load_size);
+      break;
     }
-  else
+  case MULTIBOOT_LOAD_TYPE_ELF:
     {
       mld.file = file;
       mld.filename = filename;
@@ -355,7 +387,14 @@ grub_multiboot2_load (grub_file_t file, const char *filename)
 	  grub_free (mld.buffer);
 	  return err;
 	}
+      break;
     }
+  case MULTIBOOT_LOAD_TYPE_PE:
+      grub_fatal ("Unsupported load type: %u\n", mld.load_type);
+  default:
+    /* should be impossible */
+    grub_fatal ("Unknown load type: %u\n", mld.load_type);
+  }
 
   load_base_addr = mld.load_base_addr;
 
diff --git a/include/grub/multiboot2.h b/include/grub/multiboot2.h
index b90aa6989674..1f6d4fc9e336 100644
--- a/include/grub/multiboot2.h
+++ b/include/grub/multiboot2.h
@@ -89,6 +89,7 @@ struct mbi_load_data
   grub_uint32_t link_base_addr;
   grub_uint32_t load_base_addr;
   int avoid_efi_boot_services;
+  grub_uint32_t load_type;
 };
 typedef struct mbi_load_data mbi_load_data_t;
 
diff --git a/include/multiboot2.h b/include/multiboot2.h
index a039aa0439aa..f4377bf7b394 100644
--- a/include/multiboot2.h
+++ b/include/multiboot2.h
@@ -74,6 +74,7 @@
 #define MULTIBOOT_HEADER_TAG_EFI_BS  7
 #define MULTIBOOT_HEADER_TAG_ENTRY_ADDRESS_EFI64  9
 #define MULTIBOOT_HEADER_TAG_RELOCATABLE  10
+#define MULTIBOOT_HEADER_TAG_LOAD_TYPE  11
 
 #define MULTIBOOT2_ARCHITECTURE_I386  0
 #define MULTIBOOT2_ARCHITECTURE_MIPS32  4
@@ -178,6 +179,18 @@ struct multiboot_header_tag_relocatable
   multiboot_uint32_t preference;
 };
 
+struct multiboot_header_tag_load_type
+{
+  multiboot_uint16_t type;
+  multiboot_uint16_t flags;
+  multiboot_uint32_t size;
+#define MULTIBOOT_LOAD_TYPE_ADDRESS     0
+#define MULTIBOOT_LOAD_TYPE_ELF         1
+#define MULTIBOOT_LOAD_TYPE_PE          2
+#define MULTIBOOT_LOAD_TYPE_TOTAL       3
+  multiboot_uint32_t load_type;
+};
+
 struct multiboot_color
 {
   multiboot_uint8_t red;
-- 
2.43.0


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 3/7] multiboot2: Add support for the load type header tag

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 444 bytes --]

Not a full review. Just one blocking problem


>
>      }
> +  case MULTIBOOT_LOAD_TYPE_PE:
> +      grub_fatal ("Unsupported load type: %u\n", mld.load_type);
> +  default:
> +    /* should be impossible */
> +    grub_fatal ("Unknown load type: %u\n", mld.load_type);
>
Don't use grub_fatal for this. grub_fatal is only when continue to execute
grub is unwise. Here you just have an unsupported file. This is definitely
a GRUB_ERR_BAD_OS


>

[-- Attachment #2: Type: text/html, Size: 894 bytes --]

Re: [PATCH 3/7] multiboot2: Add support for the load type header tag

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1.1: Type: text/plain, Size: 444 bytes --]

Not a full review. Just one blocking problem


>
>      }
> +  case MULTIBOOT_LOAD_TYPE_PE:
> +      grub_fatal ("Unsupported load type: %u\n", mld.load_type);
> +  default:
> +    /* should be impossible */
> +    grub_fatal ("Unknown load type: %u\n", mld.load_type);
>
Don't use grub_fatal for this. grub_fatal is only when continue to execute
grub is unwise. Here you just have an unsupported file. This is definitely
a GRUB_ERR_BAD_OS


>

[-- Attachment #1.2: Type: text/html, Size: 894 bytes --]

[-- Attachment #2: Type: text/plain, Size: 141 bytes --]

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 3/7] multiboot2: Add support for the load type header tag

[Ross Lagerwall]

On Fri, Mar 15, 2024 at 7:31 AM Vladimir 'phcoder' Serbinenko
<phcoder@gmail.com> wrote:
>
> Not a full review. Just one blocking problem
>
>>
>>
>>      }
>> +  case MULTIBOOT_LOAD_TYPE_PE:
>> +      grub_fatal ("Unsupported load type: %u\n", mld.load_type);
>> +  default:
>> +    /* should be impossible */
>> +    grub_fatal ("Unknown load type: %u\n", mld.load_type);
>
> Don't use grub_fatal for this. grub_fatal is only when continue to execute grub is unwise. Here you just have an unsupported file. This is definitely a GRUB_ERR_BAD_OS
>

Noted... I'll fix that.

Thanks,
Ross

Re: [PATCH 3/7] multiboot2: Add support for the load type header tag

[Ross Lagerwall via Grub-devel]

On Fri, Mar 15, 2024 at 7:31 AM Vladimir 'phcoder' Serbinenko
<phcoder@gmail.com> wrote:
>
> Not a full review. Just one blocking problem
>
>>
>>
>>      }
>> +  case MULTIBOOT_LOAD_TYPE_PE:
>> +      grub_fatal ("Unsupported load type: %u\n", mld.load_type);
>> +  default:
>> +    /* should be impossible */
>> +    grub_fatal ("Unknown load type: %u\n", mld.load_type);
>
> Don't use grub_fatal for this. grub_fatal is only when continue to execute grub is unwise. Here you just have an unsupported file. This is definitely a GRUB_ERR_BAD_OS
>

Noted... I'll fix that.

Thanks,
Ross

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH 4/7] multiboot2: Add PE load support

[Ross Lagerwall]

Add the ability to load multiboot binaries in PE format. This allows the
binaries to be signed and verified.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 grub-core/Makefile.core.def       |   1 +
 grub-core/loader/multiboot_mbi2.c |  15 +-
 grub-core/loader/multiboot_pe.c   | 694 ++++++++++++++++++++++++++++++
 include/grub/efi/pe32.h           |  64 +++
 include/grub/multiboot2.h         |   3 +
 5 files changed, 775 insertions(+), 2 deletions(-)
 create mode 100644 grub-core/loader/multiboot_pe.c

diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 1571421d7e84..34697ba58171 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -1815,6 +1815,7 @@ module = {
 
   common = loader/multiboot.c;
   common = loader/multiboot_mbi2.c;
+  common = loader/multiboot_pe.c;
   enable = x86;
   enable = i386_xen_pvh;
   enable = mips;
diff --git a/grub-core/loader/multiboot_mbi2.c b/grub-core/loader/multiboot_mbi2.c
index 6ef4f265070a..3ec96e2f29b9 100644
--- a/grub-core/loader/multiboot_mbi2.c
+++ b/grub-core/loader/multiboot_mbi2.c
@@ -389,8 +389,19 @@ grub_multiboot2_load (grub_file_t file, const char *filename)
 	}
       break;
     }
-  case MULTIBOOT_LOAD_TYPE_PE:
-      grub_fatal ("Unsupported load type: %u\n", mld.load_type);
+ case MULTIBOOT_LOAD_TYPE_PE:
+    {
+      mld.file = file;
+      mld.filename = filename;
+      mld.avoid_efi_boot_services = keep_bs;
+      err = grub_multiboot2_load_pe (&mld);
+      if (err)
+        {
+          grub_free (mld.buffer);
+          return err;
+        }
+      break;
+    }
   default:
     /* should be impossible */
     grub_fatal ("Unknown load type: %u\n", mld.load_type);
diff --git a/grub-core/loader/multiboot_pe.c b/grub-core/loader/multiboot_pe.c
new file mode 100644
index 000000000000..7f418348ac70
--- /dev/null
+++ b/grub-core/loader/multiboot_pe.c
@@ -0,0 +1,694 @@
+/*
+ * Significant portions of this code are derived from the Fedora GRUB patch
+ * "0007-Add-secureboot-support-on-efi-chainloader.patch" which is in turn
+ * derived from the PE loading code in Shim. The license is reproduced below:
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *
+ * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the
+ * distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
+ * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
+ * COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+ * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+ * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED
+ * OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * Copyright Peter Jones <pjones@redhat.com>
+ *
+ * Modifications:
+ *
+ * Copyright (c) 2024. Cloud Software Group, Inc.
+ */
+
+#include <grub/multiboot2.h>
+#include <grub/i386/relocator.h>
+#include <grub/efi/pe32.h>
+
+static int
+image_is_64_bit (grub_pe_header_t *pe_hdr)
+{
+  /* .Magic is the same offset in all cases */
+  return pe_hdr->pe32plus.optional_header.magic == GRUB_PE32_PE64_MAGIC;
+}
+
+static const grub_uint16_t machine_type __attribute__((__unused__)) =
+#if defined(__x86_64__)
+  GRUB_PE32_MACHINE_X86_64;
+#elif defined(__aarch64__)
+  GRUB_PE32_MACHINE_ARM64;
+#elif defined(__arm__)
+  GRUB_PE32_MACHINE_ARMTHUMB_MIXED;
+#elif defined(__i386__) || defined(__i486__) || defined(__i686__)
+  GRUB_PE32_MACHINE_I386;
+#elif defined(__ia64__)
+  GRUB_PE32_MACHINE_IA64;
+#elif defined(__riscv) && (__riscv_xlen == 32)
+  GRUB_PE32_MACHINE_RISCV32;
+#elif defined(__riscv) && (__riscv_xlen == 64)
+  GRUB_PE32_MACHINE_RISCV64;
+#else
+#error this architecture is not supported by grub2
+#endif
+
+static int
+image_is_loadable(grub_pe_header_t *pe_hdr)
+{
+  /*
+   * Check the machine type matches the binary and that we recognize
+   * the magic number.
+   */
+    return (pe_hdr->pe32.coff_header.machine == machine_type ||
+            (pe_hdr->pe32.coff_header.machine == GRUB_PE32_MACHINE_X86_64 &&
+             machine_type == GRUB_PE32_MACHINE_I386)) &&
+           (pe_hdr->pe32plus.optional_header.magic == GRUB_PE32_PE32_MAGIC ||
+            pe_hdr->pe32plus.optional_header.magic == GRUB_PE32_PE64_MAGIC);
+}
+
+/*
+ * Read the binary header and grab appropriate information from it
+ */
+static grub_err_t
+read_header(void *data, unsigned int datasize,
+            pe_coff_loader_image_context_t *context)
+{
+  grub_pe32_msdos_header_t *dos_hdr = data;
+  grub_pe_header_t *pe_hdr = data;
+  unsigned long header_without_data_dir, section_header_offset, opt_header_size;
+  unsigned long file_alignment = 0;
+
+  if (datasize < sizeof (pe_hdr->pe32))
+    {
+      grub_error (GRUB_ERR_OUT_OF_RANGE, "Invalid image");
+      return GRUB_ERR_OUT_OF_RANGE;
+    }
+
+  if (dos_hdr->e_magic == GRUB_PE32_MAGIC)
+    pe_hdr = (grub_pe_header_t *)((char *)data + dos_hdr->e_lfanew);
+
+  if (!image_is_loadable(pe_hdr))
+    {
+      grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET, "Platform does not support this image");
+      return GRUB_ERR_NOT_IMPLEMENTED_YET;
+    }
+
+  if (image_is_64_bit(pe_hdr))
+    {
+      context->number_of_rva_and_sizes = pe_hdr->pe32plus.optional_header.num_data_directories;
+      context->size_of_headers = pe_hdr->pe32plus.optional_header.header_size;
+      context->image_size = pe_hdr->pe32plus.optional_header.image_size;
+      context->section_alignment = pe_hdr->pe32plus.optional_header.section_alignment;
+      file_alignment = pe_hdr->pe32plus.optional_header.file_alignment;
+      opt_header_size = sizeof(struct grub_pe64_optional_header);
+    }
+  else
+    {
+      context->number_of_rva_and_sizes = pe_hdr->pe32.optional_header.num_data_directories;
+      context->size_of_headers = pe_hdr->pe32.optional_header.header_size;
+      context->image_size = (grub_uint64_t)pe_hdr->pe32.optional_header.image_size;
+      context->section_alignment = pe_hdr->pe32.optional_header.section_alignment;
+      file_alignment = pe_hdr->pe32.optional_header.file_alignment;
+      opt_header_size = sizeof(struct grub_pe32_optional_header);
+    }
+
+  if (file_alignment % 2 != 0)
+    {
+      grub_error (GRUB_ERR_OUT_OF_RANGE, "File Alignment is invalid (%ld)", file_alignment);
+      return GRUB_ERR_OUT_OF_RANGE;
+    }
+  if (file_alignment == 0)
+    file_alignment = 0x200;
+  if (context->section_alignment == 0)
+    context->section_alignment = GRUB_EFI_PAGE_SIZE;
+  if (context->section_alignment < file_alignment)
+    context->section_alignment = file_alignment;
+
+  context->number_of_sections = pe_hdr->pe32.coff_header.num_sections;
+
+  if (EFI_IMAGE_NUMBER_OF_DIRECTORY_ENTRIES < context->number_of_rva_and_sizes)
+    {
+      grub_error (GRUB_ERR_OUT_OF_RANGE, "Image header too small");
+      return GRUB_ERR_OUT_OF_RANGE;
+    }
+
+  header_without_data_dir = opt_header_size
+                  - sizeof (struct grub_pe32_data_directory) * EFI_IMAGE_NUMBER_OF_DIRECTORY_ENTRIES;
+  if (((grub_uint32_t)pe_hdr->pe32.coff_header.optional_header_size - header_without_data_dir) !=
+                  context->number_of_rva_and_sizes * sizeof (struct grub_pe32_data_directory))
+    {
+      grub_error (GRUB_ERR_OUT_OF_RANGE, "Image header overflows data directory");
+      return GRUB_ERR_OUT_OF_RANGE;
+    }
+
+  section_header_offset = dos_hdr->e_lfanew
+                          + sizeof (grub_uint32_t)
+                          + sizeof (struct grub_pe32_coff_header)
+                          + pe_hdr->pe32.coff_header.optional_header_size;
+  if (((grub_uint32_t)context->image_size - section_header_offset) / sizeof(struct grub_pe32_section_table)
+                  <= context->number_of_sections)
+    {
+      grub_error (GRUB_ERR_OUT_OF_RANGE, "Image sections overflow image size");
+      return GRUB_ERR_OUT_OF_RANGE;
+    }
+
+  if ((context->size_of_headers - section_header_offset) / sizeof(struct grub_pe32_section_table)
+                  < (grub_uint32_t)context->number_of_sections)
+    {
+      grub_error (GRUB_ERR_OUT_OF_RANGE, "Image sections overflow section headers");
+      return GRUB_ERR_OUT_OF_RANGE;
+    }
+
+  if ((((grub_uint8_t *)pe_hdr - (grub_uint8_t *)data) + sizeof(grub_pe_header_t)) > datasize)
+    {
+      grub_error (GRUB_ERR_OUT_OF_RANGE, "Invalid image");
+      return GRUB_ERR_OUT_OF_RANGE;
+    }
+
+  if (pe_hdr->pe32.signature[0] != 'P' ||
+              pe_hdr->pe32.signature[1] != 'E' ||
+              pe_hdr->pe32.signature[2] != 0x00 ||
+              pe_hdr->pe32.signature[3] != 0x00)
+    {
+      grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET, "Unsupported image type");
+      return GRUB_ERR_NOT_IMPLEMENTED_YET;
+    }
+
+  context->pe_hdr = pe_hdr;
+
+  if (image_is_64_bit(pe_hdr))
+    {
+      context->image_address = pe_hdr->pe32plus.optional_header.image_base;
+      context->entry_point = pe_hdr->pe32plus.optional_header.entry_addr;
+      context->reloc_dir = &pe_hdr->pe32plus.optional_header.base_relocation_table;
+      context->sec_dir = &pe_hdr->pe32plus.optional_header.certificate_table;
+    }
+  else
+    {
+      context->image_address = pe_hdr->pe32.optional_header.image_base;
+      context->entry_point = pe_hdr->pe32.optional_header.entry_addr;
+      context->reloc_dir = &pe_hdr->pe32.optional_header.base_relocation_table;
+      context->sec_dir = &pe_hdr->pe32.optional_header.certificate_table;
+    }
+
+  context->first_section = (struct grub_pe32_section_table *)((char *)pe_hdr +
+          pe_hdr->pe32.coff_header.optional_header_size +
+          sizeof(grub_uint32_t) + sizeof(struct grub_pe32_coff_header));
+
+  if (context->image_size < context->size_of_headers)
+    {
+      grub_error (GRUB_ERR_OUT_OF_RANGE, "Invalid image");
+      return GRUB_ERR_OUT_OF_RANGE;
+    }
+
+  if ((unsigned long)((grub_uint8_t *)context->sec_dir - (grub_uint8_t *)data) >
+      (datasize - sizeof(struct grub_pe32_data_directory)))
+    {
+      grub_error (GRUB_ERR_OUT_OF_RANGE, "Invalid image");
+      return GRUB_ERR_OUT_OF_RANGE;
+    }
+  return GRUB_ERR_NONE;
+}
+
+static grub_phys_addr_t
+image_address (grub_phys_addr_t image, grub_uint64_t sz, grub_uint64_t addr)
+{
+  if (addr > sz)
+    return 0;
+
+  return image + addr;
+}
+
+static grub_err_t
+relocate_coff (pe_coff_loader_image_context_t *context,
+               struct grub_pe32_section_table *section,
+               grub_phys_addr_t phys_addr, grub_uint8_t *virt_addr,
+               mbi_load_data_t *mld)
+{
+  struct grub_pe32_data_directory *reloc_base, *reloc_base_end;
+  grub_uint8_t *reloc_buffer;
+  grub_uint64_t adjust;
+  struct grub_pe32_fixup_block *reloc, *reloc_end;
+  grub_uint8_t *fixup, *fixup_base;
+  grub_uint16_t *fixup_16;
+  grub_uint32_t *fixup_32;
+  grub_uint64_t *fixup_64;
+  int n = 0;
+  grub_err_t ret = GRUB_ERR_NONE;
+
+  if (image_is_64_bit (context->pe_hdr))
+    context->pe_hdr->pe32plus.optional_header.image_base =
+      (grub_uint64_t)(unsigned long)phys_addr;
+  else
+    context->pe_hdr->pe32.optional_header.image_base =
+      (grub_uint32_t)(unsigned long)phys_addr;
+
+  /* Alright, so here's how this works:
+   *
+   * context->reloc_dir gives us two things:
+   * - the VA the table of base relocation blocks are (maybe) to be
+   *   mapped at (reloc_dir->rva)
+   * - the virtual size (reloc_dir->size)
+   *
+   * The .reloc section (section here) gives us some other things:
+   * - the name! kind of. (section->name)
+   * - the virtual size (section->virtual_size), which should be the same
+   *   as RelocDir->Size
+   * - the virtual address (section->virtual_address)
+   * - the file section size (section->raw_data_size), which is
+   *   a multiple of optional_header->file_alignment.  Only useful for image
+   *   validation, not really useful for iteration bounds.
+   * - the file address (section->raw_data_offset)
+   * - a bunch of stuff we don't use that's 0 in our binaries usually
+   * - Flags (section->characteristics)
+   *
+   * and then the thing that's actually at the file address is an array
+   * of struct grub_pe32_fixup_block structs with some values packed behind
+   * them.  The block_size field of this structure includes the
+   * structure itself, and adding it to that structure's address will
+   * yield the next entry in the array.
+   */
+
+  reloc_buffer = grub_malloc (section->virtual_size);
+  if (!reloc_buffer)
+      return grub_errno;
+
+  if (grub_file_seek (mld->file, section->raw_data_offset) == (grub_off_t) -1)
+    {
+      ret = grub_errno;
+      goto out;
+    }
+
+  if (grub_file_read (mld->file, reloc_buffer, section->virtual_size)
+      != (grub_ssize_t) section->virtual_size)
+    {
+      if (!grub_errno)
+        {
+          grub_error (GRUB_ERR_FILE_READ_ERROR, "premature end of file %s",
+                      mld->filename);
+          ret = GRUB_ERR_FILE_READ_ERROR;
+        }
+      else
+          ret = grub_errno;
+      goto out;
+    }
+
+  reloc_base = (struct grub_pe32_data_directory *)reloc_buffer;
+  reloc_base_end = (struct grub_pe32_data_directory *)(reloc_buffer + section->virtual_size);
+
+  grub_dprintf ("multiboot_loader", "relocate_coff(): reloc_base %p reloc_base_end %p\n",
+                reloc_base, reloc_base_end);
+
+  if (!reloc_base && !reloc_base_end)
+    return GRUB_ERR_NONE;
+
+  if (!reloc_base || !reloc_base_end)
+    {
+      grub_error (GRUB_ERR_BAD_ARGUMENT, "Reloc table overflows binary");
+      return GRUB_ERR_BAD_ARGUMENT;
+    }
+
+  adjust = (grub_uint64_t)(grub_efi_uintn_t)phys_addr - context->image_address;
+  if (adjust == 0)
+    return GRUB_ERR_NONE;
+
+  while (reloc_base < reloc_base_end)
+    {
+      grub_uint16_t *entry;
+      reloc = (struct grub_pe32_fixup_block *)((char*)reloc_base);
+
+      if ((reloc_base->size == 0) ||
+          (reloc_base->size > context->reloc_dir->size))
+        {
+          grub_error (GRUB_ERR_BAD_ARGUMENT,
+                      "Reloc %d block size %d is invalid\n", n,
+                      reloc_base->size);
+          ret = GRUB_ERR_BAD_ARGUMENT;
+          goto out;
+        }
+
+      entry = &reloc->entries[0];
+      reloc_end = (struct grub_pe32_fixup_block *)
+        ((char *)reloc_base + reloc_base->size);
+
+      if ((grub_uint8_t *)reloc_end < reloc_buffer ||
+          (grub_uint8_t *)reloc_end > (reloc_buffer + section->virtual_size))
+        {
+          grub_error (GRUB_ERR_BAD_ARGUMENT, "Reloc entry %d overflows binary",
+                      n);
+          return GRUB_ERR_BAD_ARGUMENT;
+        }
+
+      fixup_base = virt_addr + reloc_base->rva;
+      if (!fixup_base)
+        {
+          grub_error (GRUB_ERR_BAD_ARGUMENT, "Reloc %d Invalid fixupbase", n);
+          ret = GRUB_ERR_BAD_ARGUMENT;
+          goto out;
+        }
+
+      while ((void *)entry < (void *)reloc_end)
+        {
+          fixup = fixup_base + (*entry & 0xFFF);
+          switch ((*entry) >> 12)
+            {
+              case GRUB_PE32_REL_BASED_ABSOLUTE:
+                break;
+              case GRUB_PE32_REL_BASED_HIGH:
+                fixup_16 = (grub_uint16_t *)fixup;
+                *fixup_16 = (grub_uint16_t)
+                  (*fixup_16 + ((grub_uint16_t)((grub_uint32_t)adjust >> 16)));
+                break;
+              case GRUB_PE32_REL_BASED_LOW:
+                fixup_16 = (grub_uint16_t *)fixup;
+                *fixup_16 = (grub_uint16_t) (*fixup_16 + (grub_uint16_t)adjust);
+                break;
+              case GRUB_PE32_REL_BASED_HIGHLOW:
+                fixup_32 = (grub_uint32_t *)fixup;
+                *fixup_32 = *fixup_32 + (grub_uint32_t)adjust;
+                break;
+              case GRUB_PE32_REL_BASED_DIR64:
+                fixup_64 = (grub_uint64_t *)fixup;
+                *fixup_64 = *fixup_64 + (grub_uint64_t)adjust;
+                break;
+              default:
+                grub_error (GRUB_ERR_BAD_ARGUMENT,
+                            "Reloc %d unknown relocation type %d",
+                            n, (*entry) >> 12);
+                ret = GRUB_ERR_BAD_ARGUMENT;
+                goto out;
+            }
+          entry += 1;
+        }
+      reloc_base = (struct grub_pe32_data_directory *)reloc_end;
+      n++;
+    }
+
+out:
+  grub_free(reloc_buffer);
+
+  return ret;
+}
+
+grub_err_t
+grub_multiboot2_load_pe (mbi_load_data_t *mld)
+{
+  int i;
+  grub_uint8_t *virt_addr;
+  grub_phys_addr_t phys_addr, reloc_base, reloc_base_end, base, end;
+  struct grub_pe32_section_table *reloc_section = NULL, fake_reloc_section;
+  struct grub_pe32_section_table *section;
+  grub_relocator_chunk_t ch;
+  pe_coff_loader_image_context_t context;
+  grub_err_t ret = GRUB_ERR_NONE;
+  grub_uint32_t section_alignment;
+  int found_entry_point = 0;
+
+  ret = read_header(mld->buffer, MULTIBOOT_SEARCH, &context);
+  if (ret)
+      return ret;
+
+  /*
+   * The spec says, uselessly, of SectionAlignment:
+   * =====
+   * The alignment (in bytes) of sections when they are loaded into
+   * memory. It must be greater than or equal to FileAlignment. The
+   * default is the page size for the architecture.
+   * =====
+   * Which doesn't tell you whose responsibility it is to enforce the
+   * "default", or when.  It implies that the value in the field must
+   * be > FileAlignment (also poorly defined), but it appears visual
+   * studio will happily write 512 for FileAlignment (its default) and
+   * 0 for SectionAlignment, intending to imply PAGE_SIZE.
+   *
+   * We only support one page size, so if it's zero, nerf it to 4096.
+   */
+  section_alignment = context.section_alignment;
+  if (section_alignment == 0)
+    section_alignment = 4096;
+
+  section_alignment = grub_max(section_alignment, mld->align);
+
+  ret = grub_relocator_alloc_chunk_align_safe (grub_multiboot2_relocator, &ch,
+                                               mld->min_addr, mld->max_addr,
+                                               context.image_size, section_alignment,
+                                               mld->preference, mld->avoid_efi_boot_services);
+
+  if (ret)
+    {
+      grub_dprintf ("multiboot_loader", "Cannot allocate memory for OS image\n");
+      return ret;
+    }
+
+  virt_addr = get_virtual_current_address (ch);
+  phys_addr = get_physical_target_address (ch);
+
+  if (grub_file_seek (mld->file, 0) == (grub_off_t) -1) {
+      ret = grub_errno;
+      goto out;
+  }
+
+  if (grub_file_read (mld->file, virt_addr, context.size_of_headers)
+      != (grub_ssize_t) context.size_of_headers)
+    {
+      if (!grub_errno)
+        {
+          grub_error (GRUB_ERR_FILE_READ_ERROR, "premature end of file %s",
+                      mld->filename);
+          ret = GRUB_ERR_FILE_READ_ERROR;
+        }
+      else
+          ret = grub_errno;
+      goto out;
+    }
+
+  mld->load_base_addr = phys_addr;
+  mld->link_base_addr = context.image_address;
+
+  grub_dprintf ("multiboot_loader", "load_base_addr: 0x%08x link_base_addr 0x%08x\n",
+                mld->load_base_addr, mld->link_base_addr);
+
+  grub_multiboot2_payload_eip = context.entry_point;
+  grub_dprintf ("multiboot_loader", "entry_point: 0x%08x\n", grub_multiboot2_payload_eip);
+  if (!grub_multiboot2_payload_eip)
+    {
+      grub_error (GRUB_ERR_BAD_ARGUMENT, "invalid entry point");
+      ret = GRUB_ERR_BAD_ARGUMENT;
+      goto out;
+    }
+
+  grub_dprintf ("multiboot_loader", "reloc_dir: %p reloc_size: 0x%08x\n",
+                (void *)(unsigned long)context.reloc_dir->rva,
+                context.reloc_dir->size);
+  reloc_base = image_address (phys_addr, context.image_size,
+                              context.reloc_dir->rva);
+  /* RelocBaseEnd here is the address of the last byte of the table */
+  reloc_base_end = image_address (phys_addr, context.image_size,
+                                  context.reloc_dir->rva
+                                  + context.reloc_dir->size - 1);
+  grub_dprintf ("multiboot_loader", "reloc_base: 0x%016lx reloc_base_end: 0x%016lx\n",
+                reloc_base, reloc_base_end);
+
+  section = context.first_section;
+  for (i = 0; i < context.number_of_sections; i++, section++)
+    {
+      char name[9];
+
+      base = image_address (phys_addr, context.image_size,
+                            section->virtual_address);
+      end = image_address (phys_addr, context.image_size,
+                           section->virtual_address + section->virtual_size -1);
+
+      grub_strncpy(name, section->name, 9);
+      name[8] = '\0';
+      grub_dprintf ("multiboot_loader", "Section %d \"%s\" at 0x%016lx..0x%016lx\n", i,
+                   name, base, end);
+
+      if (end < base)
+        {
+          grub_dprintf ("multiboot_loader", " base is 0x%016lx but end is 0x%016lx... bad.\n",
+                       base, end);
+          grub_error (GRUB_ERR_BAD_ARGUMENT,
+                      "Image has invalid negative size");
+          ret = GRUB_ERR_BAD_ARGUMENT;
+          goto out;
+        }
+
+      if (section->virtual_address <= context.entry_point &&
+          (section->virtual_address + section->raw_data_size - 1)
+          > context.entry_point)
+        {
+          found_entry_point++;
+          grub_dprintf ("multiboot_loader", " section contains entry point\n");
+        }
+
+      /* We do want to process .reloc, but it's often marked
+       * discardable, so we don't want to memcpy it. */
+      if (grub_memcmp (section->name, ".reloc\0\0", 8) == 0)
+        {
+          if (reloc_section)
+            {
+              grub_error (GRUB_ERR_BAD_ARGUMENT,
+                          "Image has multiple relocation sections");
+              ret = GRUB_ERR_BAD_ARGUMENT;
+              goto out;
+            }
+
+          /* If it has nonzero sizes, and our bounds check
+           * made sense, and the VA and size match RelocDir's
+           * versions, then we believe in this section table. */
+          if (section->raw_data_size && section->virtual_size &&
+              base && end && reloc_base == base)
+            {
+              if (reloc_base_end == end)
+                {
+                  grub_dprintf ("multiboot_loader", " section is relocation section\n");
+                  reloc_section = section;
+                }
+              else if (reloc_base_end && reloc_base_end < end)
+                {
+                  /* Bogus virtual size in the reloc section -- RelocDir
+                   * reported a smaller Base Relocation Directory. Decrease
+                   * the section's virtual size so that it equal RelocDir's
+                   * idea, but only for the purposes of relocate_coff(). */
+                  grub_dprintf ("multiboot_loader",
+                                " section is (overlong) relocation section\n");
+                  grub_memcpy (&fake_reloc_section, section, sizeof *section);
+                  fake_reloc_section.virtual_size -= (end - reloc_base_end);
+                  reloc_section = &fake_reloc_section;
+                }
+            }
+
+          if (!reloc_section)
+            {
+              grub_dprintf ("multiboot_loader", " section is not reloc section?\n");
+              grub_dprintf ("multiboot_loader", " rds: 0x%08x, vs: %08x\n",
+                            section->raw_data_size, section->virtual_size);
+              grub_dprintf ("multiboot_loader", " base: 0x%016lx end: 0x%016lx\n", base, end);
+              grub_dprintf ("multiboot_loader", " reloc_base: 0x%016lx reloc_base_end: 0x%016lx\n",
+                            reloc_base, reloc_base_end);
+            }
+        }
+
+      grub_dprintf ("multiboot_loader", " Section characteristics are %08x\n",
+                   section->characteristics);
+      grub_dprintf ("multiboot_loader", " Section virtual size: %08x\n",
+                   section->virtual_size);
+      grub_dprintf ("multiboot_loader", " Section raw_data size: %08x\n",
+                   section->raw_data_size);
+      if (section->characteristics & GRUB_PE32_SCN_MEM_DISCARDABLE)
+        {
+          grub_dprintf ("multiboot_loader", " Discarding section\n");
+          continue;
+        }
+
+      if (!base || !end)
+        {
+          grub_dprintf ("multiboot_loader", " section is invalid\n");
+          grub_error (GRUB_ERR_BAD_ARGUMENT, "Invalid section size");
+          ret = GRUB_ERR_BAD_ARGUMENT;
+          goto out;
+        }
+
+      if (section->characteristics & GRUB_PE32_SCN_CNT_UNINITIALIZED_DATA)
+        {
+          if (section->raw_data_size != 0)
+            grub_dprintf ("multiboot_loader", " UNINITIALIZED_DATA section has data?\n");
+        }
+      else if (section->virtual_address < context.size_of_headers ||
+               section->raw_data_offset < context.size_of_headers)
+        {
+          grub_error (GRUB_ERR_BAD_ARGUMENT,
+                      "Section %d is inside image headers", i);
+          ret = GRUB_ERR_BAD_ARGUMENT;
+          goto out;
+        }
+
+      if (section->raw_data_size > 0)
+        {
+          grub_dprintf ("multiboot_loader", " copying 0x%08x bytes to 0x%016lx\n",
+                        section->raw_data_size, base);
+
+          if (grub_file_seek (mld->file, section->raw_data_offset) == (grub_off_t) -1)
+            {
+              ret = grub_errno;
+              goto out;
+            }
+
+          if (grub_file_read (mld->file, virt_addr + (base - phys_addr), section->raw_data_size)
+              != (grub_ssize_t) section->raw_data_size)
+            {
+              if (!grub_errno)
+                {
+                  grub_error (GRUB_ERR_FILE_READ_ERROR, "premature end of file %s",
+                              mld->filename);
+                  ret = GRUB_ERR_FILE_READ_ERROR;
+                }
+              else
+                  ret = grub_errno;
+              goto out;
+            }
+        }
+
+      if (section->raw_data_size < section->virtual_size)
+        {
+          grub_dprintf ("multiboot_loader", " padding with 0x%08x bytes at 0x%016lx\n",
+                        section->virtual_size - section->raw_data_size,
+                        base + section->raw_data_size);
+          grub_memset (virt_addr + (base - phys_addr) + section->raw_data_size, 0,
+                       section->virtual_size - section->raw_data_size);
+        }
+
+      grub_dprintf ("multiboot_loader", " finished section %s\n", name);
+    }
+
+  /* 5 == EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC */
+  if (context.number_of_rva_and_sizes <= 5)
+    {
+      grub_error (GRUB_ERR_BAD_ARGUMENT, "image has no relocation entry\n");
+      ret = GRUB_ERR_BAD_ARGUMENT;
+      goto out;
+    }
+
+  if (context.reloc_dir->size && reloc_section)
+    {
+      /* run the relocation fixups */
+      ret = relocate_coff (&context, reloc_section, phys_addr, virt_addr, mld);
+
+      if (ret)
+        {
+          grub_error (GRUB_ERR_BAD_ARGUMENT, "relocation failed");
+          goto out;
+        }
+    }
+
+  if (!found_entry_point)
+    {
+      grub_error (GRUB_ERR_BAD_ARGUMENT, "entry point is not within sections");
+      ret = GRUB_ERR_BAD_ARGUMENT;
+      goto out;
+    }
+  if (found_entry_point > 1)
+    {
+      grub_error (GRUB_ERR_BAD_ARGUMENT, "%d sections contain entry point",
+                  found_entry_point);
+      ret = GRUB_ERR_BAD_ARGUMENT;
+      goto out;
+    }
+
+out:
+  return ret;
+}
diff --git a/include/grub/efi/pe32.h b/include/grub/efi/pe32.h
index 4e6e9d254bd3..94435d758b46 100644
--- a/include/grub/efi/pe32.h
+++ b/include/grub/efi/pe32.h
@@ -20,6 +20,7 @@
 #define GRUB_EFI_PE32_HEADER	1
 
 #include <grub/types.h>
+#include <grub/efi/api.h>
 #include <grub/efi/memory.h>
 
 /* The MSDOS compatibility stub. This was copied from the output of
@@ -46,6 +47,28 @@
 
 #define GRUB_PE32_MSDOS_STUB_SIZE	0x80
 
+typedef struct {
+  grub_uint16_t  e_magic;    ///< Magic number.
+  grub_uint16_t  e_cblp;     ///< Bytes on last page of file.
+  grub_uint16_t  e_cp;       ///< Pages in file.
+  grub_uint16_t  e_crlc;     ///< Relocations.
+  grub_uint16_t  e_cparhdr;  ///< Size of header in paragraphs.
+  grub_uint16_t  e_minalloc; ///< Minimum extra paragraphs needed.
+  grub_uint16_t  e_maxalloc; ///< Maximum extra paragraphs needed.
+  grub_uint16_t  e_ss;       ///< Initial (relative) SS value.
+  grub_uint16_t  e_sp;       ///< Initial SP value.
+  grub_uint16_t  e_csum;     ///< Checksum.
+  grub_uint16_t  e_ip;       ///< Initial IP value.
+  grub_uint16_t  e_cs;       ///< Initial (relative) CS value.
+  grub_uint16_t  e_lfarlc;   ///< File address of relocation table.
+  grub_uint16_t  e_ovno;     ///< Overlay number.
+  grub_uint16_t  e_res[4];   ///< Reserved words.
+  grub_uint16_t  e_oemid;    ///< OEM identifier (for e_oeminfo).
+  grub_uint16_t  e_oeminfo;  ///< OEM information; e_oemid specific.
+  grub_uint16_t  e_res2[10]; ///< Reserved words.
+  grub_uint32_t  e_lfanew;   ///< File address of new exe header.
+} grub_pe32_msdos_header_t;
+
 #define GRUB_PE32_MAGIC			0x5a4d
 
 struct grub_msdos_image_header
@@ -249,6 +272,7 @@ struct grub_pe32_section_table
 
 #define GRUB_PE32_SCN_CNT_CODE			0x00000020
 #define GRUB_PE32_SCN_CNT_INITIALIZED_DATA	0x00000040
+#define GRUB_PE32_SCN_CNT_UNINITIALIZED_DATA    0x00000080
 #define GRUB_PE32_SCN_MEM_DISCARDABLE		0x02000000
 #define GRUB_PE32_SCN_MEM_EXECUTE		0x20000000
 #define GRUB_PE32_SCN_MEM_READ			0x40000000
@@ -349,4 +373,44 @@ struct grub_pe32_reloc
 #define GRUB_PE32_REL_I386_DIR32	0x6
 #define GRUB_PE32_REL_I386_REL32	0x14
 
+struct grub_pe32_header_32
+{
+  char signature[GRUB_PE32_SIGNATURE_SIZE];
+  struct grub_pe32_coff_header coff_header;
+  struct grub_pe32_optional_header optional_header;
+};
+
+struct grub_pe32_header_64
+{
+  char signature[GRUB_PE32_SIGNATURE_SIZE];
+  struct grub_pe32_coff_header coff_header;
+  struct grub_pe64_optional_header optional_header;
+};
+
+typedef union
+{
+  struct grub_pe32_header_32 pe32;
+  struct grub_pe32_header_64 pe32plus;
+} grub_pe_header_t;
+
+struct pe_coff_loader_image_context
+{
+  grub_efi_uint64_t image_address;
+  grub_efi_uint64_t image_size;
+  grub_efi_uint64_t entry_point;
+  grub_efi_uintn_t size_of_headers;
+  grub_efi_uint16_t image_type;
+  grub_efi_uint16_t number_of_sections;
+  grub_efi_uint32_t section_alignment;
+  struct grub_pe32_section_table *first_section;
+  struct grub_pe32_data_directory *reloc_dir;
+  struct grub_pe32_data_directory *sec_dir;
+  grub_efi_uint64_t number_of_rva_and_sizes;
+  grub_pe_header_t *pe_hdr;
+};
+
+typedef struct pe_coff_loader_image_context pe_coff_loader_image_context_t;
+
+#define EFI_IMAGE_NUMBER_OF_DIRECTORY_ENTRIES 16
+
 #endif /* ! GRUB_EFI_PE32_HEADER */
diff --git a/include/grub/multiboot2.h b/include/grub/multiboot2.h
index 1f6d4fc9e336..01e2b11755c2 100644
--- a/include/grub/multiboot2.h
+++ b/include/grub/multiboot2.h
@@ -97,6 +97,9 @@ typedef struct mbi_load_data mbi_load_data_t;
 grub_err_t
 grub_multiboot2_load_elf (mbi_load_data_t *mld);
 
+grub_err_t
+grub_multiboot2_load_pe (mbi_load_data_t *mld);
+
 extern grub_size_t grub_multiboot2_pure_size;
 extern grub_size_t grub_multiboot2_alloc_mbi;
 extern grub_uint32_t grub_multiboot2_payload_eip;
-- 
2.43.0

[PATCH 4/7] multiboot2: Add PE load support

[Ross Lagerwall via Grub-devel]

Add the ability to load multiboot binaries in PE format. This allows the
binaries to be signed and verified.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 grub-core/Makefile.core.def       |   1 +
 grub-core/loader/multiboot_mbi2.c |  15 +-
 grub-core/loader/multiboot_pe.c   | 694 ++++++++++++++++++++++++++++++
 include/grub/efi/pe32.h           |  64 +++
 include/grub/multiboot2.h         |   3 +
 5 files changed, 775 insertions(+), 2 deletions(-)
 create mode 100644 grub-core/loader/multiboot_pe.c

diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 1571421d7e84..34697ba58171 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -1815,6 +1815,7 @@ module = {
 
   common = loader/multiboot.c;
   common = loader/multiboot_mbi2.c;
+  common = loader/multiboot_pe.c;
   enable = x86;
   enable = i386_xen_pvh;
   enable = mips;
diff --git a/grub-core/loader/multiboot_mbi2.c b/grub-core/loader/multiboot_mbi2.c
index 6ef4f265070a..3ec96e2f29b9 100644
--- a/grub-core/loader/multiboot_mbi2.c
+++ b/grub-core/loader/multiboot_mbi2.c
@@ -389,8 +389,19 @@ grub_multiboot2_load (grub_file_t file, const char *filename)
 	}
       break;
     }
-  case MULTIBOOT_LOAD_TYPE_PE:
-      grub_fatal ("Unsupported load type: %u\n", mld.load_type);
+ case MULTIBOOT_LOAD_TYPE_PE:
+    {
+      mld.file = file;
+      mld.filename = filename;
+      mld.avoid_efi_boot_services = keep_bs;
+      err = grub_multiboot2_load_pe (&mld);
+      if (err)
+        {
+          grub_free (mld.buffer);
+          return err;
+        }
+      break;
+    }
   default:
     /* should be impossible */
     grub_fatal ("Unknown load type: %u\n", mld.load_type);
diff --git a/grub-core/loader/multiboot_pe.c b/grub-core/loader/multiboot_pe.c
new file mode 100644
index 000000000000..7f418348ac70
--- /dev/null
+++ b/grub-core/loader/multiboot_pe.c
@@ -0,0 +1,694 @@
+/*
+ * Significant portions of this code are derived from the Fedora GRUB patch
+ * "0007-Add-secureboot-support-on-efi-chainloader.patch" which is in turn
+ * derived from the PE loading code in Shim. The license is reproduced below:
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *
+ * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the
+ * distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
+ * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
+ * COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+ * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+ * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED
+ * OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * Copyright Peter Jones <pjones@redhat.com>
+ *
+ * Modifications:
+ *
+ * Copyright (c) 2024. Cloud Software Group, Inc.
+ */
+
+#include <grub/multiboot2.h>
+#include <grub/i386/relocator.h>
+#include <grub/efi/pe32.h>
+
+static int
+image_is_64_bit (grub_pe_header_t *pe_hdr)
+{
+  /* .Magic is the same offset in all cases */
+  return pe_hdr->pe32plus.optional_header.magic == GRUB_PE32_PE64_MAGIC;
+}
+
+static const grub_uint16_t machine_type __attribute__((__unused__)) =
+#if defined(__x86_64__)
+  GRUB_PE32_MACHINE_X86_64;
+#elif defined(__aarch64__)
+  GRUB_PE32_MACHINE_ARM64;
+#elif defined(__arm__)
+  GRUB_PE32_MACHINE_ARMTHUMB_MIXED;
+#elif defined(__i386__) || defined(__i486__) || defined(__i686__)
+  GRUB_PE32_MACHINE_I386;
+#elif defined(__ia64__)
+  GRUB_PE32_MACHINE_IA64;
+#elif defined(__riscv) && (__riscv_xlen == 32)
+  GRUB_PE32_MACHINE_RISCV32;
+#elif defined(__riscv) && (__riscv_xlen == 64)
+  GRUB_PE32_MACHINE_RISCV64;
+#else
+#error this architecture is not supported by grub2
+#endif
+
+static int
+image_is_loadable(grub_pe_header_t *pe_hdr)
+{
+  /*
+   * Check the machine type matches the binary and that we recognize
+   * the magic number.
+   */
+    return (pe_hdr->pe32.coff_header.machine == machine_type ||
+            (pe_hdr->pe32.coff_header.machine == GRUB_PE32_MACHINE_X86_64 &&
+             machine_type == GRUB_PE32_MACHINE_I386)) &&
+           (pe_hdr->pe32plus.optional_header.magic == GRUB_PE32_PE32_MAGIC ||
+            pe_hdr->pe32plus.optional_header.magic == GRUB_PE32_PE64_MAGIC);
+}
+
+/*
+ * Read the binary header and grab appropriate information from it
+ */
+static grub_err_t
+read_header(void *data, unsigned int datasize,
+            pe_coff_loader_image_context_t *context)
+{
+  grub_pe32_msdos_header_t *dos_hdr = data;
+  grub_pe_header_t *pe_hdr = data;
+  unsigned long header_without_data_dir, section_header_offset, opt_header_size;
+  unsigned long file_alignment = 0;
+
+  if (datasize < sizeof (pe_hdr->pe32))
+    {
+      grub_error (GRUB_ERR_OUT_OF_RANGE, "Invalid image");
+      return GRUB_ERR_OUT_OF_RANGE;
+    }
+
+  if (dos_hdr->e_magic == GRUB_PE32_MAGIC)
+    pe_hdr = (grub_pe_header_t *)((char *)data + dos_hdr->e_lfanew);
+
+  if (!image_is_loadable(pe_hdr))
+    {
+      grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET, "Platform does not support this image");
+      return GRUB_ERR_NOT_IMPLEMENTED_YET;
+    }
+
+  if (image_is_64_bit(pe_hdr))
+    {
+      context->number_of_rva_and_sizes = pe_hdr->pe32plus.optional_header.num_data_directories;
+      context->size_of_headers = pe_hdr->pe32plus.optional_header.header_size;
+      context->image_size = pe_hdr->pe32plus.optional_header.image_size;
+      context->section_alignment = pe_hdr->pe32plus.optional_header.section_alignment;
+      file_alignment = pe_hdr->pe32plus.optional_header.file_alignment;
+      opt_header_size = sizeof(struct grub_pe64_optional_header);
+    }
+  else
+    {
+      context->number_of_rva_and_sizes = pe_hdr->pe32.optional_header.num_data_directories;
+      context->size_of_headers = pe_hdr->pe32.optional_header.header_size;
+      context->image_size = (grub_uint64_t)pe_hdr->pe32.optional_header.image_size;
+      context->section_alignment = pe_hdr->pe32.optional_header.section_alignment;
+      file_alignment = pe_hdr->pe32.optional_header.file_alignment;
+      opt_header_size = sizeof(struct grub_pe32_optional_header);
+    }
+
+  if (file_alignment % 2 != 0)
+    {
+      grub_error (GRUB_ERR_OUT_OF_RANGE, "File Alignment is invalid (%ld)", file_alignment);
+      return GRUB_ERR_OUT_OF_RANGE;
+    }
+  if (file_alignment == 0)
+    file_alignment = 0x200;
+  if (context->section_alignment == 0)
+    context->section_alignment = GRUB_EFI_PAGE_SIZE;
+  if (context->section_alignment < file_alignment)
+    context->section_alignment = file_alignment;
+
+  context->number_of_sections = pe_hdr->pe32.coff_header.num_sections;
+
+  if (EFI_IMAGE_NUMBER_OF_DIRECTORY_ENTRIES < context->number_of_rva_and_sizes)
+    {
+      grub_error (GRUB_ERR_OUT_OF_RANGE, "Image header too small");
+      return GRUB_ERR_OUT_OF_RANGE;
+    }
+
+  header_without_data_dir = opt_header_size
+                  - sizeof (struct grub_pe32_data_directory) * EFI_IMAGE_NUMBER_OF_DIRECTORY_ENTRIES;
+  if (((grub_uint32_t)pe_hdr->pe32.coff_header.optional_header_size - header_without_data_dir) !=
+                  context->number_of_rva_and_sizes * sizeof (struct grub_pe32_data_directory))
+    {
+      grub_error (GRUB_ERR_OUT_OF_RANGE, "Image header overflows data directory");
+      return GRUB_ERR_OUT_OF_RANGE;
+    }
+
+  section_header_offset = dos_hdr->e_lfanew
+                          + sizeof (grub_uint32_t)
+                          + sizeof (struct grub_pe32_coff_header)
+                          + pe_hdr->pe32.coff_header.optional_header_size;
+  if (((grub_uint32_t)context->image_size - section_header_offset) / sizeof(struct grub_pe32_section_table)
+                  <= context->number_of_sections)
+    {
+      grub_error (GRUB_ERR_OUT_OF_RANGE, "Image sections overflow image size");
+      return GRUB_ERR_OUT_OF_RANGE;
+    }
+
+  if ((context->size_of_headers - section_header_offset) / sizeof(struct grub_pe32_section_table)
+                  < (grub_uint32_t)context->number_of_sections)
+    {
+      grub_error (GRUB_ERR_OUT_OF_RANGE, "Image sections overflow section headers");
+      return GRUB_ERR_OUT_OF_RANGE;
+    }
+
+  if ((((grub_uint8_t *)pe_hdr - (grub_uint8_t *)data) + sizeof(grub_pe_header_t)) > datasize)
+    {
+      grub_error (GRUB_ERR_OUT_OF_RANGE, "Invalid image");
+      return GRUB_ERR_OUT_OF_RANGE;
+    }
+
+  if (pe_hdr->pe32.signature[0] != 'P' ||
+              pe_hdr->pe32.signature[1] != 'E' ||
+              pe_hdr->pe32.signature[2] != 0x00 ||
+              pe_hdr->pe32.signature[3] != 0x00)
+    {
+      grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET, "Unsupported image type");
+      return GRUB_ERR_NOT_IMPLEMENTED_YET;
+    }
+
+  context->pe_hdr = pe_hdr;
+
+  if (image_is_64_bit(pe_hdr))
+    {
+      context->image_address = pe_hdr->pe32plus.optional_header.image_base;
+      context->entry_point = pe_hdr->pe32plus.optional_header.entry_addr;
+      context->reloc_dir = &pe_hdr->pe32plus.optional_header.base_relocation_table;
+      context->sec_dir = &pe_hdr->pe32plus.optional_header.certificate_table;
+    }
+  else
+    {
+      context->image_address = pe_hdr->pe32.optional_header.image_base;
+      context->entry_point = pe_hdr->pe32.optional_header.entry_addr;
+      context->reloc_dir = &pe_hdr->pe32.optional_header.base_relocation_table;
+      context->sec_dir = &pe_hdr->pe32.optional_header.certificate_table;
+    }
+
+  context->first_section = (struct grub_pe32_section_table *)((char *)pe_hdr +
+          pe_hdr->pe32.coff_header.optional_header_size +
+          sizeof(grub_uint32_t) + sizeof(struct grub_pe32_coff_header));
+
+  if (context->image_size < context->size_of_headers)
+    {
+      grub_error (GRUB_ERR_OUT_OF_RANGE, "Invalid image");
+      return GRUB_ERR_OUT_OF_RANGE;
+    }
+
+  if ((unsigned long)((grub_uint8_t *)context->sec_dir - (grub_uint8_t *)data) >
+      (datasize - sizeof(struct grub_pe32_data_directory)))
+    {
+      grub_error (GRUB_ERR_OUT_OF_RANGE, "Invalid image");
+      return GRUB_ERR_OUT_OF_RANGE;
+    }
+  return GRUB_ERR_NONE;
+}
+
+static grub_phys_addr_t
+image_address (grub_phys_addr_t image, grub_uint64_t sz, grub_uint64_t addr)
+{
+  if (addr > sz)
+    return 0;
+
+  return image + addr;
+}
+
+static grub_err_t
+relocate_coff (pe_coff_loader_image_context_t *context,
+               struct grub_pe32_section_table *section,
+               grub_phys_addr_t phys_addr, grub_uint8_t *virt_addr,
+               mbi_load_data_t *mld)
+{
+  struct grub_pe32_data_directory *reloc_base, *reloc_base_end;
+  grub_uint8_t *reloc_buffer;
+  grub_uint64_t adjust;
+  struct grub_pe32_fixup_block *reloc, *reloc_end;
+  grub_uint8_t *fixup, *fixup_base;
+  grub_uint16_t *fixup_16;
+  grub_uint32_t *fixup_32;
+  grub_uint64_t *fixup_64;
+  int n = 0;
+  grub_err_t ret = GRUB_ERR_NONE;
+
+  if (image_is_64_bit (context->pe_hdr))
+    context->pe_hdr->pe32plus.optional_header.image_base =
+      (grub_uint64_t)(unsigned long)phys_addr;
+  else
+    context->pe_hdr->pe32.optional_header.image_base =
+      (grub_uint32_t)(unsigned long)phys_addr;
+
+  /* Alright, so here's how this works:
+   *
+   * context->reloc_dir gives us two things:
+   * - the VA the table of base relocation blocks are (maybe) to be
+   *   mapped at (reloc_dir->rva)
+   * - the virtual size (reloc_dir->size)
+   *
+   * The .reloc section (section here) gives us some other things:
+   * - the name! kind of. (section->name)
+   * - the virtual size (section->virtual_size), which should be the same
+   *   as RelocDir->Size
+   * - the virtual address (section->virtual_address)
+   * - the file section size (section->raw_data_size), which is
+   *   a multiple of optional_header->file_alignment.  Only useful for image
+   *   validation, not really useful for iteration bounds.
+   * - the file address (section->raw_data_offset)
+   * - a bunch of stuff we don't use that's 0 in our binaries usually
+   * - Flags (section->characteristics)
+   *
+   * and then the thing that's actually at the file address is an array
+   * of struct grub_pe32_fixup_block structs with some values packed behind
+   * them.  The block_size field of this structure includes the
+   * structure itself, and adding it to that structure's address will
+   * yield the next entry in the array.
+   */
+
+  reloc_buffer = grub_malloc (section->virtual_size);
+  if (!reloc_buffer)
+      return grub_errno;
+
+  if (grub_file_seek (mld->file, section->raw_data_offset) == (grub_off_t) -1)
+    {
+      ret = grub_errno;
+      goto out;
+    }
+
+  if (grub_file_read (mld->file, reloc_buffer, section->virtual_size)
+      != (grub_ssize_t) section->virtual_size)
+    {
+      if (!grub_errno)
+        {
+          grub_error (GRUB_ERR_FILE_READ_ERROR, "premature end of file %s",
+                      mld->filename);
+          ret = GRUB_ERR_FILE_READ_ERROR;
+        }
+      else
+          ret = grub_errno;
+      goto out;
+    }
+
+  reloc_base = (struct grub_pe32_data_directory *)reloc_buffer;
+  reloc_base_end = (struct grub_pe32_data_directory *)(reloc_buffer + section->virtual_size);
+
+  grub_dprintf ("multiboot_loader", "relocate_coff(): reloc_base %p reloc_base_end %p\n",
+                reloc_base, reloc_base_end);
+
+  if (!reloc_base && !reloc_base_end)
+    return GRUB_ERR_NONE;
+
+  if (!reloc_base || !reloc_base_end)
+    {
+      grub_error (GRUB_ERR_BAD_ARGUMENT, "Reloc table overflows binary");
+      return GRUB_ERR_BAD_ARGUMENT;
+    }
+
+  adjust = (grub_uint64_t)(grub_efi_uintn_t)phys_addr - context->image_address;
+  if (adjust == 0)
+    return GRUB_ERR_NONE;
+
+  while (reloc_base < reloc_base_end)
+    {
+      grub_uint16_t *entry;
+      reloc = (struct grub_pe32_fixup_block *)((char*)reloc_base);
+
+      if ((reloc_base->size == 0) ||
+          (reloc_base->size > context->reloc_dir->size))
+        {
+          grub_error (GRUB_ERR_BAD_ARGUMENT,
+                      "Reloc %d block size %d is invalid\n", n,
+                      reloc_base->size);
+          ret = GRUB_ERR_BAD_ARGUMENT;
+          goto out;
+        }
+
+      entry = &reloc->entries[0];
+      reloc_end = (struct grub_pe32_fixup_block *)
+        ((char *)reloc_base + reloc_base->size);
+
+      if ((grub_uint8_t *)reloc_end < reloc_buffer ||
+          (grub_uint8_t *)reloc_end > (reloc_buffer + section->virtual_size))
+        {
+          grub_error (GRUB_ERR_BAD_ARGUMENT, "Reloc entry %d overflows binary",
+                      n);
+          return GRUB_ERR_BAD_ARGUMENT;
+        }
+
+      fixup_base = virt_addr + reloc_base->rva;
+      if (!fixup_base)
+        {
+          grub_error (GRUB_ERR_BAD_ARGUMENT, "Reloc %d Invalid fixupbase", n);
+          ret = GRUB_ERR_BAD_ARGUMENT;
+          goto out;
+        }
+
+      while ((void *)entry < (void *)reloc_end)
+        {
+          fixup = fixup_base + (*entry & 0xFFF);
+          switch ((*entry) >> 12)
+            {
+              case GRUB_PE32_REL_BASED_ABSOLUTE:
+                break;
+              case GRUB_PE32_REL_BASED_HIGH:
+                fixup_16 = (grub_uint16_t *)fixup;
+                *fixup_16 = (grub_uint16_t)
+                  (*fixup_16 + ((grub_uint16_t)((grub_uint32_t)adjust >> 16)));
+                break;
+              case GRUB_PE32_REL_BASED_LOW:
+                fixup_16 = (grub_uint16_t *)fixup;
+                *fixup_16 = (grub_uint16_t) (*fixup_16 + (grub_uint16_t)adjust);
+                break;
+              case GRUB_PE32_REL_BASED_HIGHLOW:
+                fixup_32 = (grub_uint32_t *)fixup;
+                *fixup_32 = *fixup_32 + (grub_uint32_t)adjust;
+                break;
+              case GRUB_PE32_REL_BASED_DIR64:
+                fixup_64 = (grub_uint64_t *)fixup;
+                *fixup_64 = *fixup_64 + (grub_uint64_t)adjust;
+                break;
+              default:
+                grub_error (GRUB_ERR_BAD_ARGUMENT,
+                            "Reloc %d unknown relocation type %d",
+                            n, (*entry) >> 12);
+                ret = GRUB_ERR_BAD_ARGUMENT;
+                goto out;
+            }
+          entry += 1;
+        }
+      reloc_base = (struct grub_pe32_data_directory *)reloc_end;
+      n++;
+    }
+
+out:
+  grub_free(reloc_buffer);
+
+  return ret;
+}
+
+grub_err_t
+grub_multiboot2_load_pe (mbi_load_data_t *mld)
+{
+  int i;
+  grub_uint8_t *virt_addr;
+  grub_phys_addr_t phys_addr, reloc_base, reloc_base_end, base, end;
+  struct grub_pe32_section_table *reloc_section = NULL, fake_reloc_section;
+  struct grub_pe32_section_table *section;
+  grub_relocator_chunk_t ch;
+  pe_coff_loader_image_context_t context;
+  grub_err_t ret = GRUB_ERR_NONE;
+  grub_uint32_t section_alignment;
+  int found_entry_point = 0;
+
+  ret = read_header(mld->buffer, MULTIBOOT_SEARCH, &context);
+  if (ret)
+      return ret;
+
+  /*
+   * The spec says, uselessly, of SectionAlignment:
+   * =====
+   * The alignment (in bytes) of sections when they are loaded into
+   * memory. It must be greater than or equal to FileAlignment. The
+   * default is the page size for the architecture.
+   * =====
+   * Which doesn't tell you whose responsibility it is to enforce the
+   * "default", or when.  It implies that the value in the field must
+   * be > FileAlignment (also poorly defined), but it appears visual
+   * studio will happily write 512 for FileAlignment (its default) and
+   * 0 for SectionAlignment, intending to imply PAGE_SIZE.
+   *
+   * We only support one page size, so if it's zero, nerf it to 4096.
+   */
+  section_alignment = context.section_alignment;
+  if (section_alignment == 0)
+    section_alignment = 4096;
+
+  section_alignment = grub_max(section_alignment, mld->align);
+
+  ret = grub_relocator_alloc_chunk_align_safe (grub_multiboot2_relocator, &ch,
+                                               mld->min_addr, mld->max_addr,
+                                               context.image_size, section_alignment,
+                                               mld->preference, mld->avoid_efi_boot_services);
+
+  if (ret)
+    {
+      grub_dprintf ("multiboot_loader", "Cannot allocate memory for OS image\n");
+      return ret;
+    }
+
+  virt_addr = get_virtual_current_address (ch);
+  phys_addr = get_physical_target_address (ch);
+
+  if (grub_file_seek (mld->file, 0) == (grub_off_t) -1) {
+      ret = grub_errno;
+      goto out;
+  }
+
+  if (grub_file_read (mld->file, virt_addr, context.size_of_headers)
+      != (grub_ssize_t) context.size_of_headers)
+    {
+      if (!grub_errno)
+        {
+          grub_error (GRUB_ERR_FILE_READ_ERROR, "premature end of file %s",
+                      mld->filename);
+          ret = GRUB_ERR_FILE_READ_ERROR;
+        }
+      else
+          ret = grub_errno;
+      goto out;
+    }
+
+  mld->load_base_addr = phys_addr;
+  mld->link_base_addr = context.image_address;
+
+  grub_dprintf ("multiboot_loader", "load_base_addr: 0x%08x link_base_addr 0x%08x\n",
+                mld->load_base_addr, mld->link_base_addr);
+
+  grub_multiboot2_payload_eip = context.entry_point;
+  grub_dprintf ("multiboot_loader", "entry_point: 0x%08x\n", grub_multiboot2_payload_eip);
+  if (!grub_multiboot2_payload_eip)
+    {
+      grub_error (GRUB_ERR_BAD_ARGUMENT, "invalid entry point");
+      ret = GRUB_ERR_BAD_ARGUMENT;
+      goto out;
+    }
+
+  grub_dprintf ("multiboot_loader", "reloc_dir: %p reloc_size: 0x%08x\n",
+                (void *)(unsigned long)context.reloc_dir->rva,
+                context.reloc_dir->size);
+  reloc_base = image_address (phys_addr, context.image_size,
+                              context.reloc_dir->rva);
+  /* RelocBaseEnd here is the address of the last byte of the table */
+  reloc_base_end = image_address (phys_addr, context.image_size,
+                                  context.reloc_dir->rva
+                                  + context.reloc_dir->size - 1);
+  grub_dprintf ("multiboot_loader", "reloc_base: 0x%016lx reloc_base_end: 0x%016lx\n",
+                reloc_base, reloc_base_end);
+
+  section = context.first_section;
+  for (i = 0; i < context.number_of_sections; i++, section++)
+    {
+      char name[9];
+
+      base = image_address (phys_addr, context.image_size,
+                            section->virtual_address);
+      end = image_address (phys_addr, context.image_size,
+                           section->virtual_address + section->virtual_size -1);
+
+      grub_strncpy(name, section->name, 9);
+      name[8] = '\0';
+      grub_dprintf ("multiboot_loader", "Section %d \"%s\" at 0x%016lx..0x%016lx\n", i,
+                   name, base, end);
+
+      if (end < base)
+        {
+          grub_dprintf ("multiboot_loader", " base is 0x%016lx but end is 0x%016lx... bad.\n",
+                       base, end);
+          grub_error (GRUB_ERR_BAD_ARGUMENT,
+                      "Image has invalid negative size");
+          ret = GRUB_ERR_BAD_ARGUMENT;
+          goto out;
+        }
+
+      if (section->virtual_address <= context.entry_point &&
+          (section->virtual_address + section->raw_data_size - 1)
+          > context.entry_point)
+        {
+          found_entry_point++;
+          grub_dprintf ("multiboot_loader", " section contains entry point\n");
+        }
+
+      /* We do want to process .reloc, but it's often marked
+       * discardable, so we don't want to memcpy it. */
+      if (grub_memcmp (section->name, ".reloc\0\0", 8) == 0)
+        {
+          if (reloc_section)
+            {
+              grub_error (GRUB_ERR_BAD_ARGUMENT,
+                          "Image has multiple relocation sections");
+              ret = GRUB_ERR_BAD_ARGUMENT;
+              goto out;
+            }
+
+          /* If it has nonzero sizes, and our bounds check
+           * made sense, and the VA and size match RelocDir's
+           * versions, then we believe in this section table. */
+          if (section->raw_data_size && section->virtual_size &&
+              base && end && reloc_base == base)
+            {
+              if (reloc_base_end == end)
+                {
+                  grub_dprintf ("multiboot_loader", " section is relocation section\n");
+                  reloc_section = section;
+                }
+              else if (reloc_base_end && reloc_base_end < end)
+                {
+                  /* Bogus virtual size in the reloc section -- RelocDir
+                   * reported a smaller Base Relocation Directory. Decrease
+                   * the section's virtual size so that it equal RelocDir's
+                   * idea, but only for the purposes of relocate_coff(). */
+                  grub_dprintf ("multiboot_loader",
+                                " section is (overlong) relocation section\n");
+                  grub_memcpy (&fake_reloc_section, section, sizeof *section);
+                  fake_reloc_section.virtual_size -= (end - reloc_base_end);
+                  reloc_section = &fake_reloc_section;
+                }
+            }
+
+          if (!reloc_section)
+            {
+              grub_dprintf ("multiboot_loader", " section is not reloc section?\n");
+              grub_dprintf ("multiboot_loader", " rds: 0x%08x, vs: %08x\n",
+                            section->raw_data_size, section->virtual_size);
+              grub_dprintf ("multiboot_loader", " base: 0x%016lx end: 0x%016lx\n", base, end);
+              grub_dprintf ("multiboot_loader", " reloc_base: 0x%016lx reloc_base_end: 0x%016lx\n",
+                            reloc_base, reloc_base_end);
+            }
+        }
+
+      grub_dprintf ("multiboot_loader", " Section characteristics are %08x\n",
+                   section->characteristics);
+      grub_dprintf ("multiboot_loader", " Section virtual size: %08x\n",
+                   section->virtual_size);
+      grub_dprintf ("multiboot_loader", " Section raw_data size: %08x\n",
+                   section->raw_data_size);
+      if (section->characteristics & GRUB_PE32_SCN_MEM_DISCARDABLE)
+        {
+          grub_dprintf ("multiboot_loader", " Discarding section\n");
+          continue;
+        }
+
+      if (!base || !end)
+        {
+          grub_dprintf ("multiboot_loader", " section is invalid\n");
+          grub_error (GRUB_ERR_BAD_ARGUMENT, "Invalid section size");
+          ret = GRUB_ERR_BAD_ARGUMENT;
+          goto out;
+        }
+
+      if (section->characteristics & GRUB_PE32_SCN_CNT_UNINITIALIZED_DATA)
+        {
+          if (section->raw_data_size != 0)
+            grub_dprintf ("multiboot_loader", " UNINITIALIZED_DATA section has data?\n");
+        }
+      else if (section->virtual_address < context.size_of_headers ||
+               section->raw_data_offset < context.size_of_headers)
+        {
+          grub_error (GRUB_ERR_BAD_ARGUMENT,
+                      "Section %d is inside image headers", i);
+          ret = GRUB_ERR_BAD_ARGUMENT;
+          goto out;
+        }
+
+      if (section->raw_data_size > 0)
+        {
+          grub_dprintf ("multiboot_loader", " copying 0x%08x bytes to 0x%016lx\n",
+                        section->raw_data_size, base);
+
+          if (grub_file_seek (mld->file, section->raw_data_offset) == (grub_off_t) -1)
+            {
+              ret = grub_errno;
+              goto out;
+            }
+
+          if (grub_file_read (mld->file, virt_addr + (base - phys_addr), section->raw_data_size)
+              != (grub_ssize_t) section->raw_data_size)
+            {
+              if (!grub_errno)
+                {
+                  grub_error (GRUB_ERR_FILE_READ_ERROR, "premature end of file %s",
+                              mld->filename);
+                  ret = GRUB_ERR_FILE_READ_ERROR;
+                }
+              else
+                  ret = grub_errno;
+              goto out;
+            }
+        }
+
+      if (section->raw_data_size < section->virtual_size)
+        {
+          grub_dprintf ("multiboot_loader", " padding with 0x%08x bytes at 0x%016lx\n",
+                        section->virtual_size - section->raw_data_size,
+                        base + section->raw_data_size);
+          grub_memset (virt_addr + (base - phys_addr) + section->raw_data_size, 0,
+                       section->virtual_size - section->raw_data_size);
+        }
+
+      grub_dprintf ("multiboot_loader", " finished section %s\n", name);
+    }
+
+  /* 5 == EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC */
+  if (context.number_of_rva_and_sizes <= 5)
+    {
+      grub_error (GRUB_ERR_BAD_ARGUMENT, "image has no relocation entry\n");
+      ret = GRUB_ERR_BAD_ARGUMENT;
+      goto out;
+    }
+
+  if (context.reloc_dir->size && reloc_section)
+    {
+      /* run the relocation fixups */
+      ret = relocate_coff (&context, reloc_section, phys_addr, virt_addr, mld);
+
+      if (ret)
+        {
+          grub_error (GRUB_ERR_BAD_ARGUMENT, "relocation failed");
+          goto out;
+        }
+    }
+
+  if (!found_entry_point)
+    {
+      grub_error (GRUB_ERR_BAD_ARGUMENT, "entry point is not within sections");
+      ret = GRUB_ERR_BAD_ARGUMENT;
+      goto out;
+    }
+  if (found_entry_point > 1)
+    {
+      grub_error (GRUB_ERR_BAD_ARGUMENT, "%d sections contain entry point",
+                  found_entry_point);
+      ret = GRUB_ERR_BAD_ARGUMENT;
+      goto out;
+    }
+
+out:
+  return ret;
+}
diff --git a/include/grub/efi/pe32.h b/include/grub/efi/pe32.h
index 4e6e9d254bd3..94435d758b46 100644
--- a/include/grub/efi/pe32.h
+++ b/include/grub/efi/pe32.h
@@ -20,6 +20,7 @@
 #define GRUB_EFI_PE32_HEADER	1
 
 #include <grub/types.h>
+#include <grub/efi/api.h>
 #include <grub/efi/memory.h>
 
 /* The MSDOS compatibility stub. This was copied from the output of
@@ -46,6 +47,28 @@
 
 #define GRUB_PE32_MSDOS_STUB_SIZE	0x80
 
+typedef struct {
+  grub_uint16_t  e_magic;    ///< Magic number.
+  grub_uint16_t  e_cblp;     ///< Bytes on last page of file.
+  grub_uint16_t  e_cp;       ///< Pages in file.
+  grub_uint16_t  e_crlc;     ///< Relocations.
+  grub_uint16_t  e_cparhdr;  ///< Size of header in paragraphs.
+  grub_uint16_t  e_minalloc; ///< Minimum extra paragraphs needed.
+  grub_uint16_t  e_maxalloc; ///< Maximum extra paragraphs needed.
+  grub_uint16_t  e_ss;       ///< Initial (relative) SS value.
+  grub_uint16_t  e_sp;       ///< Initial SP value.
+  grub_uint16_t  e_csum;     ///< Checksum.
+  grub_uint16_t  e_ip;       ///< Initial IP value.
+  grub_uint16_t  e_cs;       ///< Initial (relative) CS value.
+  grub_uint16_t  e_lfarlc;   ///< File address of relocation table.
+  grub_uint16_t  e_ovno;     ///< Overlay number.
+  grub_uint16_t  e_res[4];   ///< Reserved words.
+  grub_uint16_t  e_oemid;    ///< OEM identifier (for e_oeminfo).
+  grub_uint16_t  e_oeminfo;  ///< OEM information; e_oemid specific.
+  grub_uint16_t  e_res2[10]; ///< Reserved words.
+  grub_uint32_t  e_lfanew;   ///< File address of new exe header.
+} grub_pe32_msdos_header_t;
+
 #define GRUB_PE32_MAGIC			0x5a4d
 
 struct grub_msdos_image_header
@@ -249,6 +272,7 @@ struct grub_pe32_section_table
 
 #define GRUB_PE32_SCN_CNT_CODE			0x00000020
 #define GRUB_PE32_SCN_CNT_INITIALIZED_DATA	0x00000040
+#define GRUB_PE32_SCN_CNT_UNINITIALIZED_DATA    0x00000080
 #define GRUB_PE32_SCN_MEM_DISCARDABLE		0x02000000
 #define GRUB_PE32_SCN_MEM_EXECUTE		0x20000000
 #define GRUB_PE32_SCN_MEM_READ			0x40000000
@@ -349,4 +373,44 @@ struct grub_pe32_reloc
 #define GRUB_PE32_REL_I386_DIR32	0x6
 #define GRUB_PE32_REL_I386_REL32	0x14
 
+struct grub_pe32_header_32
+{
+  char signature[GRUB_PE32_SIGNATURE_SIZE];
+  struct grub_pe32_coff_header coff_header;
+  struct grub_pe32_optional_header optional_header;
+};
+
+struct grub_pe32_header_64
+{
+  char signature[GRUB_PE32_SIGNATURE_SIZE];
+  struct grub_pe32_coff_header coff_header;
+  struct grub_pe64_optional_header optional_header;
+};
+
+typedef union
+{
+  struct grub_pe32_header_32 pe32;
+  struct grub_pe32_header_64 pe32plus;
+} grub_pe_header_t;
+
+struct pe_coff_loader_image_context
+{
+  grub_efi_uint64_t image_address;
+  grub_efi_uint64_t image_size;
+  grub_efi_uint64_t entry_point;
+  grub_efi_uintn_t size_of_headers;
+  grub_efi_uint16_t image_type;
+  grub_efi_uint16_t number_of_sections;
+  grub_efi_uint32_t section_alignment;
+  struct grub_pe32_section_table *first_section;
+  struct grub_pe32_data_directory *reloc_dir;
+  struct grub_pe32_data_directory *sec_dir;
+  grub_efi_uint64_t number_of_rva_and_sizes;
+  grub_pe_header_t *pe_hdr;
+};
+
+typedef struct pe_coff_loader_image_context pe_coff_loader_image_context_t;
+
+#define EFI_IMAGE_NUMBER_OF_DIRECTORY_ENTRIES 16
+
 #endif /* ! GRUB_EFI_PE32_HEADER */
diff --git a/include/grub/multiboot2.h b/include/grub/multiboot2.h
index 1f6d4fc9e336..01e2b11755c2 100644
--- a/include/grub/multiboot2.h
+++ b/include/grub/multiboot2.h
@@ -97,6 +97,9 @@ typedef struct mbi_load_data mbi_load_data_t;
 grub_err_t
 grub_multiboot2_load_elf (mbi_load_data_t *mld);
 
+grub_err_t
+grub_multiboot2_load_pe (mbi_load_data_t *mld);
+
 extern grub_size_t grub_multiboot2_pure_size;
 extern grub_size_t grub_multiboot2_alloc_mbi;
 extern grub_uint32_t grub_multiboot2_payload_eip;
-- 
2.43.0


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH 5/7] multiboot2: Add support for 64-bit entry addresses

[Ross Lagerwall]

Add support for entry addresses that may be either 32 bits or 64 bits in
size. This may be necessary if the binary is built with an entry address
above 4G.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 grub-core/loader/multiboot_mbi2.c | 10 ++++++++--
 include/multiboot2.h              |  6 +++++-
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/grub-core/loader/multiboot_mbi2.c b/grub-core/loader/multiboot_mbi2.c
index 3ec96e2f29b9..c268aa614284 100644
--- a/grub-core/loader/multiboot_mbi2.c
+++ b/grub-core/loader/multiboot_mbi2.c
@@ -205,13 +205,19 @@ grub_multiboot2_load (grub_file_t file, const char *filename)
 
       case MULTIBOOT_HEADER_TAG_ENTRY_ADDRESS:
 	entry_specified = 1;
-	entry = ((struct multiboot_header_tag_entry_address *) tag)->entry_addr;
+	if ((tag->size - sizeof(struct multiboot_header_tag)) == sizeof(multiboot_uint64_t))
+	  entry = ((struct multiboot_header_tag_entry_address *) tag)->entry_addr64;
+	else
+	  entry = ((struct multiboot_header_tag_entry_address *) tag)->entry_addr32;
 	break;
 
       case MULTIBOOT_HEADER_TAG_ENTRY_ADDRESS_EFI64:
 #if defined (GRUB_MACHINE_EFI) && defined (__x86_64__)
 	efi_entry_specified = 1;
-	efi_entry = ((struct multiboot_header_tag_entry_address *) tag)->entry_addr;
+	if ((tag->size - sizeof(struct multiboot_header_tag)) == sizeof(multiboot_uint64_t))
+	  efi_entry = ((struct multiboot_header_tag_entry_address *) tag)->entry_addr64;
+	else
+	  efi_entry = ((struct multiboot_header_tag_entry_address *) tag)->entry_addr32;
 #endif
 	break;
 
diff --git a/include/multiboot2.h b/include/multiboot2.h
index f4377bf7b394..e4798bfef937 100644
--- a/include/multiboot2.h
+++ b/include/multiboot2.h
@@ -140,7 +140,11 @@ struct multiboot_header_tag_entry_address
   multiboot_uint16_t type;
   multiboot_uint16_t flags;
   multiboot_uint32_t size;
-  multiboot_uint32_t entry_addr;
+  union
+  {
+    multiboot_uint32_t entry_addr32;
+    multiboot_uint64_t entry_addr64;
+  };
 };
 
 struct multiboot_header_tag_console_flags
-- 
2.43.0

[PATCH 5/7] multiboot2: Add support for 64-bit entry addresses

[Ross Lagerwall via Grub-devel]

Add support for entry addresses that may be either 32 bits or 64 bits in
size. This may be necessary if the binary is built with an entry address
above 4G.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 grub-core/loader/multiboot_mbi2.c | 10 ++++++++--
 include/multiboot2.h              |  6 +++++-
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/grub-core/loader/multiboot_mbi2.c b/grub-core/loader/multiboot_mbi2.c
index 3ec96e2f29b9..c268aa614284 100644
--- a/grub-core/loader/multiboot_mbi2.c
+++ b/grub-core/loader/multiboot_mbi2.c
@@ -205,13 +205,19 @@ grub_multiboot2_load (grub_file_t file, const char *filename)
 
       case MULTIBOOT_HEADER_TAG_ENTRY_ADDRESS:
 	entry_specified = 1;
-	entry = ((struct multiboot_header_tag_entry_address *) tag)->entry_addr;
+	if ((tag->size - sizeof(struct multiboot_header_tag)) == sizeof(multiboot_uint64_t))
+	  entry = ((struct multiboot_header_tag_entry_address *) tag)->entry_addr64;
+	else
+	  entry = ((struct multiboot_header_tag_entry_address *) tag)->entry_addr32;
 	break;
 
       case MULTIBOOT_HEADER_TAG_ENTRY_ADDRESS_EFI64:
 #if defined (GRUB_MACHINE_EFI) && defined (__x86_64__)
 	efi_entry_specified = 1;
-	efi_entry = ((struct multiboot_header_tag_entry_address *) tag)->entry_addr;
+	if ((tag->size - sizeof(struct multiboot_header_tag)) == sizeof(multiboot_uint64_t))
+	  efi_entry = ((struct multiboot_header_tag_entry_address *) tag)->entry_addr64;
+	else
+	  efi_entry = ((struct multiboot_header_tag_entry_address *) tag)->entry_addr32;
 #endif
 	break;
 
diff --git a/include/multiboot2.h b/include/multiboot2.h
index f4377bf7b394..e4798bfef937 100644
--- a/include/multiboot2.h
+++ b/include/multiboot2.h
@@ -140,7 +140,11 @@ struct multiboot_header_tag_entry_address
   multiboot_uint16_t type;
   multiboot_uint16_t flags;
   multiboot_uint32_t size;
-  multiboot_uint32_t entry_addr;
+  union
+  {
+    multiboot_uint32_t entry_addr32;
+    multiboot_uint64_t entry_addr64;
+  };
 };
 
 struct multiboot_header_tag_console_flags
-- 
2.43.0


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH 6/7] efi: Allow loading multiboot modules without verification

[Ross Lagerwall]

GRUB doesn't do anything with multiboot modules except loading them and
passing a pointer to the multiboot kernel. Therefore GRUB itself doesn't
need to verify the module. Multiboot modules may contain code that needs
to be verified. If this is the case, the expectation is that the
multiboot kernel verifies the modules. For example, with Xen, the first
multiboot module contains the dom0 kernel binary and Xen verifies it
before starting it.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 grub-core/kern/efi/sb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/grub-core/kern/efi/sb.c b/grub-core/kern/efi/sb.c
index 8d3e413608bb..f76290d65e9f 100644
--- a/grub-core/kern/efi/sb.c
+++ b/grub-core/kern/efi/sb.c
@@ -171,6 +171,7 @@ shim_lock_verifier_init (grub_file_t io __attribute__ ((unused)),
     case GRUB_FILE_TYPE_LOADENV:
     case GRUB_FILE_TYPE_SAVEENV:
     case GRUB_FILE_TYPE_VERIFY_SIGNATURE:
+    case GRUB_FILE_TYPE_MULTIBOOT_MODULE:
       *flags = GRUB_VERIFY_FLAGS_SKIP_VERIFICATION;
       return GRUB_ERR_NONE;
 
-- 
2.43.0

[PATCH 6/7] efi: Allow loading multiboot modules without verification

[Ross Lagerwall via Grub-devel]

GRUB doesn't do anything with multiboot modules except loading them and
passing a pointer to the multiboot kernel. Therefore GRUB itself doesn't
need to verify the module. Multiboot modules may contain code that needs
to be verified. If this is the case, the expectation is that the
multiboot kernel verifies the modules. For example, with Xen, the first
multiboot module contains the dom0 kernel binary and Xen verifies it
before starting it.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 grub-core/kern/efi/sb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/grub-core/kern/efi/sb.c b/grub-core/kern/efi/sb.c
index 8d3e413608bb..f76290d65e9f 100644
--- a/grub-core/kern/efi/sb.c
+++ b/grub-core/kern/efi/sb.c
@@ -171,6 +171,7 @@ shim_lock_verifier_init (grub_file_t io __attribute__ ((unused)),
     case GRUB_FILE_TYPE_LOADENV:
     case GRUB_FILE_TYPE_SAVEENV:
     case GRUB_FILE_TYPE_VERIFY_SIGNATURE:
+    case GRUB_FILE_TYPE_MULTIBOOT_MODULE:
       *flags = GRUB_VERIFY_FLAGS_SKIP_VERIFICATION;
       return GRUB_ERR_NONE;
 
-- 
2.43.0


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH 7/7] verifiers: Verify after decompression

[Ross Lagerwall]

It is convenient and common to have binaries stored in gzip archives
(e.g. xen.gz). Verification should be run after decompression rather
than before so reorder the file filter list as appropriate.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 include/grub/file.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/grub/file.h b/include/grub/file.h
index a5bf3a792d6f..a1ef3582bc7b 100644
--- a/include/grub/file.h
+++ b/include/grub/file.h
@@ -182,10 +182,10 @@ extern grub_disk_read_hook_t EXPORT_VAR(grub_file_progress_hook);
 /* Filters with lower ID are executed first.  */
 typedef enum grub_file_filter_id
   {
-    GRUB_FILE_FILTER_VERIFY,
     GRUB_FILE_FILTER_GZIO,
     GRUB_FILE_FILTER_XZIO,
     GRUB_FILE_FILTER_LZOPIO,
+    GRUB_FILE_FILTER_VERIFY,
     GRUB_FILE_FILTER_MAX,
     GRUB_FILE_FILTER_COMPRESSION_FIRST = GRUB_FILE_FILTER_GZIO,
     GRUB_FILE_FILTER_COMPRESSION_LAST = GRUB_FILE_FILTER_LZOPIO,
-- 
2.43.0

[PATCH 7/7] verifiers: Verify after decompression

[Ross Lagerwall via Grub-devel]

It is convenient and common to have binaries stored in gzip archives
(e.g. xen.gz). Verification should be run after decompression rather
than before so reorder the file filter list as appropriate.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 include/grub/file.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/grub/file.h b/include/grub/file.h
index a5bf3a792d6f..a1ef3582bc7b 100644
--- a/include/grub/file.h
+++ b/include/grub/file.h
@@ -182,10 +182,10 @@ extern grub_disk_read_hook_t EXPORT_VAR(grub_file_progress_hook);
 /* Filters with lower ID are executed first.  */
 typedef enum grub_file_filter_id
   {
-    GRUB_FILE_FILTER_VERIFY,
     GRUB_FILE_FILTER_GZIO,
     GRUB_FILE_FILTER_XZIO,
     GRUB_FILE_FILTER_LZOPIO,
+    GRUB_FILE_FILTER_VERIFY,
     GRUB_FILE_FILTER_MAX,
     GRUB_FILE_FILTER_COMPRESSION_FIRST = GRUB_FILE_FILTER_GZIO,
     GRUB_FILE_FILTER_COMPRESSION_LAST = GRUB_FILE_FILTER_LZOPIO,
-- 
2.43.0


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 7/7] verifiers: Verify after decompression

[Michael Chang]

On Wed, Mar 13, 2024 at 03:07:48PM +0000, Ross Lagerwall via Grub-devel wrote:
> It is convenient and common to have binaries stored in gzip archives
> (e.g. xen.gz). Verification should be run after decompression rather
> than before so reorder the file filter list as appropriate.

The proposed change would result in the disruption of the tpm and pgp
clients within the verifier framework. Specifically, the tpm pcr
prediction software relies on the integrity of raw files rather than
decompressed ones. Additionally, pgp detached signatures are designed to
target original files, thus necessitating the current structure to
maintain functionality.

Thanks,
Michael

> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
>  include/grub/file.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/grub/file.h b/include/grub/file.h
> index a5bf3a792d6f..a1ef3582bc7b 100644
> --- a/include/grub/file.h
> +++ b/include/grub/file.h
> @@ -182,10 +182,10 @@ extern grub_disk_read_hook_t EXPORT_VAR(grub_file_progress_hook);
>  /* Filters with lower ID are executed first.  */
>  typedef enum grub_file_filter_id
>    {
> -    GRUB_FILE_FILTER_VERIFY,
>      GRUB_FILE_FILTER_GZIO,
>      GRUB_FILE_FILTER_XZIO,
>      GRUB_FILE_FILTER_LZOPIO,
> +    GRUB_FILE_FILTER_VERIFY,
>      GRUB_FILE_FILTER_MAX,
>      GRUB_FILE_FILTER_COMPRESSION_FIRST = GRUB_FILE_FILTER_GZIO,
>      GRUB_FILE_FILTER_COMPRESSION_LAST = GRUB_FILE_FILTER_LZOPIO,
> -- 
> 2.43.0
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 7/7] verifiers: Verify after decompression

[Michael Chang via Grub-devel]

On Wed, Mar 13, 2024 at 03:07:48PM +0000, Ross Lagerwall via Grub-devel wrote:
> It is convenient and common to have binaries stored in gzip archives
> (e.g. xen.gz). Verification should be run after decompression rather
> than before so reorder the file filter list as appropriate.

The proposed change would result in the disruption of the tpm and pgp
clients within the verifier framework. Specifically, the tpm pcr
prediction software relies on the integrity of raw files rather than
decompressed ones. Additionally, pgp detached signatures are designed to
target original files, thus necessitating the current structure to
maintain functionality.

Thanks,
Michael

> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
>  include/grub/file.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/grub/file.h b/include/grub/file.h
> index a5bf3a792d6f..a1ef3582bc7b 100644
> --- a/include/grub/file.h
> +++ b/include/grub/file.h
> @@ -182,10 +182,10 @@ extern grub_disk_read_hook_t EXPORT_VAR(grub_file_progress_hook);
>  /* Filters with lower ID are executed first.  */
>  typedef enum grub_file_filter_id
>    {
> -    GRUB_FILE_FILTER_VERIFY,
>      GRUB_FILE_FILTER_GZIO,
>      GRUB_FILE_FILTER_XZIO,
>      GRUB_FILE_FILTER_LZOPIO,
> +    GRUB_FILE_FILTER_VERIFY,
>      GRUB_FILE_FILTER_MAX,
>      GRUB_FILE_FILTER_COMPRESSION_FIRST = GRUB_FILE_FILTER_GZIO,
>      GRUB_FILE_FILTER_COMPRESSION_LAST = GRUB_FILE_FILTER_LZOPIO,
> -- 
> 2.43.0
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 7/7] verifiers: Verify after decompression

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1.1: Type: text/plain, Size: 1506 bytes --]

Verifying after decompression is a bad security practice. It relies on
decompression having no security holes. Given how complex decompression is,
this is almost guaranteed to be false.

Le mer. 13 mars 2024, 18:08, Ross Lagerwall via Grub-devel <
grub-devel@gnu.org> a écrit :

> It is convenient and common to have binaries stored in gzip archives
> (e.g. xen.gz). Verification should be run after decompression rather
> than before so reorder the file filter list as appropriate.
>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
>  include/grub/file.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/grub/file.h b/include/grub/file.h
> index a5bf3a792d6f..a1ef3582bc7b 100644
> --- a/include/grub/file.h
> +++ b/include/grub/file.h
> @@ -182,10 +182,10 @@ extern grub_disk_read_hook_t
> EXPORT_VAR(grub_file_progress_hook);
>  /* Filters with lower ID are executed first.  */
>  typedef enum grub_file_filter_id
>    {
> -    GRUB_FILE_FILTER_VERIFY,
>      GRUB_FILE_FILTER_GZIO,
>      GRUB_FILE_FILTER_XZIO,
>      GRUB_FILE_FILTER_LZOPIO,
> +    GRUB_FILE_FILTER_VERIFY,
>      GRUB_FILE_FILTER_MAX,
>      GRUB_FILE_FILTER_COMPRESSION_FIRST = GRUB_FILE_FILTER_GZIO,
>      GRUB_FILE_FILTER_COMPRESSION_LAST = GRUB_FILE_FILTER_LZOPIO,
> --
> 2.43.0
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #1.2: Type: text/html, Size: 2120 bytes --]

[-- Attachment #2: Type: text/plain, Size: 141 bytes --]

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 7/7] verifiers: Verify after decompression

[Ross Lagerwall via Grub-devel]

On Fri, Mar 15, 2024 at 7:26 AM Vladimir 'phcoder' Serbinenko
<phcoder@gmail.com> wrote:
>
> Verifying after decompression is a bad security practice. It relies on decompression having no security holes. Given how complex decompression is, this is almost guaranteed to be false.
>

Point taken... I'll drop this patch as it is not essential to the goal
of booting Secure Booting Xen via GRUB.

Thanks,
Ross

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel