Re: [PATCH v5 02/10] MdePkg: don't set visibility to hidden

["Marvin Häuser" <mhaeuser@posteo.de>]

[-- Attachment #1: Type: text/plain, Size: 2717 bytes --]


> 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

[-- Attachment #2: Type: text/html, Size: 5704 bytes --]