Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Sync linker scripts and crt0 for arm/aarch64 with what the latest in shim #3087

Merged
merged 2 commits into from Apr 7, 2021
Merged

Sync linker scripts and crt0 for arm/aarch64 with what the latest in shim #3087

merged 2 commits into from Apr 7, 2021

Conversation

martinezjavier
Copy link
Collaborator

Type of pull request:

@superm1
Copy link
Member

superm1 commented Mar 30, 2021

Mm I guess this means aarch64 is busted before this fix and all the tarballs will need to respin with it right?

superm1
superm1 previously approved these changes Mar 30, 2021
View reviewed changes
@superm1
Copy link
Member

superm1 commented Mar 30, 2021

As a thought - would shim consider evicting these files into a subproject? It might easier to keep in sync with downstream projects like fwupd and grub if it's subprojected.

@martinezjavier
Copy link
Collaborator Author

Mm I guess this means aarch64 is busted before this fix and all the tarballs will need to respin with it right?

Yes, that's correct. I noticed that we don't even have a SBAT section in fwupdaa64.efi binaries with latest releases.

@martinezjavier
Copy link
Collaborator Author

As a thought - would shim consider evicting these files into a subproject? It might easier to keep in sync with downstream projects like fwupd and grub if it's subprojected.

I think that makes sense but will need some coordination with shim.

@martinezjavier martinezjavier marked this pull request as draft March 30, 2021 15:35
@martinezjavier
Copy link
Collaborator Author

Marked it as a draft since I just built tested the changes for now.

@hughsie
Copy link
Member

hughsie commented Mar 31, 2021

Urgh, CI failure. Got any ideas or want me to take a look?

@martinezjavier
Copy link
Collaborator Author

Urgh, CI failure. Got any ideas or want me to take a look?

Sorry, pushed some changes this morning to address the issue @vathpela mentioned yesterday but seems that broke the x86_64 builds.

I’ll take a look at this later today.

@martinezjavier martinezjavier force-pushed the sync-lds-crt0 branch 3 times, most recently from 30db835 to 89fb849 Compare March 31, 2021 12:21
@martinezjavier
Copy link
Collaborator Author

seems that broke the x86_64 builds.

Actually I only broke on Debian due gnuefi not being present in the path to their linker scripts and crt0. That's why I didn't notice on my local builds.

I've pushed a new version that should fix the issue and also links against the gnuefi libs even when using a custom crt0. Which was the issue Peter mentioned yesterday.

@superm1
Copy link
Member

superm1 commented Mar 31, 2021

seems that broke the x86_64 builds.

Actually I only broke on Debian due gnuefi not being present in the path to their linker scripts and crt0. That's why I didn't notice on my local builds.

I've pushed a new version that should fix the issue and also links against the gnuefi libs even when using a custom crt0. Which was the issue Peter mentioned yesterday.

Yay CI for catching it :)

@hughsie
Copy link
Member

hughsie commented Mar 31, 2021

@martinezjavier yell when we're good for merging and backporting, but no pressure from me.

@martinezjavier
uefi-capsule: Sync linker scripts with latest used by shim
a0975fe 
The linker scripts used in the project were copied from the shim project,
but there were a few fixes made after this original copy.

Until binutils have proper pei-aarch64 support, the linker scripts should
be kept in sync to make sure that the PE32+ binaries are built correctly.

The fixes included in this change are the following:

 * Include missing .text sections in PE/COFF binary (Chris Coulson)
 * Put .sbat after _edata (Peter Jones)
 * Fix some PE headers for arm and aarch64 (Peter Jones)
 * Include the aligned part in SizeOfRawData of sbat for arm and aarch64 (Gary Lin)
 * Swizzle some sections to make old sbsign happier for arm and aarch64 (Peter Jones)
 * Put .rel* and .dyn* in .rodata for arm and aarch64 (Peter Jones)
@martinezjavier
uefi-capsule: Include crt0 for arm and aarch64 that add a SBAT section
75159fe 
Due the lack of pei-aarch64 support in binutils, the gnu-efi crt0 harcodes
the PE32+ sections among other things. These crt0 aren't aware of the SBAT
section and so custom ones have to be used.

In the same vein as commit cfd1f2f ("uefi-capsule: Ensure SBAT metadata
is added correctly") included custom linker scripts, this change add a set
of crt0 for arm and aarch64 that hardcode a SBAT section in the PE headers.

These are the crt0 from gnu-efi plus the following fixes from Peter Jones:

* Include .sbat in section headers
* Fix some PE headers
* Calculate the VirtualSize of .sbat separately
* Put .rel* and .dyn* in .rodata
@martinezjavier martinezjavier marked this pull request as ready for review April 7, 2021 12:17
@martinezjavier
Copy link
Collaborator Author

@martinezjavier yell when we're good for merging and backporting, but no pressure from me.

I think it is ready to be merged now. The fwupdaa64.efi builds correctly, it contains an .sbat section [0] and I tested that the binary is executed correctly on an rpi4 with u-boot's EFI stub, qemu-system-aarch64 with OVMF and bare-metal aarch64 server machine.

I didn't get access to a system that supports firmware updates so I only tested that the PE32+ binary was executed and printed the error message about no updates to process [1].

[0]:

$ objdump -h fwupdaa64.efi
...
Sections:
Idx Name          Size      VMA               LMA               File off  Algn
  0 .text         00007b40  0000000000001000  0000000000001000  00001000  2**4
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  1 .data         00000e00  000000000000a000  000000000000a000  0000a000  2**4
                  CONTENTS, ALLOC, LOAD, DATA
  2 .sbat         000000f4  000000000000b000  000000000000b000  0000b000  2**3
                  CONTENTS, ALLOC, LOAD, READONLY, DATA
  3 .rodata       00004000  000000000000c000  000000000000c000  0000c000  2**3
                  CONTENTS, ALLOC, LOAD, READONLY, DATA                  
   
$ objdump -s -j .sbat fwupdaa64.efi 
...
Contents of section .sbat:
 b000 73626174 2c312c55 45464920 7368696d  sbat,1,UEFI shim
 b010 2c736261 742c312c 68747470 733a2f2f  ,sbat,1,https://
 b020 67697468 75622e63 6f6d2f72 68626f6f  github.com/rhboo
 b030 742f7368 696d2f62 6c6f622f 6d61696e  t/shim/blob/main
 b040 2f534241 542e6d64 0a667775 70642c31  /SBAT.md.fwupd,1
 b050 2c466972 6d776172 65207570 64617465  ,Firmware update
 b060 20646165 6d6f6e2c 66777570 642c312e   daemon,fwupd,1.
 b070 362e302c 68747470 733a2f2f 67697468  6.0,https://gith
 b080 75622e63 6f6d2f66 77757064 2f667775  ub.com/fwupd/fwu
 b090 70640a66 77757064 2e666564 6f72612c  pd.fwupd.fedora,
 b0a0 312c5468 65204665 646f7261 2050726f  1,The Fedora Pro
 b0b0 6a656374 2c667775 70642c66 77757064  ject,fwupd,fwupd
 b0c0 2d666564 6f72612d 33342c68 74747073  -fedora-34,https
 b0d0 3a2f2f73 72632e66 65646f72 6170726f  ://src.fedorapro
 b0e0 6a656374 2e6f7267 2f72706d 732f6677  ject.org/rpms/fw
 b0f0 7570640a                             upd.

[1]:

FS0:\EFI\test\> fwupdaa64.efi
WARNING: No updates to process.  Called in error?

@hughsie hughsie merged commit f232701 into fwupd:master Apr 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hughsie hughsie hughsie approved these changes

@superm1 superm1 superm1 left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@martinezjavier @superm1 @hughsie