[PATCH v3] efi: Request desired alignment via the PE/COFF headers

[PATCH v3] efi: Request desired alignment via the PE/COFF headers

[Michael Brown]

The kernel will align itself to the nearest boundary specified by the
kernel_alignment field in the bzImage header.  If the kernel is loaded
to an address which is not sufficiently aligned, it will therefore use
memory beyond that indicated solely by the init_size field.

The PE/COFF headers now include a .bss section to describe the
requirements of the init_size field, but do not currently expose the
alignment requirement.  Consequently, a kernel loaded via the PE entry
point may still end up overwriting unexpected areas of memory.

Fix by exposing the desired alignment via the SectionAlignment field
in the PE/COFF headers.  Despite its name, this field provides an
overall alignment requirement for the loaded file.  (Naturally, the
FileAlignment field describes the alignment for individual sections.)

There is no way in the PE/COFF headers to express the concept of
min_alignment; we therefore do not expose the minimum (as opposed to
preferred) alignment.

Signed-off-by: Michael Brown <mbrown-OViyBiuKJBuK421+ScFKDQ@public.gmane.org>
---
 arch/x86/boot/header.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
index 7a6d43a..16ef025 100644
--- a/arch/x86/boot/header.S
+++ b/arch/x86/boot/header.S
@@ -154,7 +154,7 @@ extra_header_fields:
 #else
 	.quad	0				# ImageBase
 #endif
-	.long	0x20				# SectionAlignment
+	.long	CONFIG_PHYSICAL_ALIGN		# SectionAlignment
 	.long	0x20				# FileAlignment
 	.word	0				# MajorOperatingSystemVersion
 	.word	0				# MinorOperatingSystemVersion
-- 
1.8.4.5

Re: [PATCH v3] efi: Request desired alignment via the PE/COFF headers

[Matt Fleming]

On Thu, 10 Jul, at 04:59:23PM, Michael Brown wrote:
> The kernel will align itself to the nearest boundary specified by the
> kernel_alignment field in the bzImage header.  If the kernel is loaded
> to an address which is not sufficiently aligned, it will therefore use
> memory beyond that indicated solely by the init_size field.
> 
> The PE/COFF headers now include a .bss section to describe the
> requirements of the init_size field, but do not currently expose the
> alignment requirement.  Consequently, a kernel loaded via the PE entry
> point may still end up overwriting unexpected areas of memory.
 
Is this actually true? There is code within the EFI boot stub to
allocate space for the kernel image and perform the relocation if it's
not already suitably aligned.

Or is the above paragraph referring to the previously merged patch?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v3] efi: Request desired alignment via the PE/COFF headers

[Michael Brown]

On 10/07/14 21:36, Matt Fleming wrote:
> On Thu, 10 Jul, at 04:59:23PM, Michael Brown wrote:
>> The kernel will align itself to the nearest boundary specified by the
>> kernel_alignment field in the bzImage header.  If the kernel is loaded
>> to an address which is not sufficiently aligned, it will therefore use
>> memory beyond that indicated solely by the init_size field.
>>
>> The PE/COFF headers now include a .bss section to describe the
>> requirements of the init_size field, but do not currently expose the
>> alignment requirement.  Consequently, a kernel loaded via the PE entry
>> point may still end up overwriting unexpected areas of memory.
>
> Is this actually true? There is code within the EFI boot stub to
> allocate space for the kernel image and perform the relocation if it's
> not already suitably aligned.
>
> Or is the above paragraph referring to the previously merged patch?

The "...headers now include..." part was referring to the previously 
merged patch to add the .bss section.

I haven't actually looked at the code which performs the alignment; I 
was going on hpa's concern that merely exposing init_size would be 
insufficient due to the potential for alignment.  My understanding 
(possibly incorrect) was that the alignment was carried out using 
something simple along the lines of:

   new_kernel_start = align ( kernel_start, kernel_alignment );
   memmove ( new_kernel_start, kernel_start, kernel_len );

i.e. that the memory used for alignment was not explicitly allocated. 
If the EFI boot stub instead allocates space for the aligned kernel 
using AllocatePages() (and allocates enough space for the whole of 
init_size), then the problem I described does not exist.

Michael

Re: [PATCH v3] efi: Request desired alignment via the PE/COFF headers

[Matt Fleming]

On Fri, 11 Jul, at 01:18:43AM, Michael Brown wrote:
> 
> The "...headers now include..." part was referring to the previously
> merged patch to add the .bss section.
> 
> I haven't actually looked at the code which performs the alignment;
> I was going on hpa's concern that merely exposing init_size would be
> insufficient due to the potential for alignment.  My understanding
> (possibly incorrect) was that the alignment was carried out using
> something simple along the lines of:
> 
>   new_kernel_start = align ( kernel_start, kernel_alignment );
>   memmove ( new_kernel_start, kernel_start, kernel_len );
> 
> i.e. that the memory used for alignment was not explicitly
> allocated. If the EFI boot stub instead allocates space for the
> aligned kernel using AllocatePages() (and allocates enough space for
> the whole of init_size), then the problem I described does not
> exist.

Right, this shouldn't be a problem because we do in fact allocate space
using the EFI boottime services in efi_relocate_kernel(), taking the
alignment into account, and then perform the kernel image copy.

I still think your change makes sense, I'm just inclined to delete the
paragraph referring to the corruption bug (which we've established
doesn't exist).

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v3] efi: Request desired alignment via the PE/COFF headers

[Michael Brown]

On 11/07/14 08:41, Matt Fleming wrote:
>> i.e. that the memory used for alignment was not explicitly
>> allocated. If the EFI boot stub instead allocates space for the
>> aligned kernel using AllocatePages() (and allocates enough space for
>> the whole of init_size), then the problem I described does not
>> exist.
>
> Right, this shouldn't be a problem because we do in fact allocate space
> using the EFI boottime services in efi_relocate_kernel(), taking the
> alignment into account, and then perform the kernel image copy.
>
> I still think your change makes sense, I'm just inclined to delete the
> paragraph referring to the corruption bug (which we've established
> doesn't exist).

Since the patch itself is so trivial, I'm happy for you to commit it as 
your own, with whatever commit message you think makes most sense.

Michael

Re: [PATCH v3] efi: Request desired alignment via the PE/COFF headers

[Matt Fleming]

On Fri, 11 Jul, at 04:16:30PM, Michael Brown wrote:
> 
> Since the patch itself is so trivial, I'm happy for you to commit it
> as your own, with whatever commit message you think makes most
> sense.

I was thinking something along these lines,

---

>From 997cea56672faf5521cd5718281b99d4e6afdb2d Mon Sep 17 00:00:00 2001
From: Michael Brown <mbrown-OViyBiuKJBuK421+ScFKDQ@public.gmane.org>
Date: Thu, 10 Jul 2014 16:59:23 +0100
Subject: [PATCH] x86/efi: Request desired alignment via the PE/COFF headers

The EFI boot stub goes to great pains to relocate the kernel image to
an appropriately aligned address, as indicated by the ->kernel_alignment
field in the bzImage header.  However, for the PE stub entry case, we
can request that the EFI PE/COFF loader do the work for us.

Fix by exposing the desired alignment via the SectionAlignment field
in the PE/COFF headers.  Despite its name, this field provides an
overall alignment requirement for the loaded file.  (Naturally, the
FileAlignment field describes the alignment for individual sections.)

There is no way in the PE/COFF headers to express the concept of
min_alignment; we therefore do not expose the minimum (as opposed to
preferred) alignment.

Signed-off-by: Michael Brown <mbrown-OViyBiuKJBuK421+ScFKDQ@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 arch/x86/boot/header.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
index 84c223479e3c..1fdb350c4a58 100644
--- a/arch/x86/boot/header.S
+++ b/arch/x86/boot/header.S
@@ -155,7 +155,7 @@ extra_header_fields:
 #else
 	.quad	0				# ImageBase
 #endif
-	.long	0x20				# SectionAlignment
+	.long	CONFIG_PHYSICAL_ALIGN		# SectionAlignment
 	.long	0x20				# FileAlignment
 	.word	0				# MajorOperatingSystemVersion
 	.word	0				# MinorOperatingSystemVersion
-- 
1.9.0

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v3] efi: Request desired alignment via the PE/COFF headers

