From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from rn-mailsvcp-ppex-lapp14.apple.com (rn-mailsvcp-ppex-lapp14.apple.com [17.179.253.33]) by mx.groups.io with SMTP id smtpd.web08.2481.1616701396430438079 for ; Thu, 25 Mar 2021 12:43:16 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=iWJFGDWH; spf=pass (domain: apple.com, ip: 17.179.253.33, mailfrom: afish@apple.com) Received: from pps.filterd (rn-mailsvcp-ppex-lapp14.rno.apple.com [127.0.0.1]) by rn-mailsvcp-ppex-lapp14.rno.apple.com (8.16.0.43/8.16.0.43) with SMTP id 12PJbQGD013970; Thu, 25 Mar 2021 12:43:11 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=apple.com; h=from : message-id : content-type : mime-version : subject : date : in-reply-to : cc : to : references; s=20180706; bh=1bTEfJPWcb33N66UskOZBBRMyBiCqXSXoPNq2xEppIs=; b=iWJFGDWHG8i9jfKBpb+5ZDSlXGIgX1HyGLsctkoBgFXU8UPp+G6C7FYcY0LXNKOWM4E5 Xj0OdK/LEA1sHuo9dbyV3XZHQX5RDMRtuPX/5lMZlxuP+KAdIFxfwYA5WNwLEWAsSPwn BOBcpkL59rcDoh5HSxD4xMRbULT3iTWSC8v0zZTp2DqSx8JeQccx0Fqwtz1Q1qQjHByx SSXqnST1F6k4biajUs4oZek/f/Cke9dSIcTsE+USIqJRme1rNX7rLggIaV6A6iO7DByD bDi6H1FV6hdRmxb46sADFXLvg+YsJemv/t9zezarJav08gyNXjKPRDbkEKQv3ke8BT7j lA== Received: from rn-mailsvcp-mta-lapp04.rno.apple.com (rn-mailsvcp-mta-lapp04.rno.apple.com [10.225.203.152]) by rn-mailsvcp-ppex-lapp14.rno.apple.com with ESMTP id 37ddxarhmx-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Thu, 25 Mar 2021 12:43:11 -0700 Received: from rn-mailsvcp-mmp-lapp04.rno.apple.com (rn-mailsvcp-mmp-lapp04.rno.apple.com [17.179.253.17]) by rn-mailsvcp-mta-lapp04.rno.apple.com (Oracle Communications Messaging Server 8.1.0.7.20201203 64bit (built Dec 3 2020)) with ESMTPS id <0QQJ00K8TIRZV7I0@rn-mailsvcp-mta-lapp04.rno.apple.com>; Thu, 25 Mar 2021 12:43:11 -0700 (PDT) Received: from process_milters-daemon.rn-mailsvcp-mmp-lapp04.rno.apple.com by rn-mailsvcp-mmp-lapp04.rno.apple.com (Oracle Communications Messaging Server 8.1.0.7.20201203 64bit (built Dec 3 2020)) id <0QQJ00S00IGJKP00@rn-mailsvcp-mmp-lapp04.rno.apple.com>; Thu, 25 Mar 2021 12:43:11 -0700 (PDT) X-Va-A: X-Va-T-CD: 13715775cfe6ed78bc954dbcb503dbb2 X-Va-E-CD: 9a4ee0c196a465cee3151bf532b18c49 X-Va-R-CD: 323ef448fbae37932f95489e7d7da54b X-Va-CD: 0 X-Va-ID: 89e36a3c-7632-4c39-a427-3dd840ed8fbc X-V-A: X-V-T-CD: 13715775cfe6ed78bc954dbcb503dbb2 X-V-E-CD: 9a4ee0c196a465cee3151bf532b18c49 X-V-R-CD: 323ef448fbae37932f95489e7d7da54b X-V-CD: 0 X-V-ID: fae162de-18f0-41fc-9b2c-3ca850ff3ffd X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.369,18.0.761 definitions=2021-03-25_07:2021-03-25,2021-03-25 signatures=0 Received: from [17.235.46.127] (unknown [17.235.46.127]) by rn-mailsvcp-mmp-lapp04.rno.apple.com (Oracle Communications Messaging Server 8.1.0.7.20201203 64bit (built Dec 3 2020)) with ESMTPSA id <0QQJ00QJXIRX5C00@rn-mailsvcp-mmp-lapp04.rno.apple.com>; Thu, 25 Mar 2021 12:43:11 -0700 (PDT) From: "Andrew Fish" Message-id: <9EB7D6D7-04A7-49D7-8038-C433EEA5E46D@apple.com> MIME-version: 1.0 (Mac OS X Mail 14.0 \(3654.20.0.2.1\)) Subject: Re: [edk2-devel] [PATCH v2] OvmfPkg: strip build paths in release builds Date: Thu, 25 Mar 2021 12:43:09 -0700 In-reply-to: <95cbe27d-afbe-84f3-3473-84456379ebe2@redhat.com> Cc: edk2-devel-groups-io , ross@burtonini.com, ross.burton@arm.com, "Ard Biesheuvel (TianoCore)" To: Laszlo Ersek References: <20210324115819.605436-1-ross.burton@arm.com> <95cbe27d-afbe-84f3-3473-84456379ebe2@redhat.com> X-Mailer: Apple Mail (2.3654.20.0.2.1) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.369,18.0.761 definitions=2021-03-25_07:2021-03-25,2021-03-25 signatures=0 Content-type: multipart/alternative; boundary="Apple-Mail=_D748D055-5BA6-4AE4-85B1-923966B9A280" --Apple-Mail=_D748D055-5BA6-4AE4-85B1-923966B9A280 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On Mar 25, 2021, at 10:19 AM, Laszlo Ersek wrote: >=20 > On 03/25/21 00:25, Andrew Fish wrote: >> This breaks some usage we have have in our fork. We have symbols turned= on for Release builds, so this change would break that.=20 >>=20 >> It looks to me that the root cause of this issue might be that the GenF= w is blindly writing the debug directory entry into the PE/COFF? >=20 > Yes, that's my understanding, from TianoCore#3256. >=20 >> For native PE/COFF I think this is controlled by linker flags? For Xcod= e/clang it is controlled by the *_XCODE5_*_MTOC_FLAGS. So at this point it = is kind of up to each toolchain how they want to deal with symbols on relea= se builds.=20 >>=20 >>=20 >> It seems kind of strange to insert a section and then zero it. Almost s= eems like the intent of =E2=80=94zero was to post process compare images?= =20 >>=20 >> -z, --zero Zero the Debug Data Fields in the PE input image= file. >> It also zeros the time stamp fields. >> This option can be used to compare the binary ef= i image. >> It can't be combined with other action options >> except for -o, -r option. It is a action option. >> If it is combined with other action options, the= later >> input action option will override the previous o= ne. >>=20 >> And in case you are going to ask our fork uses relative paths from the = Build directory and/or a UUID string for the Debug Directory entry file nam= e so it is a constant value and does not impact build reproducibility.=20 >=20 > I'd like if we could satisfy both your use case and Ross's (Yocto's). >=20 > Until we have a technical solution for that, is it important that we > revert the patch upstream? (If it's urgent, I'm going to ask someone > else to do that, because I'll be back in April.) >=20 Laszlo, Per my other email about =E2=80=94keepexceptiontable it looks like source = level debug is intertwined with GenFw flags that are global, and this has b= een going on for a long time. So I think I=E2=80=99ll just file a BZ about = this issue in general and not ask for changes in this patch.=20 Thanks, Andrew Fish >> From a feature stand point this change will break any hope of source le= vel debugging with RELEASE builds. I also think it changes the exception ha= ndler code output in OVMF [1] for ELF toolchains. You are going to get the = (No PDB) vs. the file and path you were getting today. I assume if you had = tools that natively produce PE/COFF and did not have a Debug Directory entr= y the same thing would happen prior to this change.=20 >>=20 >> Status =3D PeCoffLoaderGetEntryPoint ((VOID *) Pe32Data, &EntryPoint= ); >> if (EFI_ERROR (Status)) { >> EntryPoint =3D NULL; >> } >> InternalPrintMessage ("!!!! Find image based on IP(0x%x) ", CurrentE= ip); >> PdbPointer =3D PeCoffLoaderGetPdbPointer ((VOID *) Pe32Data); >> if (PdbPointer !=3D NULL) { >> InternalPrintMessage ("%a", PdbPointer); >> } else { >> InternalPrintMessage ("(No PDB) " ); >> } >> InternalPrintMessage ( >> " (ImageBase=3D%016lp, EntryPoint=3D%016p) !!!!\n", >> (VOID *) Pe32Data, >> EntryPoint >> ); >>=20 >> Not saying we have to "stop the presses", but just trying to point out = the side effects of this change. It is not so much that this change is bad,= but that we have no way to turn off the Debug Directory Entry for ELF conv= ersion, so we seem to be working around that issue with a bigger hammer? >=20 > I don't have suggestions alas, but am open to any solution that works > for you and Ross both. >=20 > Thanks (and my apologies for breaking your process!), > Laszlo >=20 >>=20 >> [1] https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/Cp= uExceptionHandlerLib/CpuExceptionCommon.c#L117 >>=20 >> Thanks, >>=20 >> Andrew Fish >>=20 >>> On Mar 24, 2021, at 4:58 AM, Ross Burton wrote: >>>=20 >>> GenFw will embed a NB10 section which contains the path to the input f= ile, >>> which means the output files have build paths embedded in them. To re= duce >>> information leakage and ensure reproducible builds, pass --zero in rel= ease >>> builds to remove this information. >>>=20 >>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3256 >>> Signed-off-by: Ross Burton >>> --- >>> OvmfPkg/AmdSev/AmdSevX64.dsc | 1 + >>> OvmfPkg/Bhyve/BhyveX64.dsc | 1 + >>> OvmfPkg/OvmfPkgIa32.dsc | 1 + >>> OvmfPkg/OvmfPkgIa32X64.dsc | 1 + >>> OvmfPkg/OvmfPkgX64.dsc | 1 + >>> OvmfPkg/OvmfXen.dsc | 1 + >>> 6 files changed, 6 insertions(+) >>>=20 >>> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.d= sc >>> index 65c42284d9..69a05feea9 100644 >>> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc >>> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc >>> @@ -78,6 +78,7 @@ >>> GCC:*_*_X64_GENFW_FLAGS =3D --keepexceptiontable >>> INTEL:*_*_X64_GENFW_FLAGS =3D --keepexceptiontable >>> !endif >>> + RELEASE_*_*_GENFW_FLAGS =3D --zero >>>=20 >>> # >>> # Disable deprecated APIs. >>> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc >>> index 4a1cdf5aca..132f55cf69 100644 >>> --- a/OvmfPkg/Bhyve/BhyveX64.dsc >>> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc >>> @@ -76,6 +76,7 @@ >>> GCC:*_*_X64_GENFW_FLAGS =3D --keepexceptiontable >>> INTEL:*_*_X64_GENFW_FLAGS =3D --keepexceptiontable >>> !endif >>> + RELEASE_*_*_GENFW_FLAGS =3D --zero >>>=20 >>> # >>> # Disable deprecated APIs. >>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc >>> index 1eaf3e99c6..93c209950c 100644 >>> --- a/OvmfPkg/OvmfPkgIa32.dsc >>> +++ b/OvmfPkg/OvmfPkgIa32.dsc >>> @@ -80,6 +80,7 @@ >>> !if $(TOOL_CHAIN_TAG) !=3D "XCODE5" && $(TOOL_CHAIN_TAG) !=3D "CLANGPD= B" >>> GCC:*_*_*_CC_FLAGS =3D -mno-mmx -mno-sse >>> !endif >>> + RELEASE_*_*_GENFW_FLAGS =3D --zero >>>=20 >>> # >>> # Disable deprecated APIs. >>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc >>> index 4a5a430147..97cc438250 100644 >>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc >>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc >>> @@ -84,6 +84,7 @@ >>> GCC:*_*_X64_GENFW_FLAGS =3D --keepexceptiontable >>> INTEL:*_*_X64_GENFW_FLAGS =3D --keepexceptiontable >>> !endif >>> + RELEASE_*_*_GENFW_FLAGS =3D --zero >>>=20 >>> # >>> # Disable deprecated APIs. >>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc >>> index d4d601b444..f544fb04bf 100644 >>> --- a/OvmfPkg/OvmfPkgX64.dsc >>> +++ b/OvmfPkg/OvmfPkgX64.dsc >>> @@ -84,6 +84,7 @@ >>> GCC:*_*_X64_GENFW_FLAGS =3D --keepexceptiontable >>> INTEL:*_*_X64_GENFW_FLAGS =3D --keepexceptiontable >>> !endif >>> + RELEASE_*_*_GENFW_FLAGS =3D --zero >>>=20 >>> # >>> # Disable deprecated APIs. >>> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc >>> index 507029404f..fcaa35acf1 100644 >>> --- a/OvmfPkg/OvmfXen.dsc >>> +++ b/OvmfPkg/OvmfXen.dsc >>> @@ -74,6 +74,7 @@ >>> GCC:*_*_X64_GENFW_FLAGS =3D --keepexceptiontable >>> INTEL:*_*_X64_GENFW_FLAGS =3D --keepexceptiontable >>> !endif >>> + RELEASE_*_*_GENFW_FLAGS =3D --zero >>>=20 >>> # >>> # Disable deprecated APIs. >>> --=20 >>> 2.25.1 >>>=20 >>>=20 >>>=20 >>>=20 --Apple-Mail=_D748D055-5BA6-4AE4-85B1-923966B9A280 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

