From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout01.posteo.de (mout01.posteo.de [185.67.36.65]) by mx.groups.io with SMTP id smtpd.web10.6560.1682063167750948311 for ; Fri, 21 Apr 2023 00:46:08 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@posteo.de header.s=2017 header.b=oxDroqec; spf=pass (domain: posteo.de, ip: 185.67.36.65, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [185.67.36.169]) by mout01.posteo.de (Postfix) with ESMTPS id 47261240244 for ; Fri, 21 Apr 2023 09:46:05 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1682063165; bh=FpfSnI8BHvgsdQXiW2KkWgq4721LKhrxzhb/dNkaTYQ=; h=From:Subject:Date:Cc:To:From; b=oxDroqecAPLx8S9DJ8i5jCYCLln3JA/NHNuGmHPnFct03PG6YtmbHQ2DMbEdiunas hPwtHojDhuO58De8z2EKA6aVrSbeecg+Fjj/w3RykH12c1JjtsIzaRaqNyz8bQlR9r 0q4bQKqll61dGMkxmJmSMuYzkb0Mt3wMvdmuL5Jtgwwl98jQ0AZAH2eXaLLJ3bUg6w YjoPB7dyn7YTrNhTQEQ8DMZG253y4gQjx0j6WtbEv2vuJanpw3DS8mXlqPlixdV70n M1wcU3JfcnGRH/cciZ9l4bLsrhzPLZX9RZjIYExn1hNNzXqbQNQ/jK/km4vxF5hY1Q YqWM0d5S/LAhA== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4Q2mmV48wBz6tv5; Fri, 21 Apr 2023 09:46:01 +0200 (CEST) From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Mime-Version: 1.0 (1.0) Subject: Re: [PATCH v5 02/10] MdePkg: don't set visibility to hidden Date: Fri, 21 Apr 2023 07:46:01 +0000 Message-Id: References: Cc: Gerd Hoffmann , devel@edk2.groups.io, Yuwei Chen , Oliver Steffen , Bob Feng , Daniel Schaefer , Chao Li , Dongyan Qian , Michael D Kinney , Pawel Polawski , Baoqi Zhang , Leif Lindholm , Rebecca Cran , Zhiguang Liu , Sunil V L , Ard Biesheuvel , Liming Gao In-Reply-To: To: Ard Biesheuvel Content-Type: multipart/alternative; boundary=Apple-Mail-319704F7-7657-4588-840A-62D43297ECC7 Content-Transfer-Encoding: 7bit --Apple-Mail-319704F7-7657-4588-840A-62D43297ECC7 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable > On 21. Apr 2023, at 09:21, Ard Biesheuvel wrote: >=20 > =EF=BB=BFOn Fri, 21 Apr 2023 at 08:49, Gerd Hoffmann w= rote: >>=20 >>> On Fri, Apr 21, 2023 at 06:01:11AM +0000, Marvin H=C3=A4user wrote: >>>=20 >>>> On 21. Apr 2023, at 06:45, Gerd Hoffmann wrote: >>>>=20 >>>> =EF=BB=BFNot needed any more on modern toolchains, they are better >>>> in not creating a GOT without this trick. >>>=20 >>> Hi Gerd, >>>=20 >>> Thanks! Just out of interest, how did you test this and what were the re= sults? >>=20 >> Patch #1, adding a linker script assert as suggested by ard, then: >>=20 >> * 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) >>=20 >=20 > 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. >=20 > *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) >=20 > 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),%r1= 0 > ffffffff82fa59e3: R_X86_64_REX_GOTPCRELX level3_kernel_pgt-0x4= >=20 > 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. >=20 > 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=E2=80=99ve been toying with using only PIE relocs for X64 for a bit a= nd finally merged it into master, so far no issues: https://github.com/acidanthera/audk/commit/92bb32130bcd0c35e48bdc308a18e5bc7= 4cbaa42 https://github.com/acidanthera/audk/commit/42988773a06f9d6bf345fcbe82c1082ff= 1cfa2af In fact (I *did not* confirm this, it=E2=80=99s only a report I got), it see= ms to fix something regarding the stack protector. I=E2=80=99d not be surpri= sed if there are edge-cases where -q does not get all necessary relocs when P= IE is enabled. Best regards, Marvin= --Apple-Mail-319704F7-7657-4588-840A-62D43297ECC7 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable
On 21. Apr 2023, at 09:21,= Ard Biesheuvel <ardb@kernel.org> wrote:

=EF=BB=BFOn Fri, 21 Apr 2023 a= t 08:49, Gerd Hoffmann <kraxel@redhat.com> wrote:

On Fri, Apr 21, 2023 at 06:01:11AM +0000, Marvin H=C3=A4user wrote:=
<= /span>
On 21. Apr 2023, at 06:45, Gerd H= offmann <kraxel@redhat.com> wrote:

=EF=BB=BFNot needed any more on modern toolchains, they are better
in not creating a GOT wi= thout this trick.

Hi Gerd,=

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

Patch #1, adding a linker script assert as suggested by ard, the= n:

* compile + test on my local workstat= ion (fedora 37, gcc 12).
* run CI
* comp= ile 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
<= span>entries if they are emitted. I'm not sure how often that getsexercised, 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 int= o account is the fact that
relaxations are not usually refle= cted in the relocations emitted by
the compiler when using -= -emit-relocs. So we might end up with
occurrences like the b= elow (taken from the Linux kernel but the idea
is the same)<= /span>

ffffffff82fa59d5:     &= nbsp; 4c 8d 0d 24 66 88 ff    lea    -0x7= 799dc(%rip),%r9
     ffffffff82fa5= 9d8: R_X86_64_REX_GOTPCRELX        level4= _kernel_pgt-0x4
ffffffff82fa59dc:     &n= bsp; 49 8d 69 67          =    lea    0x67(%r9),%rbp
fffff= fff82fa59e0:       4c 8d 15 19 76 88 ff  =   lea    -0x7789e7(%rip),%r10
&nbs= p;    ffffffff82fa59e3: R_X86_64_REX_GOTPCRELX  &nb= sp;     level3_kernel_pgt-0x4

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

= So it would be better to ASSERT() on non-empty GOT, and ignore suchGOTPCREL relocations instead of attempting to relocate the GOT entri= es
they (used to) refer to.
Hmm, we=E2=80=99ve 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/aci= danthera/audk/commit/92bb32130bcd0c35e48bdc308a18e5bc74cbaa42
https://github.com/a= cidanthera/audk/commit/42988773a06f9d6bf345fcbe82c1082ff1cfa2af

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

Best regards,
Marvin
= --Apple-Mail-319704F7-7657-4588-840A-62D43297ECC7--