[Michael Brown]

On 14/07/14 14:10, Matt Fleming wrote:
> On Fri, 11 Jul, at 04:16:30PM, Michael Brown wrote:
> The EFI boot stub goes to great pains to relocate the kernel image to
> an appropriately aligned address, as indicated by the ->kernel_alignment
> field in the bzImage header.  However, for the PE stub entry case, we
> can request that the EFI PE/COFF loader do the work for us.
>
> Fix by exposing the desired alignment via the SectionAlignment field
> in the PE/COFF headers.  Despite its name, this field provides an
> overall alignment requirement for the loaded file.  (Naturally, the
> FileAlignment field describes the alignment for individual sections.)
>
> There is no way in the PE/COFF headers to express the concept of
> min_alignment; we therefore do not expose the minimum (as opposed to
> preferred) alignment.
>
> Signed-off-by: Michael Brown <mbrown-OViyBiuKJBuK421+ScFKDQ@public.gmane.org>
> Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Looks good to me.

Michael

Re: [PATCH v3] efi: Request desired alignment via the PE/COFF headers

[Linn Crosetto]

Michael Brown <mbrown@...> writes:

> 
> The kernel will align itself to the nearest boundary specified by the
> kernel_alignment field in the bzImage header.  If the kernel is loaded
> to an address which is not sufficiently aligned, it will therefore use
> memory beyond that indicated solely by the init_size field.
> 
> The PE/COFF headers now include a .bss section to describe the
> requirements of the init_size field, but do not currently expose the
> alignment requirement.  Consequently, a kernel loaded via the PE entry
> point may still end up overwriting unexpected areas of memory.
> 
> Fix by exposing the desired alignment via the SectionAlignment field
> in the PE/COFF headers.  Despite its name, this field provides an
> overall alignment requirement for the loaded file.  (Naturally, the
> FileAlignment field describes the alignment for individual sections.)
> 
> There is no way in the PE/COFF headers to express the concept of
> min_alignment; we therefore do not expose the minimum (as opposed to
> preferred) alignment.
> 
> Signed-off-by: Michael Brown <mbrown@...>
> ---
>  arch/x86/boot/header.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
> index 7a6d43a..16ef025 100644
> --- a/arch/x86/boot/header.S
> +++ b/arch/x86/boot/header.S
>  <at>  <at>  -154,7 +154,7  <at>  <at>  extra_header_fields:
>  #else
>  	.quad	0				# ImageBase
>  #endif
> -	.long	0x20				# SectionAlignment
> +	.long	CONFIG_PHYSICAL_ALIGN		# SectionAlignment
>  	.long	0x20				# FileAlignment
>  	.word	0				# 
MajorOperatingSystemVersion
>  	.word	0				# 
MinorOperatingSystemVersion

There may be a problem with this change. The specification says that the 
SizeOfImage field must be a multiple of SectionAlignment. That was the case 
when SectionAlignment was 0x20, but now that it is so large it would 
require quite an increase in the last section to comply. A kernel I am 
looking at now has:

Alignment of sections:	0x1000000
Size of image:	0x105a000

Re: [PATCH v3] efi: Request desired alignment via the PE/COFF headers

[Michael Brown]

