[PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled

[PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled

[Jeffy Chen]

The zImage file size should be aligned.

Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate PE/COFF sections")
Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

 arch/arm/boot/compressed/vmlinux.lds.S | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
index b38dcef90756..1636fa259577 100644
--- a/arch/arm/boot/compressed/vmlinux.lds.S
+++ b/arch/arm/boot/compressed/vmlinux.lds.S
@@ -70,10 +70,6 @@ SECTIONS
   .got			: { *(.got) }
   _got_end = .;
 
-  /* ensure the zImage file size is always a multiple of 64 bits */
-  /* (without a dummy byte, ld just ignores the empty section) */
-  .pad			: { BYTE(0); . = ALIGN(8); }
-
 #ifdef CONFIG_EFI_STUB
   .data : ALIGN(4096) {
     __pecoff_data_start = .;
@@ -93,6 +89,10 @@ SECTIONS
   __pecoff_data_rawsize = . - ADDR(.data);
 #endif
 
+  /* ensure the zImage file size is always a multiple of 64 bits */
+  /* (without a dummy byte, ld just ignores the empty section) */
+  .pad			: { BYTE(0); . = ALIGN(8); }
+
   _edata = .;
 
   _magic_sig = ZIMAGE_MAGIC(0x016f2818);
-- 
2.11.0

[PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled

[Chris Zhong]

Tested-by: Chris Zhong <zyw@rock-chips.com>


On Wednesday, October 18, 2017 01:01 PM, Jeffy Chen wrote:
> The zImage file size should be aligned.
>
> Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate PE/COFF sections")
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
>
>   arch/arm/boot/compressed/vmlinux.lds.S | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
> index b38dcef90756..1636fa259577 100644
> --- a/arch/arm/boot/compressed/vmlinux.lds.S
> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> @@ -70,10 +70,6 @@ SECTIONS
>     .got			: { *(.got) }
>     _got_end = .;
>   
> -  /* ensure the zImage file size is always a multiple of 64 bits */
> -  /* (without a dummy byte, ld just ignores the empty section) */
> -  .pad			: { BYTE(0); . = ALIGN(8); }
> -
>   #ifdef CONFIG_EFI_STUB
>     .data : ALIGN(4096) {
>       __pecoff_data_start = .;
> @@ -93,6 +89,10 @@ SECTIONS
>     __pecoff_data_rawsize = . - ADDR(.data);
>   #endif
>   
> +  /* ensure the zImage file size is always a multiple of 64 bits */
> +  /* (without a dummy byte, ld just ignores the empty section) */
> +  .pad			: { BYTE(0); . = ALIGN(8); }
> +
>     _edata = .;
>   
>     _magic_sig = ZIMAGE_MAGIC(0x016f2818);

-- 
Chris Zhong

[PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled

[Ard Biesheuvel]

On 18 October 2017 at 06:01, Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
> The zImage file size should be aligned.
>
> Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate PE/COFF sections")
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
>
>  arch/arm/boot/compressed/vmlinux.lds.S | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
> index b38dcef90756..1636fa259577 100644
> --- a/arch/arm/boot/compressed/vmlinux.lds.S
> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> @@ -70,10 +70,6 @@ SECTIONS
>    .got                 : { *(.got) }
>    _got_end = .;
>
> -  /* ensure the zImage file size is always a multiple of 64 bits */
> -  /* (without a dummy byte, ld just ignores the empty section) */
> -  .pad                 : { BYTE(0); . = ALIGN(8); }
> -
>  #ifdef CONFIG_EFI_STUB
>    .data : ALIGN(4096) {
>      __pecoff_data_start = .;
> @@ -93,6 +89,10 @@ SECTIONS
>    __pecoff_data_rawsize = . - ADDR(.data);
>  #endif
>
> +  /* ensure the zImage file size is always a multiple of 64 bits */
> +  /* (without a dummy byte, ld just ignores the empty section) */
> +  .pad                 : { BYTE(0); . = ALIGN(8); }
> +
>    _edata = .;
>
>    _magic_sig = ZIMAGE_MAGIC(0x016f2818);
> --
> 2.11.0
>

This is not the right fix. If CONFIG_EFI_STUB is enabled, the zImage
filesize should be rounded up to 512 bytes not 8 bytes. The '. =
ALIGN(512);' in the .data section appears to ensure that, but for some
reason, that appears not to be working.

Could you share the output of

$ readelf -S arch/arm/boot/compressed/vmlinux

please? And could you please check whether this patch

https://marc.info/?l=linux-arm-kernel&m=150488477807353

fixes the issue for you?

Thanks,
Ard.

[PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled

[Russell King - ARM Linux]

On Sun, Oct 22, 2017 at 12:01:13PM +0100, Ard Biesheuvel wrote:
> On 18 October 2017 at 06:01, Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
> > The zImage file size should be aligned.
> >
> > Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate PE/COFF sections")
> > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> > ---
> >
> >  arch/arm/boot/compressed/vmlinux.lds.S | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
> > index b38dcef90756..1636fa259577 100644
> > --- a/arch/arm/boot/compressed/vmlinux.lds.S
> > +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> > @@ -70,10 +70,6 @@ SECTIONS
> >    .got                 : { *(.got) }
> >    _got_end = .;
> >
> > -  /* ensure the zImage file size is always a multiple of 64 bits */
> > -  /* (without a dummy byte, ld just ignores the empty section) */
> > -  .pad                 : { BYTE(0); . = ALIGN(8); }
> > -
> >  #ifdef CONFIG_EFI_STUB
> >    .data : ALIGN(4096) {
> >      __pecoff_data_start = .;
> > @@ -93,6 +89,10 @@ SECTIONS
> >    __pecoff_data_rawsize = . - ADDR(.data);
> >  #endif
> >
> > +  /* ensure the zImage file size is always a multiple of 64 bits */
> > +  /* (without a dummy byte, ld just ignores the empty section) */
> > +  .pad                 : { BYTE(0); . = ALIGN(8); }
> > +
> >    _edata = .;
> >
> >    _magic_sig = ZIMAGE_MAGIC(0x016f2818);
> > --
> > 2.11.0
> >
> 
> This is not the right fix. If CONFIG_EFI_STUB is enabled, the zImage
> filesize should be rounded up to 512 bytes not 8 bytes. The '. =
> ALIGN(512);' in the .data section appears to ensure that, but for some
> reason, that appears not to be working.

Actually, the existing .pad section is totally and utterly bogus when
EFI is enabled:

  . = ALIGN(4);
  _etext = .;

  .got.plt              : { *(.got.plt) }
  _got_start = .;
  .got                  : { *(.got) }
  _got_end = .;

The .got.plt and .got are always word-based.  This is then followed by
.pad, which does nothing but pad out to a multiple of 64 bit:

  /* ensure the zImage file size is always a multiple of 64 bits */
  /* (without a dummy byte, ld just ignores the empty section) */
  .pad                  : { BYTE(0); . = ALIGN(8); }

So this may add zero or 4 bytes of padding.

This is then followed by the EFI data:

  .data : ALIGN(4096) {
  ...
    . = ALIGN(512);
  }

which is aligned to 4K but aligns the end of itself to 512.

So, we have the end of .got aligned to 4, followed by .pad that tries to
align to 8, followed by an optional .data section.  This is pointless.

A sane patch would be to choose between the EFI .data section and the
.pad section.  So, it should be:

#ifdef CONFIG_EFI_STUB
   .data : ALIGN(4096) {
   ...
     . = ALIGN(512);
   }
#else
   .pad                 : { BYTE(0); . = ALIGN(8); }
#endif

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

[PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled

[Ard Biesheuvel]

On 22 October 2017 at 13:47, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Sun, Oct 22, 2017 at 12:01:13PM +0100, Ard Biesheuvel wrote:
>> On 18 October 2017 at 06:01, Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
>> > The zImage file size should be aligned.
>> >
>> > Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate PE/COFF sections")
>> > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>> > ---
>> >
>> >  arch/arm/boot/compressed/vmlinux.lds.S | 8 ++++----
>> >  1 file changed, 4 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
>> > index b38dcef90756..1636fa259577 100644
>> > --- a/arch/arm/boot/compressed/vmlinux.lds.S
>> > +++ b/arch/arm/boot/compressed/vmlinux.lds.S
>> > @@ -70,10 +70,6 @@ SECTIONS
>> >    .got                 : { *(.got) }
>> >    _got_end = .;
>> >
>> > -  /* ensure the zImage file size is always a multiple of 64 bits */
>> > -  /* (without a dummy byte, ld just ignores the empty section) */
>> > -  .pad                 : { BYTE(0); . = ALIGN(8); }
>> > -
>> >  #ifdef CONFIG_EFI_STUB
>> >    .data : ALIGN(4096) {
>> >      __pecoff_data_start = .;
>> > @@ -93,6 +89,10 @@ SECTIONS
>> >    __pecoff_data_rawsize = . - ADDR(.data);
>> >  #endif
>> >
>> > +  /* ensure the zImage file size is always a multiple of 64 bits */
>> > +  /* (without a dummy byte, ld just ignores the empty section) */
>> > +  .pad                 : { BYTE(0); . = ALIGN(8); }
>> > +
>> >    _edata = .;
>> >
>> >    _magic_sig = ZIMAGE_MAGIC(0x016f2818);
>> > --
>> > 2.11.0
>> >
>>
>> This is not the right fix. If CONFIG_EFI_STUB is enabled, the zImage
>> filesize should be rounded up to 512 bytes not 8 bytes. The '. =
>> ALIGN(512);' in the .data section appears to ensure that, but for some
>> reason, that appears not to be working.
>
> Actually, the existing .pad section is totally and utterly bogus when
> EFI is enabled:
>
>   . = ALIGN(4);
>   _etext = .;
>
>   .got.plt              : { *(.got.plt) }
>   _got_start = .;
>   .got                  : { *(.got) }
>   _got_end = .;
>
> The .got.plt and .got are always word-based.  This is then followed by
> .pad, which does nothing but pad out to a multiple of 64 bit:
>
>   /* ensure the zImage file size is always a multiple of 64 bits */
>   /* (without a dummy byte, ld just ignores the empty section) */
>   .pad                  : { BYTE(0); . = ALIGN(8); }
>
> So this may add zero or 4 bytes of padding.
>
> This is then followed by the EFI data:
>
>   .data : ALIGN(4096) {
>   ...
>     . = ALIGN(512);
>   }
>
> which is aligned to 4K but aligns the end of itself to 512.
>
> So, we have the end of .got aligned to 4, followed by .pad that tries to
> align to 8, followed by an optional .data section.  This is pointless.
>
> A sane patch would be to choose between the EFI .data section and the
> .pad section.  So, it should be:
>
> #ifdef CONFIG_EFI_STUB
>    .data : ALIGN(4096) {
>    ...
>      . = ALIGN(512);
>    }
> #else
>    .pad                 : { BYTE(0); . = ALIGN(8); }
> #endif
>

Agreed, the .pad section has no point for EFI_STUB=y. However, it
seems this symptom is caused by the same issues I am trying to address
here

https://marc.info/?l=linux-arm-kernel&m=150488477807353

which is that we have __ksymtab_xxx sections that we should discard,
because the linker will otherwise emit them /after/ .data or .pad.
This is caused by the use of lib/sort.c in the EFI stub, which
contains an EXPORT_SYMBOL().

Would you perhaps prefer that I clone sort.c into its own .c file
specifically for the EFI stub? (under drivers/firmware/efi/libstub)
That should get rid of these spurious sections and thus the
misalignments and/or movements that are causing all of these issues.

[PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled

[jeffy]

Hi Ard,

On 10/22/2017 09:01 PM, Ard Biesheuvel wrote:
> On 22 October 2017 at 13:47, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
>> On Sun, Oct 22, 2017 at 12:01:13PM +0100, Ard Biesheuvel wrote:
>>> On 18 October 2017 at 06:01, Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
>>>> The zImage file size should be aligned.
>>>>
>>>> Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate PE/COFF sections")
>>>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>>>> ---
>>>>
>>>>   arch/arm/boot/compressed/vmlinux.lds.S | 8 ++++----
>>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
>>>> index b38dcef90756..1636fa259577 100644
>>>> --- a/arch/arm/boot/compressed/vmlinux.lds.S
>>>> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
>>>> @@ -70,10 +70,6 @@ SECTIONS
>>>>     .got                 : { *(.got) }
>>>>     _got_end = .;
>>>>
>>>> -  /* ensure the zImage file size is always a multiple of 64 bits */
>>>> -  /* (without a dummy byte, ld just ignores the empty section) */
>>>> -  .pad                 : { BYTE(0); . = ALIGN(8); }
>>>> -
>>>>   #ifdef CONFIG_EFI_STUB
>>>>     .data : ALIGN(4096) {
>>>>       __pecoff_data_start = .;
>>>> @@ -93,6 +89,10 @@ SECTIONS
>>>>     __pecoff_data_rawsize = . - ADDR(.data);
>>>>   #endif
>>>>
>>>> +  /* ensure the zImage file size is always a multiple of 64 bits */
>>>> +  /* (without a dummy byte, ld just ignores the empty section) */
>>>> +  .pad                 : { BYTE(0); . = ALIGN(8); }
>>>> +
>>>>     _edata = .;
>>>>
>>>>     _magic_sig = ZIMAGE_MAGIC(0x016f2818);
>>>> --
>>>> 2.11.0
>>>>
>>>
>>> This is not the right fix. If CONFIG_EFI_STUB is enabled, the zImage
>>> filesize should be rounded up to 512 bytes not 8 bytes. The '. =
>>> ALIGN(512);' in the .data section appears to ensure that, but for some
>>> reason, that appears not to be working.
>>
>> Actually, the existing .pad section is totally and utterly bogus when
>> EFI is enabled:
>>
>>    . = ALIGN(4);
>>    _etext = .;
>>
>>    .got.plt              : { *(.got.plt) }
>>    _got_start = .;
>>    .got                  : { *(.got) }
>>    _got_end = .;
>>
>> The .got.plt and .got are always word-based.  This is then followed by
>> .pad, which does nothing but pad out to a multiple of 64 bit:
>>
>>    /* ensure the zImage file size is always a multiple of 64 bits */
>>    /* (without a dummy byte, ld just ignores the empty section) */
>>    .pad                  : { BYTE(0); . = ALIGN(8); }
>>
>> So this may add zero or 4 bytes of padding.
>>
>> This is then followed by the EFI data:
>>
>>    .data : ALIGN(4096) {
>>    ...
>>      . = ALIGN(512);
>>    }
>>
>> which is aligned to 4K but aligns the end of itself to 512.
>>
>> So, we have the end of .got aligned to 4, followed by .pad that tries to
>> align to 8, followed by an optional .data section.  This is pointless.
>>
>> A sane patch would be to choose between the EFI .data section and the
>> .pad section.  So, it should be:
>>
>> #ifdef CONFIG_EFI_STUB
>>     .data : ALIGN(4096) {
>>     ...
>>       . = ALIGN(512);
>>     }
>> #else
>>     .pad                 : { BYTE(0); . = ALIGN(8); }
>> #endif
>>
>
> Agreed, the .pad section has no point for EFI_STUB=y. However, it
> seems this symptom is caused by the same issues I am trying to address
> here
>
> https://marc.info/?l=linux-arm-kernel&m=150488477807353
>
> which is that we have __ksymtab_xxx sections that we should discard,
> because the linker will otherwise emit them /after/ .data or .pad.
> This is caused by the use of lib/sort.c in the EFI stub, which
> contains an EXPORT_SYMBOL().

hmm, right, didn't notice the data is already aligned...
so it's indeed caused by the ksym:

   [ 9] .data             PROGBITS        006ce000 6d6000 000200 00  WA 
  0   0 4096
   [10] ___ksymtab+sort   PROGBITS        006ce200 6d6200 000008 00  WA 
  0   0  4
   [11] .bss              NOBITS          006ce208 6d6208 00001c 00  WA 
  0   0  4


and both of your old([PATCH] ARM: compressed: discard ksym/kcrctab input 
section) and new([PATCH] efi/libstub: arm: omit sorting of the UEFI 
memory map) patches fix the issue i meet, thanks:)

>
> Would you perhaps prefer that I clone sort.c into its own .c file
> specifically for the EFI stub? (under drivers/firmware/efi/libstub)
> That should get rid of these spurious sections and thus the
> misalignments and/or movements that are causing all of these issues.
>
>
>

[PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled

[Russell King - ARM Linux]

On Mon, Oct 23, 2017 at 11:26:49AM +0800, jeffy wrote:
> Hi Ard,
> 
> On 10/22/2017 09:01 PM, Ard Biesheuvel wrote:
> >On 22 October 2017 at 13:47, Russell King - ARM Linux
> ><linux@armlinux.org.uk> wrote:
> >>On Sun, Oct 22, 2017 at 12:01:13PM +0100, Ard Biesheuvel wrote:
> >>>On 18 October 2017 at 06:01, Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
> >>>>The zImage file size should be aligned.
> >>>>
> >>>>Fixes: e4bae4d0b5f3 ("arm/efi: Split zImage code and data into separate PE/COFF sections")
> >>>>Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> >>>>---
> >>>>
> >>>>  arch/arm/boot/compressed/vmlinux.lds.S | 8 ++++----
> >>>>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>>>
> >>>>diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
> >>>>index b38dcef90756..1636fa259577 100644
> >>>>--- a/arch/arm/boot/compressed/vmlinux.lds.S
> >>>>+++ b/arch/arm/boot/compressed/vmlinux.lds.S
> >>>>@@ -70,10 +70,6 @@ SECTIONS
> >>>>    .got                 : { *(.got) }
> >>>>    _got_end = .;
> >>>>
> >>>>-  /* ensure the zImage file size is always a multiple of 64 bits */
> >>>>-  /* (without a dummy byte, ld just ignores the empty section) */
> >>>>-  .pad                 : { BYTE(0); . = ALIGN(8); }
> >>>>-
> >>>>  #ifdef CONFIG_EFI_STUB
> >>>>    .data : ALIGN(4096) {
> >>>>      __pecoff_data_start = .;
> >>>>@@ -93,6 +89,10 @@ SECTIONS
> >>>>    __pecoff_data_rawsize = . - ADDR(.data);
> >>>>  #endif
> >>>>
> >>>>+  /* ensure the zImage file size is always a multiple of 64 bits */
> >>>>+  /* (without a dummy byte, ld just ignores the empty section) */
> >>>>+  .pad                 : { BYTE(0); . = ALIGN(8); }
> >>>>+
> >>>>    _edata = .;
> >>>>
> >>>>    _magic_sig = ZIMAGE_MAGIC(0x016f2818);
> >>>>--
> >>>>2.11.0
> >>>>
> >>>
> >>>This is not the right fix. If CONFIG_EFI_STUB is enabled, the zImage
> >>>filesize should be rounded up to 512 bytes not 8 bytes. The '. =
> >>>ALIGN(512);' in the .data section appears to ensure that, but for some
> >>>reason, that appears not to be working.
> >>
> >>Actually, the existing .pad section is totally and utterly bogus when
> >>EFI is enabled:
> >>
> >>   . = ALIGN(4);
> >>   _etext = .;
> >>
> >>   .got.plt              : { *(.got.plt) }
> >>   _got_start = .;
> >>   .got                  : { *(.got) }
> >>   _got_end = .;
> >>
> >>The .got.plt and .got are always word-based.  This is then followed by
> >>.pad, which does nothing but pad out to a multiple of 64 bit:
> >>
> >>   /* ensure the zImage file size is always a multiple of 64 bits */
> >>   /* (without a dummy byte, ld just ignores the empty section) */
> >>   .pad                  : { BYTE(0); . = ALIGN(8); }
> >>
> >>So this may add zero or 4 bytes of padding.
> >>
> >>This is then followed by the EFI data:
> >>
> >>   .data : ALIGN(4096) {
> >>   ...
> >>     . = ALIGN(512);
> >>   }
> >>
> >>which is aligned to 4K but aligns the end of itself to 512.
> >>
> >>So, we have the end of .got aligned to 4, followed by .pad that tries to
> >>align to 8, followed by an optional .data section.  This is pointless.
> >>
> >>A sane patch would be to choose between the EFI .data section and the
> >>.pad section.  So, it should be:
> >>
> >>#ifdef CONFIG_EFI_STUB
> >>    .data : ALIGN(4096) {
> >>    ...
> >>      . = ALIGN(512);
> >>    }
> >>#else
> >>    .pad                 : { BYTE(0); . = ALIGN(8); }
> >>#endif
> >>
> >
> >Agreed, the .pad section has no point for EFI_STUB=y. However, it
> >seems this symptom is caused by the same issues I am trying to address
> >here
> >
> >https://marc.info/?l=linux-arm-kernel&m=150488477807353
> >
> >which is that we have __ksymtab_xxx sections that we should discard,
> >because the linker will otherwise emit them /after/ .data or .pad.
> >This is caused by the use of lib/sort.c in the EFI stub, which
> >contains an EXPORT_SYMBOL().
> 
> hmm, right, didn't notice the data is already aligned...
> so it's indeed caused by the ksym:
> 
>   [ 9] .data             PROGBITS        006ce000 6d6000 000200 00  WA  0
> 0 4096
>   [10] ___ksymtab+sort   PROGBITS        006ce200 6d6200 000008 00  WA  0
> 0  4
>   [11] .bss              NOBITS          006ce208 6d6208 00001c 00  WA  0
> 0  4

It's earlier - look for __ksymtab_strings.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

[PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled

[jeffy]

Hi Russell,

Thanks for your reply.

On 10/23/2017 04:50 PM, Russell King - ARM Linux wrote:
>> >
>> >hmm, right, didn't notice the data is already aligned...
>> >so it's indeed caused by the ksym:
>> >
>> >   [ 9] .data             PROGBITS        006ce000 6d6000 000200 00  WA  0
>> >0 4096
>> >   [10] ___ksymtab+sort   PROGBITS        006ce200 6d6200 000008 00  WA  0
>> >0  4
>> >   [11] .bss              NOBITS          006ce208 6d6208 00001c 00  WA  0
>> >0  4
> It's earlier - look for __ksymtab_strings.

the problem i meet is the appended dtb code found dtb invalid. i thought 
that is because of unaligned zImage size, but i was wrong...


it looks like the size is still aligned, and after add more logs, it 
seems the problem is due to _edata not matched the real file size, which 
is because of the unexpected ___ksymtab+sort:

currently:
zImage size is 6d6208:
-rwxr-xr-x 1 root root 7135752 Oct 23 18:12 zImage

_edata is 006ce200:
  006ce200     0 NOTYPE  GLOBAL DEFAULT    9 _edata

vmlinux sections:
Section Headers:
   [Nr] Name              Type            Addr     Off    Size   ES Flg 
Lk Inf Al
   [ 0]                   NULL            00000000 000000 000000 00 
  0   0  0
   [ 1] .text             PROGBITS        00000000 008000 00b7a0 00  AX 
  0   0 4096
   [ 2] .table            PROGBITS        0000b7a0 0137a0 000014 00  WA 
  0   0  4
   [ 3] .rodata           PROGBITS        0000b7b4 0137b4 0015ef 00   A 
  0   0  2
   [ 4] __ksymtab_strings PROGBITS        0000cda3 014da3 000005 00   A 
  0   0  1
   [ 5] .piggydata        PROGBITS        0000cda8 014da8 6c026f 00   A 
  0   0  1
   [ 6] .got.plt          PROGBITS        006cd018 6d5018 00000c 04  WA 
  0   0  4
   [ 7] .got              PROGBITS        006cd024 6d5024 000028 00  WA 
  0   0  4
   [ 8] .pad              PROGBITS        006cd04c 6d504c 000004 00  WA 
  0   0  1
   [ 9] .data             PROGBITS        006ce000 6d6000 000200 00  WA 
  0   0 4096
   [10] ___ksymtab+sort   PROGBITS        006ce200 6d6200 000008 00  WA 
  0   0  4
   [11] .bss              NOBITS          006ce208 6d6208 00001c 00  WA 
  0   0  4





and it turns out moving around .pad section only hide the problem by 
placing the .pad after the ___ksymtab+sort:

Section Headers:
   [Nr] Name              Type            Addr     Off    Size   ES Flg 
Lk Inf Al
   [ 0]                   NULL            00000000 000000 000000 00 
  0   0  0
   [ 1] .text             PROGBITS        00000000 008000 00b7a0 00  AX 
  0   0 4096
   [ 2] .table            PROGBITS        0000b7a0 0137a0 000014 00  WA 
  0   0  4
   [ 3] .rodata           PROGBITS        0000b7b4 0137b4 0015ef 00   A 
  0   0  2
   [ 4] __ksymtab_strings PROGBITS        0000cda3 014da3 000005 00   A 
  0   0  1
   [ 5] .piggydata        PROGBITS        0000cda8 014da8 6c026f 00   A 
  0   0  1
   [ 6] .got.plt          PROGBITS        006cd018 6d5018 00000c 04  WA 
  0   0  4
   [ 7] .got              PROGBITS        006cd024 6d5024 000028 00  WA 
  0   0  4
   [ 8] .data             PROGBITS        006ce000 6d6000 000200 00  WA 
  0   0 4096
   [ 9] ___ksymtab+sort   PROGBITS        006ce200 6d6200 000008 00  WA 
  0   0  4
   [10] .pad              PROGBITS        006ce208 6d6208 000008 00  WA 
  0   0  1
   [11] .bss              NOBITS          006ce210 6d6210 00001c 00  WA 
  0   0  4

-rwxr-xr-x 1 root root 7135760 Oct 23 18:09 zImage

  006ce210     0 NOTYPE  GLOBAL DEFAULT   10 _edata




and i think Ard's new patch could be the right way to fix it :)

[PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled

[Russell King - ARM Linux]

On Mon, Oct 23, 2017 at 06:24:12PM +0800, jeffy wrote:
> Hi Russell,
> 
> Thanks for your reply.
> 
> On 10/23/2017 04:50 PM, Russell King - ARM Linux wrote:
> >>>
> >>>hmm, right, didn't notice the data is already aligned...
> >>>so it's indeed caused by the ksym:
> >>>
> >>>   [ 9] .data             PROGBITS        006ce000 6d6000 000200 00  WA  0
> >>>0 4096
> >>>   [10] ___ksymtab+sort   PROGBITS        006ce200 6d6200 000008 00  WA  0
> >>>0  4
> >>>   [11] .bss              NOBITS          006ce208 6d6208 00001c 00  WA  0
> >>>0  4
> >It's earlier - look for __ksymtab_strings.
> 
> the problem i meet is the appended dtb code found dtb invalid. i thought
> that is because of unaligned zImage size, but i was wrong...

Hmm, you really ought not to be using the appended dtb code for modern
systems - the appended dtb system is there for old boot loaders that
are incapable of dealing with a dtb.  As is said in the option's help
text:

  This is meant as a backward compatibility convenience for those
  systems with a bootloader that can't be upgraded to accommodate
  the documented boot protocol using a device tree.

  Beware that there is very little in terms of protection against
  this option being confused by leftover garbage in memory that might
  look like a DTB header after a reboot if no actual DTB is appended
  to zImage.  Do not leave this option active in a production kernel
  if you don't intend to always append a DTB.  Proper passing of the
  location into r2 of a bootloader provided DTB is always preferable
  to this option.

If you rely on it, and you have something that looks like a dtb after
the image, then things will go wrong, so it's better _not_ to use it
and to keep it disabled.

That aside, thanks for doing a more in-depth analysis of what is going
on, which helps to understand /why/ Ard's fix works (whereas before
it was rather nebulous.)

I wonder whether we ought to tell the linker to discard any unknown
sections by adding at the bottom:

	/DISCARD/ { *(*) }

but I do think we need to document this, specifically that _edata must
point to the first byte after the binary file, and that the only
sections after it are allowed to be the .bss and stack sections.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

[PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled

[Russell King - ARM Linux]

On Mon, Oct 23, 2017 at 11:50:46AM +0100, Russell King - ARM Linux wrote:
> On Mon, Oct 23, 2017 at 06:24:12PM +0800, jeffy wrote:
> > Hi Russell,
> > 
> > Thanks for your reply.
> > 
> > On 10/23/2017 04:50 PM, Russell King - ARM Linux wrote:
> > >>>
> > >>>hmm, right, didn't notice the data is already aligned...
> > >>>so it's indeed caused by the ksym:
> > >>>
> > >>>   [ 9] .data             PROGBITS        006ce000 6d6000 000200 00  WA  0
> > >>>0 4096
> > >>>   [10] ___ksymtab+sort   PROGBITS        006ce200 6d6200 000008 00  WA  0
> > >>>0  4
> > >>>   [11] .bss              NOBITS          006ce208 6d6208 00001c 00  WA  0
> > >>>0  4
> > >It's earlier - look for __ksymtab_strings.
> > 
> > the problem i meet is the appended dtb code found dtb invalid. i thought
> > that is because of unaligned zImage size, but i was wrong...
> 
> Hmm, you really ought not to be using the appended dtb code for modern
> systems - the appended dtb system is there for old boot loaders that
> are incapable of dealing with a dtb.  As is said in the option's help
> text:
> 
>   This is meant as a backward compatibility convenience for those
>   systems with a bootloader that can't be upgraded to accommodate
>   the documented boot protocol using a device tree.
> 
>   Beware that there is very little in terms of protection against
>   this option being confused by leftover garbage in memory that might
>   look like a DTB header after a reboot if no actual DTB is appended
>   to zImage.  Do not leave this option active in a production kernel
>   if you don't intend to always append a DTB.  Proper passing of the
>   location into r2 of a bootloader provided DTB is always preferable
>   to this option.
> 
> If you rely on it, and you have something that looks like a dtb after
> the image, then things will go wrong, so it's better _not_ to use it
> and to keep it disabled.
> 
> That aside, thanks for doing a more in-depth analysis of what is going
> on, which helps to understand /why/ Ard's fix works (whereas before
> it was rather nebulous.)
> 
> I wonder whether we ought to tell the linker to discard any unknown
> sections by adding at the bottom:
> 
> 	/DISCARD/ { *(*) }
> 
> but I do think we need to document this, specifically that _edata must
> point to the first byte after the binary file, and that the only
> sections after it are allowed to be the .bss and stack sections.

Short of adding the discard (which I think is itself risky, we've had
problems in the main kernel's vmlinux.lds in this area), I think we
ought to verify the size of the zImage file, so that the build fails
when we generate a zImage which is wrong, rather than producing a
zImage that is incorrect.  Maybe something like this?

8<=====
From: Russell King <rmk+kernel@armlinux.org.uk>
Subject: [PATCH] ARM: verify size of zImage

The linker can sometimes add additional sections to the zImage ELF file
which results in the zImage binary being larger than expected.  This
causes appended DT blobs to fail.

Verify that the zImage binary is the expected size, and fail the build
if this is not the case.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 arch/arm/boot/Makefile         |  6 +++++-
 arch/arm/boot/verify_zimage.sh | 21 +++++++++++++++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)
 create mode 100755 arch/arm/boot/verify_zimage.sh

diff --git a/arch/arm/boot/Makefile b/arch/arm/boot/Makefile
index a3af4dc08c3e..1290875556ae 100644
--- a/arch/arm/boot/Makefile
+++ b/arch/arm/boot/Makefile
@@ -53,6 +53,10 @@ $(obj)/Image $(obj)/zImage: FORCE
 
 else
 
+quiet_cmd_mkzimage = ZIMAGE  $@
+cmd_mkzimage = $(cmd_objcopy) && $(CONFIG_SHELL) -c \
+	'$(srctree)/$(src)/verify_zimage.sh $< $@ "$(NM)" || { rm -f $@; false; }'
+
 $(obj)/xipImage: FORCE
 	@echo 'Kernel not configured for XIP (CONFIG_XIP_KERNEL!=y)'
 	@false
@@ -64,7 +68,7 @@ $(obj)/compressed/vmlinux: $(obj)/Image FORCE
 	$(Q)$(MAKE) $(build)=$(obj)/compressed $@
 
 $(obj)/zImage:	$(obj)/compressed/vmlinux FORCE
-	$(call if_changed,objcopy)
+	$(call if_changed,mkzimage)
 
 endif
 
diff --git a/arch/arm/boot/verify_zimage.sh b/arch/arm/boot/verify_zimage.sh
new file mode 100755
index 000000000000..922a93e61aa7
--- /dev/null
+++ b/arch/arm/boot/verify_zimage.sh
@@ -0,0 +1,21 @@
+#!/bin/sh
+set -e
+
+vmlinuz="$1"
+zimage="$2"
+nm="$3"
+
+magic_size=$("$nm" "$vmlinuz" | perl -e 'while (<>) {
+	$magic_start = hex($1) if /^([[:xdigit:]]+) . _magic_start$/;
+	$magic_end = hex($1) if /^([[:xdigit:]]+) . _magic_end$/;
+}; printf "%d\n", $magic_end - $magic_start;')
+
+zimage_size=$(stat -c '%s' "$zimage")
+
+# Verify that the resulting binary matches the size contained within
+# the binary (iow, the linker has not added any additional sections.)
+if [ $magic_size -ne $zimage_size ]; then
+   echo "zImage size ($zimage_size) disagrees with linked size ($magic_size)" >&2
+   exit 1
+fi
+exit 0
-- 
2.7.4


-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

[PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled

[Ard Biesheuvel]

On 23 October 2017 at 12:45, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Mon, Oct 23, 2017 at 11:50:46AM +0100, Russell King - ARM Linux wrote:
>> On Mon, Oct 23, 2017 at 06:24:12PM +0800, jeffy wrote:
>> > Hi Russell,
>> >
>> > Thanks for your reply.
>> >
>> > On 10/23/2017 04:50 PM, Russell King - ARM Linux wrote:
>> > >>>
>> > >>>hmm, right, didn't notice the data is already aligned...
>> > >>>so it's indeed caused by the ksym:
>> > >>>
>> > >>>   [ 9] .data             PROGBITS        006ce000 6d6000 000200 00  WA  0
>> > >>>0 4096
>> > >>>   [10] ___ksymtab+sort   PROGBITS        006ce200 6d6200 000008 00  WA  0
>> > >>>0  4
>> > >>>   [11] .bss              NOBITS          006ce208 6d6208 00001c 00  WA  0
>> > >>>0  4
>> > >It's earlier - look for __ksymtab_strings.
>> >
>> > the problem i meet is the appended dtb code found dtb invalid. i thought
>> > that is because of unaligned zImage size, but i was wrong...
>>
>> Hmm, you really ought not to be using the appended dtb code for modern
>> systems - the appended dtb system is there for old boot loaders that
>> are incapable of dealing with a dtb.  As is said in the option's help
>> text:
>>
>>   This is meant as a backward compatibility convenience for those
>>   systems with a bootloader that can't be upgraded to accommodate
>>   the documented boot protocol using a device tree.
>>
>>   Beware that there is very little in terms of protection against
>>   this option being confused by leftover garbage in memory that might
>>   look like a DTB header after a reboot if no actual DTB is appended
>>   to zImage.  Do not leave this option active in a production kernel
>>   if you don't intend to always append a DTB.  Proper passing of the
>>   location into r2 of a bootloader provided DTB is always preferable
>>   to this option.
>>
>> If you rely on it, and you have something that looks like a dtb after
>> the image, then things will go wrong, so it's better _not_ to use it
>> and to keep it disabled.
>>
>> That aside, thanks for doing a more in-depth analysis of what is going
>> on, which helps to understand /why/ Ard's fix works (whereas before
>> it was rather nebulous.)
>>
>> I wonder whether we ought to tell the linker to discard any unknown
>> sections by adding at the bottom:
>>
>>       /DISCARD/ { *(*) }
>>
>> but I do think we need to document this, specifically that _edata must
>> point to the first byte after the binary file, and that the only
>> sections after it are allowed to be the .bss and stack sections.
>
> Short of adding the discard (which I think is itself risky, we've had
> problems in the main kernel's vmlinux.lds in this area), I think we
> ought to verify the size of the zImage file, so that the build fails
> when we generate a zImage which is wrong, rather than producing a
> zImage that is incorrect.  Maybe something like this?
>
> 8<=====
> From: Russell King <rmk+kernel@armlinux.org.uk>
> Subject: [PATCH] ARM: verify size of zImage
>
> The linker can sometimes add additional sections to the zImage ELF file
> which results in the zImage binary being larger than expected.  This
> causes appended DT blobs to fail.
>
> Verify that the zImage binary is the expected size, and fail the build
> if this is not the case.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  arch/arm/boot/Makefile         |  6 +++++-
>  arch/arm/boot/verify_zimage.sh | 21 +++++++++++++++++++++
>  2 files changed, 26 insertions(+), 1 deletion(-)
>  create mode 100755 arch/arm/boot/verify_zimage.sh
>
> diff --git a/arch/arm/boot/Makefile b/arch/arm/boot/Makefile
> index a3af4dc08c3e..1290875556ae 100644
> --- a/arch/arm/boot/Makefile
> +++ b/arch/arm/boot/Makefile
> @@ -53,6 +53,10 @@ $(obj)/Image $(obj)/zImage: FORCE
>
>  else
>
> +quiet_cmd_mkzimage = ZIMAGE  $@
> +cmd_mkzimage = $(cmd_objcopy) && $(CONFIG_SHELL) -c \
> +       '$(srctree)/$(src)/verify_zimage.sh $< $@ "$(NM)" || { rm -f $@; false; }'
> +
>  $(obj)/xipImage: FORCE
>         @echo 'Kernel not configured for XIP (CONFIG_XIP_KERNEL!=y)'
>         @false
> @@ -64,7 +68,7 @@ $(obj)/compressed/vmlinux: $(obj)/Image FORCE
>         $(Q)$(MAKE) $(build)=$(obj)/compressed $@
>
>  $(obj)/zImage: $(obj)/compressed/vmlinux FORCE
> -       $(call if_changed,objcopy)
> +       $(call if_changed,mkzimage)
>
>  endif
>
> diff --git a/arch/arm/boot/verify_zimage.sh b/arch/arm/boot/verify_zimage.sh
> new file mode 100755
> index 000000000000..922a93e61aa7
> --- /dev/null
> +++ b/arch/arm/boot/verify_zimage.sh
> @@ -0,0 +1,21 @@
> +#!/bin/sh
> +set -e
> +
> +vmlinuz="$1"
> +zimage="$2"
> +nm="$3"
> +
> +magic_size=$("$nm" "$vmlinuz" | perl -e 'while (<>) {
> +       $magic_start = hex($1) if /^([[:xdigit:]]+) . _magic_start$/;
> +       $magic_end = hex($1) if /^([[:xdigit:]]+) . _magic_end$/;
> +}; printf "%d\n", $magic_end - $magic_start;')
> +
> +zimage_size=$(stat -c '%s' "$zimage")
> +
> +# Verify that the resulting binary matches the size contained within
> +# the binary (iow, the linker has not added any additional sections.)
> +if [ $magic_size -ne $zimage_size ]; then
> +   echo "zImage size ($zimage_size) disagrees with linked size ($magic_size)" >&2
> +   exit 1
> +fi
> +exit 0
> --
> 2.7.4
>

The following patch appears to fix the issue as well:

diff --git a/arch/arm/boot/compressed/vmlinux.lds.S
b/arch/arm/boot/compressed/vmlinux.lds.S
index 7a4c59154361..0e0f504e256e 100644
--- a/arch/arm/boot/compressed/vmlinux.lds.S
+++ b/arch/arm/boot/compressed/vmlinux.lds.S
@@ -83,7 +83,9 @@ SECTIONS
   __pecoff_data_rawsize = . - ADDR(.data);
 #endif

-  _edata = .;
+  .edata (NOLOAD) : {
+    _edata = .;
+  }

   _magic_sig = ZIMAGE_MAGIC(0x016f2818);
   _magic_start = ZIMAGE_MAGIC(_start);

because the NOLOAD triggers the linker to emit all PROGBITS sections
before _edata, including unknown ones.

E.g., in my binary, it gives me

00798020 D __pecoff_data_end
00798200 d __ksymtab_sort
00798208 B __bss_start
00798208 D _edata

[PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled

[Russell King - ARM Linux]

On Tue, Oct 24, 2017 at 09:09:52AM +0100, Ard Biesheuvel wrote:
> The following patch appears to fix the issue as well:
> 
> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S
> b/arch/arm/boot/compressed/vmlinux.lds.S
> index 7a4c59154361..0e0f504e256e 100644
> --- a/arch/arm/boot/compressed/vmlinux.lds.S
> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> @@ -83,7 +83,9 @@ SECTIONS
>    __pecoff_data_rawsize = . - ADDR(.data);
>  #endif
> 
> -  _edata = .;
> +  .edata (NOLOAD) : {
> +    _edata = .;
> +  }
> 
>    _magic_sig = ZIMAGE_MAGIC(0x016f2818);
>    _magic_start = ZIMAGE_MAGIC(_start);
> 
> because the NOLOAD triggers the linker to emit all PROGBITS sections
> before _edata, including unknown ones.
> 
> E.g., in my binary, it gives me
> 
> 00798020 D __pecoff_data_end
> 00798200 d __ksymtab_sort
> 00798208 B __bss_start
> 00798208 D _edata

The question is: do we want to know when additional sections get
emitted into the binary?

You've already said that for EFI, you need the size of the binary
to be a multiple of 512, so I guess the answer to that is "yes".

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

[PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled

[Ard Biesheuvel]

On 24 October 2017 at 10:09, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Tue, Oct 24, 2017 at 09:09:52AM +0100, Ard Biesheuvel wrote:
>> The following patch appears to fix the issue as well:
>>
>> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S
>> b/arch/arm/boot/compressed/vmlinux.lds.S
>> index 7a4c59154361..0e0f504e256e 100644
>> --- a/arch/arm/boot/compressed/vmlinux.lds.S
>> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
>> @@ -83,7 +83,9 @@ SECTIONS
>>    __pecoff_data_rawsize = . - ADDR(.data);
>>  #endif
>>
>> -  _edata = .;
>> +  .edata (NOLOAD) : {
>> +    _edata = .;
>> +  }
>>
>>    _magic_sig = ZIMAGE_MAGIC(0x016f2818);
>>    _magic_start = ZIMAGE_MAGIC(_start);
>>
>> because the NOLOAD triggers the linker to emit all PROGBITS sections
>> before _edata, including unknown ones.
>>
>> E.g., in my binary, it gives me
>>
>> 00798020 D __pecoff_data_end
>> 00798200 d __ksymtab_sort
>> 00798208 B __bss_start
>> 00798208 D _edata
>
> The question is: do we want to know when additional sections get
> emitted into the binary?
>

Well, we need to know whether the size of zImage is a multiple of 512
bytes. We could check that separately by adding the following as well

#ifdef CONFIG_EFI_STUB
ASSERT((_edata % 512 == 0), "zImage file size is not a multiple of 512 bytes")
#endif

> You've already said that for EFI, you need the size of the binary
> to be a multiple of 512, so I guess the answer to that is "yes".
>

Indeed.

[PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled

[Russell King - ARM Linux]

On Tue, Oct 24, 2017 at 10:13:09AM +0100, Ard Biesheuvel wrote:
> On 24 October 2017 at 10:09, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> > The question is: do we want to know when additional sections get
> > emitted into the binary?
> 
> Well, we need to know whether the size of zImage is a multiple of 512
> bytes. We could check that separately by adding the following as well
> 
> #ifdef CONFIG_EFI_STUB
> ASSERT((_edata % 512 == 0), "zImage file size is not a multiple of 512 bytes")

ASSERT((_edata - _text) % 512 == 0, "EFI zImage file size is not a multiple of 512 bytes")

This would only catch them when EFI is enabled, which doesn't give very
good build coverage.  I still prefer my solution over adding the assert
and a section for _edata - my solution catches it with EFI disabled as
well.

A variant on that would be this, which should catch additional sections
for EFI and non-EFI:

diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
index b38dcef90756..9d0c5d80979a 100644
--- a/arch/arm/boot/compressed/vmlinux.lds.S
+++ b/arch/arm/boot/compressed/vmlinux.lds.S
@@ -95,6 +95,10 @@ SECTIONS
 
   _edata = .;
 
+  .image_end (NOLOAD) : {
+    _image_end = .;
+  }
+
   _magic_sig = ZIMAGE_MAGIC(0x016f2818);
   _magic_start = ZIMAGE_MAGIC(_start);
   _magic_end = ZIMAGE_MAGIC(_edata);
@@ -119,3 +123,5 @@ SECTIONS
   .stab.indexstr 0	: { *(.stab.indexstr) }
   .comment 0		: { *(.comment) }
 }
+
+ASSERT(image_end == end, "zImage file size is incorrect")


-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

[PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled

[Ard Biesheuvel]

On 24 October 2017 at 10:22, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Tue, Oct 24, 2017 at 10:13:09AM +0100, Ard Biesheuvel wrote:
>> On 24 October 2017 at 10:09, Russell King - ARM Linux
>> <linux@armlinux.org.uk> wrote:
>> > The question is: do we want to know when additional sections get
>> > emitted into the binary?
>>
>> Well, we need to know whether the size of zImage is a multiple of 512
>> bytes. We could check that separately by adding the following as well
>>
>> #ifdef CONFIG_EFI_STUB
>> ASSERT((_edata % 512 == 0), "zImage file size is not a multiple of 512 bytes")
>
> ASSERT((_edata - _text) % 512 == 0, "EFI zImage file size is not a multiple of 512 bytes")
>
> This would only catch them when EFI is enabled, which doesn't give very
> good build coverage.  I still prefer my solution over adding the assert
> and a section for _edata - my solution catches it with EFI disabled as
> well.
>
> A variant on that would be this, which should catch additional sections
> for EFI and non-EFI:
>
> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
> index b38dcef90756..9d0c5d80979a 100644
> --- a/arch/arm/boot/compressed/vmlinux.lds.S
> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> @@ -95,6 +95,10 @@ SECTIONS
>
>    _edata = .;
>
> +  .image_end (NOLOAD) : {
> +    _image_end = .;
> +  }
> +
>    _magic_sig = ZIMAGE_MAGIC(0x016f2818);
>    _magic_start = ZIMAGE_MAGIC(_start);
>    _magic_end = ZIMAGE_MAGIC(_edata);
> @@ -119,3 +123,5 @@ SECTIONS
>    .stab.indexstr 0     : { *(.stab.indexstr) }
>    .comment 0           : { *(.comment) }
>  }
> +
> +ASSERT(image_end == end, "zImage file size is incorrect")
>

I take it you mean

_image_end == _edata

here? That works for me:

Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

(with CONFIG_EFI enabled)

[PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled

[Ard Biesheuvel]

On 24 October 2017 at 10:26, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 24 October 2017 at 10:22, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
>> On Tue, Oct 24, 2017 at 10:13:09AM +0100, Ard Biesheuvel wrote:
>>> On 24 October 2017 at 10:09, Russell King - ARM Linux
>>> <linux@armlinux.org.uk> wrote:
>>> > The question is: do we want to know when additional sections get
>>> > emitted into the binary?
>>>
>>> Well, we need to know whether the size of zImage is a multiple of 512
>>> bytes. We could check that separately by adding the following as well
>>>
>>> #ifdef CONFIG_EFI_STUB
>>> ASSERT((_edata % 512 == 0), "zImage file size is not a multiple of 512 bytes")
>>
>> ASSERT((_edata - _text) % 512 == 0, "EFI zImage file size is not a multiple of 512 bytes")
>>
>> This would only catch them when EFI is enabled, which doesn't give very
>> good build coverage.  I still prefer my solution over adding the assert
>> and a section for _edata - my solution catches it with EFI disabled as
>> well.
>>
>> A variant on that would be this, which should catch additional sections
>> for EFI and non-EFI:
>>
>> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
>> index b38dcef90756..9d0c5d80979a 100644
>> --- a/arch/arm/boot/compressed/vmlinux.lds.S
>> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
>> @@ -95,6 +95,10 @@ SECTIONS
>>
>>    _edata = .;
>>
>> +  .image_end (NOLOAD) : {
>> +    _image_end = .;
>> +  }
>> +
>>    _magic_sig = ZIMAGE_MAGIC(0x016f2818);
>>    _magic_start = ZIMAGE_MAGIC(_start);
>>    _magic_end = ZIMAGE_MAGIC(_edata);
>> @@ -119,3 +123,5 @@ SECTIONS
>>    .stab.indexstr 0     : { *(.stab.indexstr) }
>>    .comment 0           : { *(.comment) }
>>  }
>> +
>> +ASSERT(image_end == end, "zImage file size is incorrect")
>>
>
> I take it you mean
>
> _image_end == _edata
>
> here? That works for me:
>
> Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> (with CONFIG_EFI enabled)


BTW you could simplify that to

diff --git a/arch/arm/boot/compressed/vmlinux.lds.S
b/arch/arm/boot/compressed/vmlinux.lds.S
index 7a4c59154361..204d4580a04f 100644
--- a/arch/arm/boot/compressed/vmlinux.lds.S
+++ b/arch/arm/boot/compressed/vmlinux.lds.S
@@ -85,6 +85,10 @@ SECTIONS

   _edata = .;

+  .image_end (NOLOAD) : {
+    ASSERT(. == _edata, "zImage file size is incorrect");
+  }
+
   _magic_sig = ZIMAGE_MAGIC(0x016f2818);
   _magic_start = ZIMAGE_MAGIC(_start);
   _magic_end = ZIMAGE_MAGIC(_edata);

[PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled

[Russell King - ARM Linux]

On Tue, Oct 24, 2017 at 10:30:41AM +0100, Ard Biesheuvel wrote:
> On 24 October 2017 at 10:26, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > On 24 October 2017 at 10:22, Russell King - ARM Linux
> > <linux@armlinux.org.uk> wrote:
> >> On Tue, Oct 24, 2017 at 10:13:09AM +0100, Ard Biesheuvel wrote:
> >>> On 24 October 2017 at 10:09, Russell King - ARM Linux
> >>> <linux@armlinux.org.uk> wrote:
> >>> > The question is: do we want to know when additional sections get
> >>> > emitted into the binary?
> >>>
> >>> Well, we need to know whether the size of zImage is a multiple of 512
> >>> bytes. We could check that separately by adding the following as well
> >>>
> >>> #ifdef CONFIG_EFI_STUB
> >>> ASSERT((_edata % 512 == 0), "zImage file size is not a multiple of 512 bytes")
> >>
> >> ASSERT((_edata - _text) % 512 == 0, "EFI zImage file size is not a multiple of 512 bytes")
> >>
> >> This would only catch them when EFI is enabled, which doesn't give very
> >> good build coverage.  I still prefer my solution over adding the assert
> >> and a section for _edata - my solution catches it with EFI disabled as
> >> well.
> >>
> >> A variant on that would be this, which should catch additional sections
> >> for EFI and non-EFI:
> >>
> >> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
> >> index b38dcef90756..9d0c5d80979a 100644
> >> --- a/arch/arm/boot/compressed/vmlinux.lds.S
> >> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> >> @@ -95,6 +95,10 @@ SECTIONS
> >>
> >>    _edata = .;
> >>
> >> +  .image_end (NOLOAD) : {
> >> +    _image_end = .;
> >> +  }
> >> +
> >>    _magic_sig = ZIMAGE_MAGIC(0x016f2818);
> >>    _magic_start = ZIMAGE_MAGIC(_start);
> >>    _magic_end = ZIMAGE_MAGIC(_edata);
> >> @@ -119,3 +123,5 @@ SECTIONS
> >>    .stab.indexstr 0     : { *(.stab.indexstr) }
> >>    .comment 0           : { *(.comment) }
> >>  }
> >> +
> >> +ASSERT(image_end == end, "zImage file size is incorrect")
> >>
> >
> > I take it you mean
> >
> > _image_end == _edata
> >
> > here? That works for me:
> >
> > Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >
> > (with CONFIG_EFI enabled)
> 
> 
> BTW you could simplify that to
> 
> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S
> b/arch/arm/boot/compressed/vmlinux.lds.S
> index 7a4c59154361..204d4580a04f 100644
> --- a/arch/arm/boot/compressed/vmlinux.lds.S
> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> @@ -85,6 +85,10 @@ SECTIONS
> 
>    _edata = .;
> 
> +  .image_end (NOLOAD) : {
> +    ASSERT(. == _edata, "zImage file size is incorrect");
> +  }

Yep.  Okay, so now we have a patch from Arnd to fix the alignment problem
(so don't need alignment of the piggydata), and we have a patch to catch
the additional sections.  That leaves a patch to sort out the additional
sections.

Do you have any preference - I'd prefer one that I can merge along with
these changes?  One way forward would be to temporarily add the /DISCARD/
for the ksym sections, which can then be removed once your sort() removal
gets added to the EFI trees.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

[PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled

[Ard Biesheuvel]

On 24 October 2017 at 10:38, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Tue, Oct 24, 2017 at 10:30:41AM +0100, Ard Biesheuvel wrote:
>> On 24 October 2017 at 10:26, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> > On 24 October 2017 at 10:22, Russell King - ARM Linux
>> > <linux@armlinux.org.uk> wrote:
>> >> On Tue, Oct 24, 2017 at 10:13:09AM +0100, Ard Biesheuvel wrote:
>> >>> On 24 October 2017 at 10:09, Russell King - ARM Linux
>> >>> <linux@armlinux.org.uk> wrote:
>> >>> > The question is: do we want to know when additional sections get
>> >>> > emitted into the binary?
>> >>>
>> >>> Well, we need to know whether the size of zImage is a multiple of 512
>> >>> bytes. We could check that separately by adding the following as well
>> >>>
>> >>> #ifdef CONFIG_EFI_STUB
>> >>> ASSERT((_edata % 512 == 0), "zImage file size is not a multiple of 512 bytes")
>> >>
>> >> ASSERT((_edata - _text) % 512 == 0, "EFI zImage file size is not a multiple of 512 bytes")
>> >>
>> >> This would only catch them when EFI is enabled, which doesn't give very
>> >> good build coverage.  I still prefer my solution over adding the assert
>> >> and a section for _edata - my solution catches it with EFI disabled as
>> >> well.
>> >>
>> >> A variant on that would be this, which should catch additional sections
>> >> for EFI and non-EFI:
>> >>
>> >> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
>> >> index b38dcef90756..9d0c5d80979a 100644
>> >> --- a/arch/arm/boot/compressed/vmlinux.lds.S
>> >> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
>> >> @@ -95,6 +95,10 @@ SECTIONS
>> >>
>> >>    _edata = .;
>> >>
>> >> +  .image_end (NOLOAD) : {
>> >> +    _image_end = .;
>> >> +  }
>> >> +
>> >>    _magic_sig = ZIMAGE_MAGIC(0x016f2818);
>> >>    _magic_start = ZIMAGE_MAGIC(_start);
>> >>    _magic_end = ZIMAGE_MAGIC(_edata);
>> >> @@ -119,3 +123,5 @@ SECTIONS
>> >>    .stab.indexstr 0     : { *(.stab.indexstr) }
>> >>    .comment 0           : { *(.comment) }
>> >>  }
>> >> +
>> >> +ASSERT(image_end == end, "zImage file size is incorrect")
>> >>
>> >
>> > I take it you mean
>> >
>> > _image_end == _edata
>> >
>> > here? That works for me:
>> >
>> > Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >
>> > (with CONFIG_EFI enabled)
>>
>>
>> BTW you could simplify that to
>>
>> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S
>> b/arch/arm/boot/compressed/vmlinux.lds.S
>> index 7a4c59154361..204d4580a04f 100644
>> --- a/arch/arm/boot/compressed/vmlinux.lds.S
>> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
>> @@ -85,6 +85,10 @@ SECTIONS
>>
>>    _edata = .;
>>
>> +  .image_end (NOLOAD) : {
>> +    ASSERT(. == _edata, "zImage file size is incorrect");
>> +  }
>
> Yep.  Okay, so now we have a patch from Arnd to fix the alignment problem
> (so don't need alignment of the piggydata), and we have a patch to catch
> the additional sections.  That leaves a patch to sort out the additional
> sections.
>
> Do you have any preference - I'd prefer one that I can merge along with
> these changes?  One way forward would be to temporarily add the /DISCARD/
> for the ksym sections, which can then be removed once your sort() removal
> gets added to the EFI trees.
>

Well, I can respin that patch to only discard the ksymtab/kcrctab
sections (and drop the alignment of piggydata), but I'd prefer to make
it permanent if you don't mind. I still intend to remove the sort()
call, given that ARM doesn't need it, but it seems more future proof
to me to always discard those sections, and if we end up incorporating
more core kernel code into the EFI stub, we won't need to revisit this
(although I am not aware of any reasons why we would be doing that in
the near future)

[PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled

[Russell King - ARM Linux]

On Tue, Oct 24, 2017 at 10:44:00AM +0100, Ard Biesheuvel wrote:
> On 24 October 2017 at 10:38, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> > Do you have any preference - I'd prefer one that I can merge along with
> > these changes?  One way forward would be to temporarily add the /DISCARD/
> > for the ksym sections, which can then be removed once your sort() removal
> > gets added to the EFI trees.
> >
> 
> Well, I can respin that patch to only discard the ksymtab/kcrctab
> sections (and drop the alignment of piggydata), but I'd prefer to make
> it permanent if you don't mind. I still intend to remove the sort()
> call, given that ARM doesn't need it, but it seems more future proof
> to me to always discard those sections, and if we end up incorporating
> more core kernel code into the EFI stub, we won't need to revisit this
> (although I am not aware of any reasons why we would be doing that in
> the near future)

I don't think it's future-proof.  The decompressor is a particularly
special environment due to its runtime relocation support, where we
(eg) don't really support initialised pointers in the .data section.
Stuffing code from other parts of the kernel into the decompressor
isn't going to work reliably, and I'd much rather discourage the
practice.

Hence, there's really no reason to keep that discard, and I'd much
rather have the build fail if the sections re-appear as a pointer
towards something not being correct.

A solution to this would be to link the decompressor ELF with -r
and wrap that up in a mini-linker, but I suspect that will need
quite a re-write because of the self-relocation that the image
does if it detects overlap between itself and the destination for
the decompressed kernel image.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

[PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled

[Ard Biesheuvel]

On 24 October 2017 at 10:54, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Tue, Oct 24, 2017 at 10:44:00AM +0100, Ard Biesheuvel wrote:
>> On 24 October 2017 at 10:38, Russell King - ARM Linux
>> <linux@armlinux.org.uk> wrote:
>> > Do you have any preference - I'd prefer one that I can merge along with
>> > these changes?  One way forward would be to temporarily add the /DISCARD/
>> > for the ksym sections, which can then be removed once your sort() removal
>> > gets added to the EFI trees.
>> >
>>
>> Well, I can respin that patch to only discard the ksymtab/kcrctab
>> sections (and drop the alignment of piggydata), but I'd prefer to make
>> it permanent if you don't mind. I still intend to remove the sort()
>> call, given that ARM doesn't need it, but it seems more future proof
>> to me to always discard those sections, and if we end up incorporating
>> more core kernel code into the EFI stub, we won't need to revisit this
>> (although I am not aware of any reasons why we would be doing that in
>> the near future)
>
> I don't think it's future-proof.  The decompressor is a particularly
> special environment due to its runtime relocation support, where we
> (eg) don't really support initialised pointers in the .data section.
> Stuffing code from other parts of the kernel into the decompressor
> isn't going to work reliably, and I'd much rather discourage the
> practice.
>

Fair enough.

> Hence, there's really no reason to keep that discard, and I'd much
> rather have the build fail if the sections re-appear as a pointer
> towards something not being correct.
>

OK. You're welcome to take that sort() removal patch through the ARM
tree if you want to keep these patches together. Or I could respin the
/discard/ one. Whichever you prefer (although I am leaning towards the
former, so we can merge them and be done with it)

> A solution to this would be to link the decompressor ELF with -r
> and wrap that up in a mini-linker, but I suspect that will need
> quite a re-write because of the self-relocation that the image
> does if it detects overlap between itself and the destination for
> the decompressed kernel image.
>

It was already quite tricky to get the hybrid PE-COFF/bare metal image
working, especially because the stub code is shared between x86, arm64
and ARM, all of which have their own quirks when it comes to the early
environment this code executes in.

[PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled

[jeffy]

Hi guys,

On 10/24/2017 04:09 PM, Ard Biesheuvel wrote:
> The following patch appears to fix the issue as well:
>
> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S
> b/arch/arm/boot/compressed/vmlinux.lds.S
> index 7a4c59154361..0e0f504e256e 100644
> --- a/arch/arm/boot/compressed/vmlinux.lds.S
> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> @@ -83,7 +83,9 @@ SECTIONS
>     __pecoff_data_rawsize = . - ADDR(.data);
>   #endif
>
> -  _edata = .;
> +  .edata (NOLOAD) : {
> +    _edata = .;
> +  }
>
>     _magic_sig = ZIMAGE_MAGIC(0x016f2818);
>     _magic_start = ZIMAGE_MAGIC(_start);
>
> because the NOLOAD triggers the linker to emit all PROGBITS sections
> before _edata, including unknown ones.
>
> E.g., in my binary, it gives me
>
> 00798020 D __pecoff_data_end
> 00798200 d __ksymtab_sort
> 00798208 B __bss_start
> 00798208 D _edata

this works too:

+++ b/arch/arm/boot/compressed/vmlinux.lds.S
@@ -70,10 +70,6 @@ SECTIONS
    .got                 : { *(.got) }
    _got_end = .;

-  /* ensure the zImage file size is always a multiple of 64 bits */
-  /* (without a dummy byte, ld just ignores the empty section) */
-  .pad                 : { BYTE(0); . = ALIGN(8); }
-
  #ifdef CONFIG_EFI_STUB
    .data : ALIGN(4096) {
      __pecoff_data_start = .;
@@ -93,7 +89,10 @@ SECTIONS
    __pecoff_data_rawsize = . - ADDR(.data);
  #endif

-  _edata = .;
+  /* ensure the zImage file size is always a multiple of 64 bits */
+  .edata : ALIGN(8) {
+    _edata = .;
+  }



    339: 006cf200     8 OBJECT  LOCAL  DEFAULT    9 __ksymtab_sort
...
    528: 006cf208     0 NOTYPE  GLOBAL DEFAULT    9 _edata