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 09:50:06 +0100
From:   Russell King - ARM Linux <linux@...linux.org.uk>
To:     jeffy <jeffy.chen@...k-chips.com>
Cc:     Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        "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

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@...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

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