On 15/06/15 22:43, Linn Crosetto wrote:
> Michael Brown <mbrown@...> writes:
>> The kernel will align itself to the nearest boundary specified by the
>> kernel_alignment field in the bzImage header.  If the kernel is loaded
>> to an address which is not sufficiently aligned, it will therefore use
>> memory beyond that indicated solely by the init_size field.
>>
>> The PE/COFF headers now include a .bss section to describe the
>> requirements of the init_size field, but do not currently expose the
>> alignment requirement.  Consequently, a kernel loaded via the PE entry
>> point may still end up overwriting unexpected areas of memory.
>>
>> Fix by exposing the desired alignment via the SectionAlignment field
>> in the PE/COFF headers.  Despite its name, this field provides an
>> overall alignment requirement for the loaded file.  (Naturally, the
>> FileAlignment field describes the alignment for individual sections.)
>>
>> There is no way in the PE/COFF headers to express the concept of
>> min_alignment; we therefore do not expose the minimum (as opposed to
>> preferred) alignment.
>
> <snip>
>
> There may be a problem with this change. The specification says that the
> SizeOfImage field must be a multiple of SectionAlignment. That was the case
> when SectionAlignment was 0x20, but now that it is so large it would
> require quite an increase in the last section to comply. A kernel I am
> looking at now has:
>
> Alignment of sections:	0x1000000
> Size of image:	0x105a000

Any suggested solution?

Michael

Re: [PATCH v3] efi: Request desired alignment via the PE/COFF headers

[Linn Crosetto]

On Tue, Jun 16, 2015 at 05:19:29PM +0100, Michael Brown wrote:
> On 15/06/15 22:43, Linn Crosetto wrote:
> >Michael Brown <mbrown@...> writes:
> >>The kernel will align itself to the nearest boundary specified by the
> >>kernel_alignment field in the bzImage header.  If the kernel is loaded
> >>to an address which is not sufficiently aligned, it will therefore use
> >>memory beyond that indicated solely by the init_size field.
> >>
> >>The PE/COFF headers now include a .bss section to describe the
> >>requirements of the init_size field, but do not currently expose the
> >>alignment requirement.  Consequently, a kernel loaded via the PE entry
> >>point may still end up overwriting unexpected areas of memory.
> >>
> >>Fix by exposing the desired alignment via the SectionAlignment field
> >>in the PE/COFF headers.  Despite its name, this field provides an
> >>overall alignment requirement for the loaded file.  (Naturally, the
> >>FileAlignment field describes the alignment for individual sections.)
> >>
> >>There is no way in the PE/COFF headers to express the concept of
> >>min_alignment; we therefore do not expose the minimum (as opposed to
> >>preferred) alignment.
> >
> ><snip>
> >
> >There may be a problem with this change. The specification says that the
> >SizeOfImage field must be a multiple of SectionAlignment. That was the case
> >when SectionAlignment was 0x20, but now that it is so large it would
> >require quite an increase in the last section to comply. A kernel I am
> >looking at now has:
> >
> >Alignment of sections:	0x1000000
> >Size of image:	0x105a000
> 
> Any suggested solution?

I have been reverting this patch as a workaround. The fields need to be changed,
but I am not that familiar with the code. Maybe there is a way to use a
heuristic to calculate the best values based on init_sz?

Re: [PATCH v3] efi: Request desired alignment via the PE/COFF headers

[Matt Fleming]

On Tue, 16 Jun, at 11:37:25AM, Linn Crosetto wrote:
> 
> I have been reverting this patch as a workaround. The fields need to be changed,
> but I am not that familiar with the code. Maybe there is a way to use a
> heuristic to calculate the best values based on init_sz?

Linn, could you please provide some details of the system that you're
booting this kernel on? EDK2 does not include any checks for this
alignment requirement, which probably explains why no one else ever
caught this issue.

I can't think of any way to fix this without simply doing a revert of
commit aeffc4928ea2 ("x86/efi: Request desired alignment via the PE/COFF
headers"). Especially since that patch was an optimisation and not a bug
fix.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v3] efi: Request desired alignment via the PE/COFF headers