On Mar 25, 2= 021, at 10:19 AM, Laszlo Ersek <lersek@redhat.com> wrote:

On = 03/25/21 00:25, Andrew Fish wrote:
This breaks some usage we have have in our fork.= We have symbols turned on for Release builds, so this change would break t= hat. 

It looks to me that the root cause of this issue might be that th= e GenFw is blindly writing the debug directory entry into the PE/COFF?

Yes= , that's my understanding, from TianoCore#3256.

For native PE/COFF I think this is controlled by linker flags? For X= code/clang it is controlled by the *_XCODE5_*_MTOC_FLAGS. So at this point = it is kind of up to each toolchain how they want to deal with symbols on re= lease builds. 


It seems kind of strange to insert a s= ection and then zero it. Almost seems like the intent of =E2=80=94zero was = to post process compare images? =

 -z, --zero    &nb= sp;       Zero the Debug Data Fields in = the PE input image file.
      =             &nb= sp;    It also zeros the time stamp fields.
            =            This opti= on can be used to compare the binary efi image.
  &= nbsp;           &nbs= p;        It can't be combined with= other action options
      &nb= sp;            =     except for -o, -r option. It is a action option.          &nb= sp;            = If it is combined with other action options, the later
 =             &nb= sp;         input action optio= n will override the previous one.

And in case = you are going to ask our fork uses relative paths from the Build directory = and/or a UUID string for the Debug Directory entry file name so it is a con= stant value and does not impact build reproducibility. 

I'd like if we could satisfy both your use cas= e and Ross's (Yocto's).

Until we have a technical solution = for that, is it important that we
revert the patch upstream? (If it's urgent, I'm going to ask = someone
else to do that= , because I'll be back in April.)


Laszlo,

Per my oth= er email about =E2=80=94keepexceptiontable it looks like source level debug= is intertwined with GenFw flags that are global, and this has been going o= n for a long time. So I think I=E2=80=99ll just file a BZ about this issue = in general and not ask for changes in this patch. 

Thanks,

Andrew Fish
From a feature stand point this change will break any hope= of source level debugging with RELEASE builds. I also think it changes the= exception handler code output in OVMF [1] for ELF toolchains. You are goin= g to get the (No PDB) vs. the file and path you were getting today. I assum= e if you had tools that natively produce PE/COFF and did not have a Debug D= irectory entry the same thing would happen prior to this change. 

&nbs= p;  Status =3D PeCoffLoaderGetEntryPoint ((VOID *) Pe32Data, &= ;EntryPoint);
   if (EFI_ERROR (Status)) {
     EntryPoint =3D NULL;
   }
   InternalPrintMessage (= "!!!! Find image based on IP(0x%x) ", CurrentEip);
 &nbs= p; PdbPointer =3D PeCoffLoaderGetPdbPointer ((VOID *) Pe32Data);
   if (PdbPointer !=3D NULL) {
 =     InternalPrintMessage ("%a", PdbPointer);
   } else {
    &nb= sp;InternalPrintMessage ("(No PDB) " );
   }   InternalPrintMessage (
 &= nbsp;   " (ImageBase=3D%016lp, EntryPoint=3D%016p) !!!!\n",<= br class=3D"">     (VOID *) Pe32Data,
     EntryPoint
   =   );

Not saying we have to "stop the= presses", but just trying to point out the side effects of this change. It= is not so much that this change is bad, but that we have no way to turn of= f the Debug Directory Entry for ELF conversion, so we seem to be working ar= ound that issue with a bigger hammer?

I don't have suggestions alas, but am= open to any solution that works
for you and Ross both.

Thanks (and my apologies f= or breaking your process!),
Laszlo


[1] https://github.com/tianoco= re/edk2/blob/master/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionC= ommon.c#L117

Thanks,

Andrew Fish

On Mar 24, 2021, at 4:58 AM, Ross Burton <ross@burtonini.com> wrote:

GenFw will embed a NB10 section which contains the path t= o the input file,
which means the output files have build pat= hs embedded in them.  To reduce
information leakage and = ensure reproducible builds, pass --zero in release
builds to = remove this information.

Ref: https://bugzil= la.tianocore.org/show_bug.cgi?id=3D3256
Signed-off-by: Ro= ss Burton <ross.burton= @arm.com>
---
OvmfPkg/AmdSev/AmdSevX64.d= sc | 1 +
OvmfPkg/Bhyve/BhyveX64.dsc   | 1 +
OvmfPkg/OvmfPkgIa32.dsc      | 1 +
OvmfPkg/OvmfPkgIa32X64.dsc   | 1 +
OvmfPkg/O= vmfPkgX64.dsc       | 1 +
OvmfP= kg/OvmfXen.dsc          | 1 +<= br class=3D"">6 files changed, 6 insertions(+)

diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dscindex 65c42284d9..69a05feea9 100644
--- a/OvmfPk= g/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -78,6 +78,7 @@
 GCC:*_*_X64_GENFW_FLAGS &n= bsp; =3D --keepexceptiontable
 INTEL:*_*_X64_GENFW_= FLAGS =3D --keepexceptiontable
!endif
+  R= ELEASE_*_*_GENFW_FLAGS =3D --zero

 #
 # Disable deprecated APIs.
diff --git a/OvmfP= kg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
index 4a1c= df5aca..132f55cf69 100644
--- a/OvmfPkg/Bhyve/BhyveX64.dsc+++ b/OvmfPkg/Bhyve/BhyveX64.dsc
@@ -76,6 +76,7 @= @
 GCC:*_*_X64_GENFW_FLAGS   =3D --keepexcepti= ontable
 INTEL:*_*_X64_GENFW_FLAGS =3D --keepexceptionta= ble
!endif
+  RELEASE_*_*_GENFW_FLAGS =3D = --zero

 #
 # Disable d= eprecated APIs.
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPk= g/OvmfPkgIa32.dsc
index 1eaf3e99c6..93c209950c 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa3= 2.dsc
@@ -80,6 +80,7 @@
!if $(TOOL_CHAIN_TAG) != = =3D "XCODE5" && $(TOOL_CHAIN_TAG) !=3D "CLANGPDB"
&n= bsp;GCC:*_*_*_CC_FLAGS         &nbs= p;         =3D -mno-mmx -mno-s= se
!endif
+  RELEASE_*_*_GENFW_FLAGS =3D -= -zero

 #
 # Disable de= precated APIs.
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/Ovmf= Pkg/OvmfPkgIa32X64.dsc
index 4a5a430147..97cc438250 100644--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/Ov= mfPkgIa32X64.dsc
@@ -84,6 +84,7 @@
 GCC:*_= *_X64_GENFW_FLAGS   =3D --keepexceptiontable
 = INTEL:*_*_X64_GENFW_FLAGS =3D --keepexceptiontable
!endif
+  RELEASE_*_*_GENFW_FLAGS =3D --zero

 #
 # Disable deprecated APIs.
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index d4d601b444..f544fb04bf 100644
--- a/OvmfPkg/OvmfPkgX6= 4.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -84,6 +84= ,7 @@
 GCC:*_*_X64_GENFW_FLAGS   =3D --keepexc= eptiontable
 INTEL:*_*_X64_GENFW_FLAGS =3D --keepexcepti= ontable
!endif
+  RELEASE_*_*_GENFW_FLAGS = = =3D --zero

 #
 # Disa= ble deprecated APIs.
diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfP= kg/OvmfXen.dsc
index 507029404f..fcaa35acf1 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc@@ -74,6 +74,7 @@
 GCC:*_*_X64_GENFW_FLAGS &= nbsp; =3D --keepexceptiontable
 INTEL:*_*_X64_GENFW= _FLAGS =3D --keepexceptiontable
!endif
+  = RELEASE_*_*_GENFW_FLAGS =3D --zero

 #
 # Disable deprecated APIs.
-- 
2.25.1


<= /blockquote>

--Apple-Mail=_D748D055-5BA6-4AE4-85B1-923966B9A280--