linux-kernel - Re: [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 23 Oct 2017 11:26:49 +0800
From:   jeffy <jeffy.chen@...k-chips.com>
To:     Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        Russell King - ARM Linux <linux@...linux.org.uk>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        chris.zhong@...k-chips.com, Ingo Molnar <mingo@...nel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB
 enabled

Hi Ard,

On 10/22/2017 09:01 PM, Ard Biesheuvel wrote:
> On 22 October 2017 at 13:47, Russell King - ARM Linux
> <linux@...linux.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@...k-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@...k-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.
>
>
>