[Michael Brown]

On 18/06/15 23:02, Matt Fleming wrote:
> On Tue, 16 Jun, at 11:37:25AM, Linn Crosetto wrote:
>> I have been reverting this patch as a workaround. The fields need to be changed,
>> but I am not that familiar with the code. Maybe there is a way to use a
>> heuristic to calculate the best values based on init_sz?
>
> Linn, could you please provide some details of the system that you're
> booting this kernel on? EDK2 does not include any checks for this
> alignment requirement, which probably explains why no one else ever
> caught this issue.
>
> I can't think of any way to fix this without simply doing a revert of
> commit aeffc4928ea2 ("x86/efi: Request desired alignment via the PE/COFF
> headers"). Especially since that patch was an optimisation and not a bug
> fix.

I'm pretty sure that patch _is_ a bug fix, not just an optimisation.  It 
looks as though the commit log message was changed from what I 
originally wrote:

    The kernel will align itself to the nearest boundary specified by the
    kernel_alignment field in the bzImage header.  If the kernel is loaded
    to an address which is not sufficiently aligned, it will therefore use
    memory beyond that indicated solely by the init_size field.

    The PE/COFF headers now include a .bss section to describe the
    requirements of the init_size field, but do not currently expose the
    alignment requirement.  Consequently, a kernel loaded via the PE entry
    point may still end up overwriting unexpected areas of memory.

to

    The EFI boot stub goes to great pains to relocate the kernel image to
    an appropriately aligned address, as indicated by the ->kernel_alignment
    field in the bzImage header.  However, for the PE stub entry case, we
    can request that the EFI PE/COFF loader do the work for us.

If the patch is reverted, then I think it will cause undefined behaviour 
on some platforms (which happen to load the kernel to non-preferred 
alignment, and where the memory immediately after the loaded kernel 
happens to be in use for something).

Michael

Re: [PATCH v3] efi: Request desired alignment via the PE/COFF headers

[Matt Fleming]

On Thu, 18 Jun, at 11:27:55PM, Michael Brown wrote:
> On 18/06/15 23:02, Matt Fleming wrote:
> >On Tue, 16 Jun, at 11:37:25AM, Linn Crosetto wrote:
> >>I have been reverting this patch as a workaround. The fields need to be changed,
> >>but I am not that familiar with the code. Maybe there is a way to use a
> >>heuristic to calculate the best values based on init_sz?
> >
> >Linn, could you please provide some details of the system that you're
> >booting this kernel on? EDK2 does not include any checks for this
> >alignment requirement, which probably explains why no one else ever
> >caught this issue.
> >
> >I can't think of any way to fix this without simply doing a revert of
> >commit aeffc4928ea2 ("x86/efi: Request desired alignment via the PE/COFF
> >headers"). Especially since that patch was an optimisation and not a bug
> >fix.
> 
> I'm pretty sure that patch _is_ a bug fix, not just an optimisation.
> It looks as though the commit log message was changed from what I
> originally wrote:
> 
>    The kernel will align itself to the nearest boundary specified by the
>    kernel_alignment field in the bzImage header.  If the kernel is loaded
>    to an address which is not sufficiently aligned, it will therefore use
>    memory beyond that indicated solely by the init_size field.
> 
>    The PE/COFF headers now include a .bss section to describe the
>    requirements of the init_size field, but do not currently expose the
>    alignment requirement.  Consequently, a kernel loaded via the PE entry
>    point may still end up overwriting unexpected areas of memory.
> 
> to
> 
>    The EFI boot stub goes to great pains to relocate the kernel image to
>    an appropriately aligned address, as indicated by the ->kernel_alignment
>    field in the bzImage header.  However, for the PE stub entry case, we
>    can request that the EFI PE/COFF loader do the work for us.
> 
> If the patch is reverted, then I think it will cause undefined
> behaviour on some platforms (which happen to load the kernel to
> non-preferred alignment, and where the memory immediately after the
> loaded kernel happens to be in use for something).

I thought that we had previously established that this wasn't true?

On Fri, 11 Jul, at 01:18:43AM, Michael Brown wrote:
> > Is this actually true? There is code within the EFI boot stub to
> > allocate space for the kernel image and perform the relocation if it's
> > not already suitably aligned.
> > 
> > Or is the above paragraph referring to the previously merged patch?
> 
> The "...headers now include..." part was referring to the previously
> merged patch to add the .bss section.
> 
> I haven't actually looked at the code which performs the alignment; I
> was going on hpa's concern that merely exposing init_size would be
> insufficient due to the potential for alignment.  My understanding
> (possibly incorrect) was that the alignment was carried out using
> something simple along the lines of:
> 
>   new_kernel_start = align ( kernel_start, kernel_alignment );
>   memmove ( new_kernel_start, kernel_start, kernel_len );
> 
> i.e. that the memory used for alignment was not explicitly allocated.
> If the EFI boot stub instead allocates space for the aligned kernel
> using AllocatePages() (and allocates enough space for the whole of
> init_size), then the problem I described does not exist.

To which I replied with,

> Right, this shouldn't be a problem because we do in fact allocate space
> using the EFI boottime services in efi_relocate_kernel(), taking the
> alignment into account, and then perform the kernel image copy.
> 
> I still think your change makes sense, I'm just inclined to delete the
> paragraph referring to the corruption bug (which we've established
> doesn't exist).

Do we still have a bug?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v3] efi: Request desired alignment via the PE/COFF headers

[Michael Brown]

On 19/06/15 13:21, Matt Fleming wrote:
>> If the patch is reverted, then I think it will cause undefined
>> behaviour on some platforms (which happen to load the kernel to
>> non-preferred alignment, and where the memory immediately after the
>> loaded kernel happens to be in use for something).
>
> I thought that we had previously established that this wasn't true?
>
> <snip>
>
> To which I replied with,
>
>> Right, this shouldn't be a problem because we do in fact allocate space
>> using the EFI boottime services in efi_relocate_kernel(), taking the
>> alignment into account, and then perform the kernel image copy.
>>
>> I still think your change makes sense, I'm just inclined to delete the
>> paragraph referring to the corruption bug (which we've established
>> doesn't exist).
>
> Do we still have a bug?

Ah; that was the old e-mail thread I couldn't find!

You are right; this change can safely be reverted.  Sorry for the noise.

Michael

Re: [PATCH v3] efi: Request desired alignment via the PE/COFF headers

[Matt Fleming]

On Fri, 19 Jun, at 01:25:31PM, Michael Brown wrote:
> 
> You are right; this change can safely be reverted.  Sorry for the noise.

Guys, this is what I'm going to queue up for v4.3. My plan is to let it
bake in linux-next for a while, to try and shake out any problems.

Interestingly, I'm not sure that the size of the bzImage is actually
guaranteed to be a multiple of 32, since it appears that
arch/x86/boot/tools/build.c only deals with 16-byte "paragraphs".

---

>From 809ca3d4eac5836e5f48c19f28babff1e5b71c77 Mon Sep 17 00:00:00 2001
From: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Date: Wed, 15 Jul 2015 14:05:53 +0100
Subject: [PATCH] Revert "x86/efi: Request desired alignment via the PE/COFF
 headers"

This reverts commit aeffc4928ea21aab3c7be72f00e257799b661c29.

Linn reports that Signtool complains that kernels built with
CONFIG_EFI_STUB are violating the PE/COFF specification because the
SizeOfImage field is not a multiple of SectionAlignment.

This violation was introduced as an optimisation to skip having the
kernel relocate itself during boot and instead have the firmware place
it at a correctly aligned address.

No one else has complained and I'm not aware of any firmware
implementations that refuse to boot with commit aeffc4928ea2, but it's a
legitimate bug, so revert the offending commit.

Reported-by: Linn Crosetto <linn-VXdhtT5mjnY@public.gmane.org>
Cc: Michael Brown <mbrown-OViyBiuKJBuK421+ScFKDQ@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 arch/x86/boot/header.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
index 16ef02596db2..7a6d43a554d7 100644
--- a/arch/x86/boot/header.S
+++ b/arch/x86/boot/header.S
@@ -154,7 +154,7 @@ extra_header_fields:
 #else
 	.quad	0				# ImageBase
 #endif
-	.long	CONFIG_PHYSICAL_ALIGN		# SectionAlignment
+	.long	0x20				# SectionAlignment
 	.long	0x20				# FileAlignment
 	.word	0				# MajorOperatingSystemVersion
 	.word	0				# MinorOperatingSystemVersion
-- 
2.1.0

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v3] efi: Request desired alignment via the PE/COFF headers

[Linn Crosetto]

On Wed, Jul 15, 2015 at 03:11:19PM +0100, Matt Fleming wrote:
> On Fri, 19 Jun, at 01:25:31PM, Michael Brown wrote:
> > 
> > You are right; this change can safely be reverted.  Sorry for the noise.
> 
> Guys, this is what I'm going to queue up for v4.3. My plan is to let it
> bake in linux-next for a while, to try and shake out any problems.
> 
> Interestingly, I'm not sure that the size of the bzImage is actually
> guaranteed to be a multiple of 32, since it appears that
> arch/x86/boot/tools/build.c only deals with 16-byte "paragraphs".
> 
> ---
> 
> From 809ca3d4eac5836e5f48c19f28babff1e5b71c77 Mon Sep 17 00:00:00 2001
> From: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Date: Wed, 15 Jul 2015 14:05:53 +0100
> Subject: [PATCH] Revert "x86/efi: Request desired alignment via the PE/COFF
>  headers"
> 
> This reverts commit aeffc4928ea21aab3c7be72f00e257799b661c29.
> 
> Linn reports that Signtool complains that kernels built with
> CONFIG_EFI_STUB are violating the PE/COFF specification because the
> SizeOfImage field is not a multiple of SectionAlignment.

Thanks Matt. The actual problem causing the failure in Signtool is that
FileAlignment is less than 512 and not equal to SectionAlignment. Reverting
the commit fixes this problem as well. The spec says that FileAlignment
should be a power of 2 between 512 and 64K, but Signtool accepts other
values when it is equal to SectionAlignment.

> This violation was introduced as an optimisation to skip having the
> kernel relocate itself during boot and instead have the firmware place
> it at a correctly aligned address.
> 
> No one else has complained and I'm not aware of any firmware
> implementations that refuse to boot with commit aeffc4928ea2, but it's a
> legitimate bug, so revert the offending commit.
> 
> Reported-by: Linn Crosetto <linn-VXdhtT5mjnY@public.gmane.org>
> Cc: Michael Brown <mbrown-OViyBiuKJBuK421+ScFKDQ@public.gmane.org>
> Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  arch/x86/boot/header.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
> index 16ef02596db2..7a6d43a554d7 100644
> --- a/arch/x86/boot/header.S
> +++ b/arch/x86/boot/header.S
> @@ -154,7 +154,7 @@ extra_header_fields:
>  #else
>  	.quad	0				# ImageBase
>  #endif
> -	.long	CONFIG_PHYSICAL_ALIGN		# SectionAlignment
> +	.long	0x20				# SectionAlignment
>  	.long	0x20				# FileAlignment
>  	.word	0				# MajorOperatingSystemVersion
>  	.word	0				# MinorOperatingSystemVersion

Reviewed-by: Linn Crosetto <linn-VXdhtT5mjnY@public.gmane.org>

-- 
Linn Crosetto
Linux for HP Helion OpenStack