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.

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

Aa64 fixes #6

Open
wants to merge 7 commits into
base: rawhide
Choose a base branch
from
Open

Aa64 fixes #6

wants to merge 7 commits into from

Conversation

ozbenh
Copy link

@ozbenh ozbenh commented Dec 21, 2022

These are three backports cherry-picked from a newer upstream (you can drop them if you rebase) and two fixes I added.

WARNING: The second fix might be controversial or might require further changes, see below:

  • Add the missing .note.GNU-stack section to lib/ctors.S (that file is added by a RH specific patch)
  • Fix section alignment issues, see below:

The issue with alignement is that the new binutils assume a 64k alignemnt for aarch64 by default. But the gnu-efi provided .lds doesn't align sections properly (.dynamic isn't aligned at all and it uses 4k for .data), so ld ends up putting everything in the same segment which ends up RWE which the new ld barfs about (and is a bad idea anyways).

Now, It can be overriden from the command line, so one solution would be to fix any user of gnu-efi (and the bundled magic Makefile fragments that Peter conveniently added but aren't upstream) to pass -z max-page-size=4096. That would have to be added to things like systemd-boot I suspect and possibly others.

The approach I chose instead (patch also sent to upstream gnu-efi mailing list without feedback so far) is to change the .lds to align everything to 64k on aa64.

HOWEVER: I just noticed that the .mk fragments that Fedora provides (not upstream) explicitely pass -DPAGE_SIZE=4096 -DPAGE_SHIFT=12 .. .that will probably need fixing, so that PR is incomplete, but I want to start the discussion (I'll send a fix for that later).

Is this the approach we want to take ? Or did I miss a bit of spec that says that EFI on aa64 only supports 4k pages ? using -z max-page-size is inconvenient as it can't be self contained in the .lds ...

@ozbenh
Copy link
Author

ozbenh commented Dec 21, 2022

Note.. the UEFI spec does say that for aarch64:

MMU configuration: Implementations must use only 4k pages and a single translation base
register. On devices supporting multiple translation base registers, TTBR0 must be used solely.
The binding does not mandate whether page tables are cached or un-cached.

So in theory, 4k is mandated as base page size for EFI binary and thus -DPAGE_SIZE is correct.

That means that in theory (again), -z maz-page-size=4096 is the correct approach, but it would require modifying the Makefile of every user of gnu-efi (including systemd-boot).

What do you reckon ? My patch still acts as a workaround...

@ozbenh
Copy link
Author

ozbenh commented Dec 21, 2022

So... we have two issues it seems :-) Passing -z maz-page-size=4096 isn't in itself sufficient. We also need to fix gnu-efi's .lds to properly put .dynamic in its own 4k block otherwise ld still does the wrong thing. I've verified that the page size argument alone doesn't fix it.

trofi and others added 4 commits December 21, 2022 13:36
Without the change there is no guarantee that .o files will be built
after directories are created for them and build fails as:

    gcc -I/build/gnu-efi-code//lib ... -c lib/runtime/rtstr.c -o runtime/rtstr.o
    Assembler messages:
    Fatal error: can't create runtime/rtstr.o: No such file or directory

(cherry picked from commit 2ed6486)
binutils-2.39 enabed a few warning by default
(https://sourceware.org/pipermail/binutils/2022-August/122246.html):

> The ELF linker will now generate a warning message if the stack is made executable.

Let's suppress the warnings in assembly files by adding non-executables
stack markings. This fixes at least systemd build which uses '-Wl,--fatal-warnings':

    systemd/systemd#24226

(cherry picked from commit 803b49c)
The assembly code uses fixed offsets into the jmp_buf and leaves an 8 byte
gap between the GPRs and the FPRs, but the jmp_buf structure was not laid
out to account for this so the code would overrun the jmp_buf by 8 bytes.

Found-by: Oskar Engen <oskar.engen@gmail.com>
Signed-off-by: Dwight Engen <dwight.engen@gmail.com>
(cherry picked from commit 4a566dd)
Fix link with binutils 2.39

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
@ozbenh
Copy link
Author

ozbenh commented Dec 21, 2022

Allright, I've updated the PR. This time, I add 4 patches instead of the 2 mentioned earlier:

  • Missing note section (same)
  • Align .dynamic properly
  • Pass -z max-page-size=4096 when building the apps
  • Pass it as well in aa64.mk

@ozbenh
Copy link
Author

ozbenh commented Dec 21, 2022

There packages might need fixing for the ld argument:

[fedora@xxxxxxxx ~]$ repoquery --enablerepo fedora-source --whatrequires "gnu-efi*"
Last metadata expiration check: 0:03:19 ago on Wed 21 Dec 2022 04:12:28 AM UTC.
efitools-0:1.9.2-8.fc37.src
fwupd-efi-0:1.3-1.fc37.src
gnu-efi-devel-1:3.0.11-10.fc37.noarch
mokutil-2:0.6.0-5.fc37.src
pesign-test-app-0:5-27.fc37.src
sbsigntools-0:0.9.4-9.fc37.src
systemd-0:251.7-611.fc37.src

@ozbenh
Copy link
Author

ozbenh commented Dec 21, 2022

mokutil : seems to build fine without BuildRequires: gnu-efi .. however it does need BuildRequires: which for completeness (buildroot has it but it's never nice to depend implicitely is it ?)

Otherwise it gets blended with .text causing the linker to create
a RWX segment, which is bad and causes a fatal warning with newer
binutils

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
All our lds assume a 4k alignment. Without this, on archs like aarch64
where the linker may assume a larger page size (64k), gnu ld will end up
merging .text and .data sections into the same segment.

This results in the segment being RWX which will cause a fatal warning
(and is a bad thing anyways)

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
This follows the change to apps

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
@ozbenh
Copy link
Author

ozbenh commented Dec 21, 2022

LIttle fixup, let's not move .dynamic completely outside of the PE section definition, keep it in the data one.

BTW. Why do we keep those dynamic reloc sections around in the final PE ? I though they would be only useful if an ELF linker was interested in the object which ... won't happen. Or am I missing something subtle here ?

vathpela pushed a commit that referenced this pull request Nov 22, 2024
Align RTLIB CopyMem/SetMem with normal versions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants