Re: [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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx> wrote:
>> > On 24 October 2017 at 10:22, Russell King - ARM Linux
>> > <linux@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
>> >
>> > (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)