On 21. Apr 2023, at 09:21, Ard Biesheuvel <ardb@kernel.org> wrote:

On Fri, 21 Apr 2023 at 08:49, Gerd Hoffmann <kraxel@redhat.com> wrote:

On Fri, Apr 21, 2023 at 06:01:11AM +0000, Marvin Häuser wrote:

On 21. Apr 2023, at 06:45, Gerd Hoffmann <kraxel@redhat.com> wrote:

Not needed any more on modern toolchains, they are better
in not creating a GOT without this trick.

Hi Gerd,

Thanks! Just out of interest, how did you test this and what were the results?

Patch #1, adding a linker script assert as suggested by ard, then:

* compile + test on my local workstation (fedora 37, gcc 12).
* run CI
* compile on some older distros:
  - rhel-8 (gcc 8)
  - ubuntu-18.04 (gcc 7)


I just realized that on x86, GenFw has some code to deal with GOT
entries if they are emitted. I'm not sure how often that gets
exercised, given our prior use of hidden visibility, but at least the
GOT entries should be covered by relocations if they exist.

*However*, one thing we are not taking into account is the fact that
relaxations are not usually reflected in the relocations emitted by
the compiler when using --emit-relocs. So we might end up with
occurrences like the below (taken from the Linux kernel but the idea
is the same)

ffffffff82fa59d5:       4c 8d 0d 24 66 88 ff    lea    -0x7799dc(%rip),%r9
     ffffffff82fa59d8: R_X86_64_REX_GOTPCRELX        level4_kernel_pgt-0x4
ffffffff82fa59dc:       49 8d 69 67             lea    0x67(%r9),%rbp
ffffffff82fa59e0:       4c 8d 15 19 76 88 ff    lea    -0x7789e7(%rip),%r10
     ffffffff82fa59e3: R_X86_64_REX_GOTPCRELX        level3_kernel_pgt-0x4

So here, the GOT loads have been relaxed into LEA instructions, but
GenFw will decode the immediate and assume it points to the GOT entry
rather than the variable itself, and happily emit a PE relocation for
it.

So it would be better to ASSERT() on non-empty GOT, and ignore such
GOTPCREL relocations instead of attempting to relocate the GOT entries
they (used to) refer to.

Hmm, we’ve been toying with using only PIE relocs for X64 for a bit and finally merged it into master, so far no issues:
https://github.com/acidanthera/audk/commit/92bb32130bcd0c35e48bdc308a18e5bc74cbaa42
https://github.com/acidanthera/audk/commit/42988773a06f9d6bf345fcbe82c1082ff1cfa2af

In fact (I *did not* confirm this, it’s only a report I got), it seems to fix something regarding the stack protector. I’d not be surprised if there are edge-cases where -q does not get all necessary relocs when PIE is enabled.

Best regards,
Marvin