[PATCH] efi_loader: Fix section alignment on EFI binaries

[Ilias Apalodimas]

Hi Heinrich

On Fri, 3 Jan 2025 at 22:20, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 10.12.24 17:01, Ilias Apalodimas wrote:
> > When creating EFI binaries, the alignment of the text section isn't
> > correctly factored in. As a result trying to load signed EFI binaries
> > throws an error with:
> >
> > efi_image_region_add() efi_image_region_add: new region already part of another
> > Image not authenticated
>
> You are reporting here two different problems here:
>
> * We create EFI binaries that sbsign does not want to sign and U-Boot
> does not accept for secure boot.
> * efi_image_region_add() creates a message that does not match the
> situation.
>
> Will you prepare a second patch for the latter?

I briefly tried to fix the second problem without success. Since it
doesn't cause any issues I just mentioned it on the commit message for
completeness. I'll try to figure out what causes it, but it's not on
my top prios.

>
> >
> > Running the binary through sbverify has a similar warning
> > sbverify ./lib/efi_loader/helloworld.efi
> > warning: gap in section table:
> >      .text   : 0x00001000 - 0x00001c00,
> >      .data   : 0x00002000 - 0x00002200,
> > gaps in the section table may result in different checksums
> > warning: data remaining[7680 vs 12720]: gaps between PE/COFF sections?
> > .....
> >
> > If we include the alignment in the text section, the signed binary boots
> > fine, and the relevant sbverify warning goes away
> > sbverify ./lib/efi_loader/helloworld.efi
> > warning: data remaining[8704 vs 12720]: gaps between PE/COFF sections?
> > .....
>
> Does EDK II complain?

I haven't tried any EDK2 apps, but the linker scripts differ between projects.

>
> >
> > We should look into the remaining warning at some point as well
> > regarding the gaps between PE/COFF sections.
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> > ---
> >   arch/arm/lib/elf_aarch64_efi.lds | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/lib/elf_aarch64_efi.lds b/arch/arm/lib/elf_aarch64_efi.lds
> > index 5dd98091698c..e382254a6cf5 100644
> > --- a/arch/arm/lib/elf_aarch64_efi.lds
> > +++ b/arch/arm/lib/elf_aarch64_efi.lds
> > @@ -32,9 +32,9 @@ SECTIONS
> >       .rela.plt : { *(.rela.plt) }
> >       .rela.got : { *(.rela.got) }
> >       .rela.data : { *(.rela.data) *(.rela.data*) }
> > +     . = ALIGN(4096);
>
> If we make this change, we should do so for all UEFI architectures!
>
> Only OUTPUT_FORMAT and OUTPUT_ARCH need to be architecture specific in
> the linker scripts. Can't we use an INCLUDE statement (cf.
> https://sourceware.org/binutils/docs/ld/File-Commands.html)?
>
> Of aarch64, arm, riscv32, riscv64 only the arm script differs heavily
> for no good reason. We should get rid of this exception.

Sure, but this is fixing sandbox for the CI testing on arm64 hosts. We
don't have any other native hosts apart from x86 and arm64.
Refactoring all the linker scripts for apps is a bigger task. Mind if
we take one step at a time?

Thanks
/Ilias
> Best regards
>
> Heinrich
>
> >       _etext = .;
> >       _text_size = . - _text;
> > -     . = ALIGN(4096);
> >       .data : {
> >               _data = .;
> >               *(.sdata)
>