[PATCH v2 5/7] arm: efi: split zImage code and data into separate PE/COFF sections

[Gregory CLEMENT]

Hi Ard,
 
 On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel at linaro.org> wrote:

> On 8 September 2017 at 15:57, Ard Biesheuvel <ard.biesheuvel at linaro.org> wrote:
>> On 8 September 2017 at 15:56, Gregory CLEMENT
>> <gregory.clement at free-electrons.com> wrote:
>>> Hi Ard,
>>>
>>>  On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel at linaro.org> wrote:
>>>
>>>> On 8 September 2017 at 15:33, Gregory CLEMENT
>>>> <gregory.clement at free-electrons.com> wrote:
>>>>> Hi Ard,
>>>>>
>>>>>  On ven., sept. 08 2017, Ard Biesheuvel <ard.biesheuvel at linaro.org> wrote:
>>>>>
>>>>>> On 8 September 2017 at 14:54, Ard Biesheuvel <ard.biesheuvel at linaro.org> wrote:
>>>>>>> On 8 September 2017 at 14:50, Gregory CLEMENT
>>>>>>> <gregory.clement at free-electrons.com> wrote:
>>>>>>>> Hi Ard,
>>>>>>>>
>>>>>>>>  On jeu., juin 29 2017, Ard Biesheuvel <ard.biesheuvel at linaro.org> 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 <gregory.clement at free-electrons.com>
>>>
>
> 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 :(

Gregory


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com