From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ard Biesheuvel Subject: Re: [PATCH v2 5/7] arm: efi: split zImage code and data into separate PE/COFF sections Date: Fri, 8 Sep 2017 16:18:06 +0100 Message-ID: References: <20170629081849.15081-1-ard.biesheuvel@linaro.org> <20170629081849.15081-6-ard.biesheuvel@linaro.org> <87r2vhs3il.fsf@free-electrons.com> <87mv65s1iu.fsf@free-electrons.com> <87efrhs0gj.fsf@free-electrons.com> <87a825rzio.fsf@free-electrons.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: In-Reply-To: <87a825rzio.fsf-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> Sender: linux-efi-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Gregory CLEMENT Cc: Thomas Petazzoni , Andrew Lunn , "linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Jason Cooper , Matt Fleming , =?UTF-8?Q?Antoine_T=C3=A9nart?= , Russell King , Leif Lindholm , Miquel RAYNAL , "linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" , Sebastian Hesselbarth List-Id: linux-efi@vger.kernel.org On 8 September 2017 at 16:17, Gregory CLEMENT wrote: > Hi Ard, > > On ven., sept. 08 2017, Ard Biesheuvel wrote: > >> On 8 September 2017 at 15:57, Ard Biesheuvel wrote: >>> On 8 September 2017 at 15:56, Gregory CLEMENT >>> wrote: >>>> Hi Ard, >>>> >>>> On ven., sept. 08 2017, Ard Biesheuvel wrote: >>>> >>>>> On 8 September 2017 at 15:33, Gregory CLEMENT >>>>> wrote: >>>>>> Hi Ard, >>>>>> >>>>>> On ven., sept. 08 2017, Ard Biesheuvel wrote: >>>>>> >>>>>>> On 8 September 2017 at 14:54, Ard Biesheuvel wrote: >>>>>>>> On 8 September 2017 at 14:50, Gregory CLEMENT >>>>>>>> wrote: >>>>>>>>> Hi Ard, >>>>>>>>> >>>>>>>>> On jeu., juin 29 2017, Ard Biesheuvel wrote: >>>>>>>>> >>>>>>>>>> To prevent unintended modifications to the kernel text (malicious or >>>>>>>>>> otherwise) while running the EFI stub, describe the kernel image as >>>>>>>>>> two separate sections: a .text section with read-execute permissions, >>>>>>>>>> covering .text, .rodata, .piggytext and the GOT sections (which the >>>>>>>>>> stub does not care about anyway), and a .data section with read-write >>>>>>>>>> permissions, covering .data and .bss. >>>>>>>>>> >>>>>>>>>> This relies on the firmware to actually take the section permission >>>>>>>>>> flags into account, but this is something that is currently being >>>>>>>>>> implemented in EDK2, which means we will likely start seeing it in >>>>>>>>>> the wild between one and two years from now. >>>>>>>>> >>>>>>>>> This patch had been merged in mainline yesterday and now prevent the >>>>>>>>> Marvell Armada 370 and the Armada XP based SoC to boot. I also suspect >>>>>>>>> that more Socs are impacted because the number of boot fail exploded >>>>>>>>> according to kci: >>>>>>>>> https://kernelci.org/boot/all/job/mainline/branch/master/kernel/v4.13-8899-g8dc5b3a6cb2f/ >>>>>>>>> >>>>>>>> >>>>>>>> Ouch. >>>>>>>> >>>>>>>>> I found this patch after bisecting (I can provide the bisect log if >>>>>>>>> needed). >>>>>>>>> >>>>>>>>> The kernel failed to boot only if CONFIG_EFI is enabled so it occurs in >>>>>>>>> multi_v7_defconfig but not with mvebu_v7_defconfig. >>>>>>>>> >>>>>>>>> Currently the solution is to revert this patch. >>>>>>>>> >>>>>>>>> Have you a better option? >>>>>>>>> >>>>>>>> >>>>>>>> I will investigate. >>>>>>> >>>>>>> I cannot reproduce this on QEMU or my Beaglebone white. I have tried a >>>>>>> locally built zImage as well as the one built by kernelci. >>>>>>> >>>>>>> Could you please try whether this fixes things? It does not explain >>>>>>> anything but it will help me figure out what is going on (hopefully) >>>>>> >>>>>> I've just tested this change and it didn't fix anything. >>>>>> >>>>>> Gregory >>>>>> >>>>>>> >>>>>>> >>>>>>> diff --git a/arch/arm/boot/compressed/efi-header.S >>>>>>> b/arch/arm/boot/compressed/efi-header.S >>>>>>> index c94a88ae834d..671a6e5b7b99 100644 >>>>>>> --- a/arch/arm/boot/compressed/efi-header.S >>>>>>> +++ b/arch/arm/boot/compressed/efi-header.S >>>>>>> @@ -127,7 +127,7 @@ section_table: >>>>>>> >>>>>>> .set section_count, (. - section_table) / 40 >>>>>>> >>>>>>> - .align 12 >>>>>>> + .align 9 >>>>>>> __efi_start: >>>>>>> #endif >>>>>>> .endm >>>>>> >>>>> >>>>> >>>>> How about this? >>>> >>>> It fixed the bug! (I tested with and without your previous patch and it >>>> worked in both case) >>>> >>>> When you will send your patch, you can add my: >>>> Tested-by: Gregory CLEMENT >>>> >> >> Would you mind checking whether this fixes the issue as well? >> >> diff --git a/arch/arm/boot/compressed/piggy.S b/arch/arm/boot/compressed/piggy.S >> index f72088495f43..5d52c556dd32 100644 >> --- a/arch/arm/boot/compressed/piggy.S >> +++ b/arch/arm/boot/compressed/piggy.S >> @@ -1,5 +1,6 @@ >> .section .piggydata,#alloc >> .globl input_data >> + .align 2 >> input_data: >> .incbin "arch/arm/boot/compressed/piggy_data" >> .globl input_data_end >> >> There may be other reasons than ksymtab entries that could result in >> piggydata ending up unaligned in the decompressor (which is what >> caused the issue before) >> This is a better fix, because it addresses the root cause. > > I've tested this single patch and it does not fix the boot issue :( > OK, strange. Anyway, thanks for testing. I will post the original fix then. From mboxrd@z Thu Jan 1 00:00:00 1970 From: ard.biesheuvel@linaro.org (Ard Biesheuvel) Date: Fri, 8 Sep 2017 16:18:06 +0100 Subject: [PATCH v2 5/7] arm: efi: split zImage code and data into separate PE/COFF sections In-Reply-To: <87a825rzio.fsf@free-electrons.com> References: <20170629081849.15081-1-ard.biesheuvel@linaro.org> <20170629081849.15081-6-ard.biesheuvel@linaro.org> <87r2vhs3il.fsf@free-electrons.com> <87mv65s1iu.fsf@free-electrons.com> <87efrhs0gj.fsf@free-electrons.com> <87a825rzio.fsf@free-electrons.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 8 September 2017 at 16:17, Gregory CLEMENT wrote: > Hi Ard, > > On ven., sept. 08 2017, Ard Biesheuvel wrote: > >> On 8 September 2017 at 15:57, Ard Biesheuvel wrote: >>> On 8 September 2017 at 15:56, Gregory CLEMENT >>> wrote: >>>> Hi Ard, >>>> >>>> On ven., sept. 08 2017, Ard Biesheuvel wrote: >>>> >>>>> On 8 September 2017 at 15:33, Gregory CLEMENT >>>>> wrote: >>>>>> Hi Ard, >>>>>> >>>>>> On ven., sept. 08 2017, Ard Biesheuvel wrote: >>>>>> >>>>>>> On 8 September 2017 at 14:54, Ard Biesheuvel wrote: >>>>>>>> On 8 September 2017 at 14:50, Gregory CLEMENT >>>>>>>> wrote: >>>>>>>>> Hi Ard, >>>>>>>>> >>>>>>>>> On jeu., juin 29 2017, Ard Biesheuvel wrote: >>>>>>>>> >>>>>>>>>> To prevent unintended modifications to the kernel text (malicious or >>>>>>>>>> otherwise) while running the EFI stub, describe the kernel image as >>>>>>>>>> two separate sections: a .text section with read-execute permissions, >>>>>>>>>> covering .text, .rodata, .piggytext and the GOT sections (which the >>>>>>>>>> stub does not care about anyway), and a .data section with read-write >>>>>>>>>> permissions, covering .data and .bss. >>>>>>>>>> >>>>>>>>>> This relies on the firmware to actually take the section permission >>>>>>>>>> flags into account, but this is something that is currently being >>>>>>>>>> implemented in EDK2, which means we will likely start seeing it in >>>>>>>>>> the wild between one and two years from now. >>>>>>>>> >>>>>>>>> This patch had been merged in mainline yesterday and now prevent the >>>>>>>>> Marvell Armada 370 and the Armada XP based SoC to boot. I also suspect >>>>>>>>> that more Socs are impacted because the number of boot fail exploded >>>>>>>>> according to kci: >>>>>>>>> https://kernelci.org/boot/all/job/mainline/branch/master/kernel/v4.13-8899-g8dc5b3a6cb2f/ >>>>>>>>> >>>>>>>> >>>>>>>> Ouch. >>>>>>>> >>>>>>>>> I found this patch after bisecting (I can provide the bisect log if >>>>>>>>> needed). >>>>>>>>> >>>>>>>>> The kernel failed to boot only if CONFIG_EFI is enabled so it occurs in >>>>>>>>> multi_v7_defconfig but not with mvebu_v7_defconfig. >>>>>>>>> >>>>>>>>> Currently the solution is to revert this patch. >>>>>>>>> >>>>>>>>> Have you a better option? >>>>>>>>> >>>>>>>> >>>>>>>> I will investigate. >>>>>>> >>>>>>> I cannot reproduce this on QEMU or my Beaglebone white. I have tried a >>>>>>> locally built zImage as well as the one built by kernelci. >>>>>>> >>>>>>> Could you please try whether this fixes things? It does not explain >>>>>>> anything but it will help me figure out what is going on (hopefully) >>>>>> >>>>>> I've just tested this change and it didn't fix anything. >>>>>> >>>>>> Gregory >>>>>> >>>>>>> >>>>>>> >>>>>>> diff --git a/arch/arm/boot/compressed/efi-header.S >>>>>>> b/arch/arm/boot/compressed/efi-header.S >>>>>>> index c94a88ae834d..671a6e5b7b99 100644 >>>>>>> --- a/arch/arm/boot/compressed/efi-header.S >>>>>>> +++ b/arch/arm/boot/compressed/efi-header.S >>>>>>> @@ -127,7 +127,7 @@ section_table: >>>>>>> >>>>>>> .set section_count, (. - section_table) / 40 >>>>>>> >>>>>>> - .align 12 >>>>>>> + .align 9 >>>>>>> __efi_start: >>>>>>> #endif >>>>>>> .endm >>>>>> >>>>> >>>>> >>>>> How about this? >>>> >>>> It fixed the bug! (I tested with and without your previous patch and it >>>> worked in both case) >>>> >>>> When you will send your patch, you can add my: >>>> Tested-by: Gregory CLEMENT >>>> >> >> Would you mind checking whether this fixes the issue as well? >> >> diff --git a/arch/arm/boot/compressed/piggy.S b/arch/arm/boot/compressed/piggy.S >> index f72088495f43..5d52c556dd32 100644 >> --- a/arch/arm/boot/compressed/piggy.S >> +++ b/arch/arm/boot/compressed/piggy.S >> @@ -1,5 +1,6 @@ >> .section .piggydata,#alloc >> .globl input_data >> + .align 2 >> input_data: >> .incbin "arch/arm/boot/compressed/piggy_data" >> .globl input_data_end >> >> There may be other reasons than ksymtab entries that could result in >> piggydata ending up unaligned in the decompressor (which is what >> caused the issue before) >> This is a better fix, because it addresses the root cause. > > I've tested this single patch and it does not fix the boot issue :( > OK, strange. Anyway, thanks for testing. I will post the original fix then.