From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from rn-mailsvcp-ppex-lapp15.apple.com (rn-mailsvcp-ppex-lapp15.apple.com [17.179.253.34]) by mx.groups.io with SMTP id smtpd.web12.369.1616628362208624728 for ; Wed, 24 Mar 2021 16:26:02 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=qZ6MpwMA; spf=pass (domain: apple.com, ip: 17.179.253.34, mailfrom: afish@apple.com) Received: from pps.filterd (rn-mailsvcp-ppex-lapp15.rno.apple.com [127.0.0.1]) by rn-mailsvcp-ppex-lapp15.rno.apple.com (8.16.0.43/8.16.0.43) with SMTP id 12ONNue5022755; Wed, 24 Mar 2021 16:26:00 -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=WrkNJMxUPYxKX+XsfVKAGsmcWbx5uksMUPkvnRVj58A=; b=qZ6MpwMA5E9X/dl73+MQsZtx1GCd5ewDtXEvgvFdvH2mpwMW/FVTuvefo+yg/xSNNuFX 6h5bIoukocKo6lkIDqlpcQJBWKhSHFAQL9AmPV8ydF4BGT8NaFI0UtFv0UqaCDn4dLVc aSHS53my/yuRWBUhuuvahfDZan3xjPdpkos/1ZrF7oL0HlSMpi0alVnNdiMIEV7NNGpP 7ooejhFzZNBr0ayYXOOCw2IGM8yZegwwJjwBvS0WDsWehyjqFMAgr4uQy6zHy+N4kGrO BtyanTUL/BHk+iujb8MX434e1zfAUO9lvhnoeh6fEjUX1xuSb/0ueUWpphGjs0BiBW1x Sw== Received: from rn-mailsvcp-mta-lapp03.rno.apple.com (rn-mailsvcp-mta-lapp03.rno.apple.com [10.225.203.151]) by rn-mailsvcp-ppex-lapp15.rno.apple.com with ESMTP id 37dembaybk-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Wed, 24 Mar 2021 16:26:00 -0700 Received: from rn-mailsvcp-mmp-lapp01.rno.apple.com (rn-mailsvcp-mmp-lapp01.rno.apple.com [17.179.253.14]) by rn-mailsvcp-mta-lapp03.rno.apple.com (Oracle Communications Messaging Server 8.1.0.7.20201203 64bit (built Dec 3 2020)) with ESMTPS id <0QQH010BRYFCFCI0@rn-mailsvcp-mta-lapp03.rno.apple.com>; Wed, 24 Mar 2021 16:26:00 -0700 (PDT) Received: from process_milters-daemon.rn-mailsvcp-mmp-lapp01.rno.apple.com by rn-mailsvcp-mmp-lapp01.rno.apple.com (Oracle Communications Messaging Server 8.1.0.7.20201203 64bit (built Dec 3 2020)) id <0QQH00M00Y0ZAR00@rn-mailsvcp-mmp-lapp01.rno.apple.com>; Wed, 24 Mar 2021 16:26:00 -0700 (PDT) X-Va-A: X-Va-T-CD: cb122c4d92e6fbe810bf13cd7c8a558b X-Va-E-CD: 9a4ee0c196a465cee3151bf532b18c49 X-Va-R-CD: 323ef448fbae37932f95489e7d7da54b X-Va-CD: 0 X-Va-ID: 98c6e341-f7ca-45f4-88f0-95249559ee2f X-V-A: X-V-T-CD: cb122c4d92e6fbe810bf13cd7c8a558b X-V-E-CD: 9a4ee0c196a465cee3151bf532b18c49 X-V-R-CD: 323ef448fbae37932f95489e7d7da54b X-V-CD: 0 X-V-ID: 71a3c00a-a3b2-45a7-a5cb-1f25d7fc5ad2 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.369,18.0.761 definitions=2021-03-24_13:2021-03-24,2021-03-24 signatures=0 Received: from [17.235.46.127] (unknown [17.235.46.127]) by rn-mailsvcp-mmp-lapp01.rno.apple.com (Oracle Communications Messaging Server 8.1.0.7.20201203 64bit (built Dec 3 2020)) with ESMTPSA id <0QQH00GJQYFATW00@rn-mailsvcp-mmp-lapp01.rno.apple.com>; Wed, 24 Mar 2021 16:26:00 -0700 (PDT) From: "Andrew Fish" Message-id: 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: Wed, 24 Mar 2021 16:25:58 -0700 In-reply-to: <20210324115819.605436-1-ross.burton@arm.com> Cc: lersek@redhat.com To: edk2-devel-groups-io , ross@burtonini.com References: <20210324115819.605436-1-ross.burton@arm.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-24_13:2021-03-24,2021-03-24 signatures=0 Content-type: multipart/alternative; boundary="Apple-Mail=_19A847C6-099A-4FA4-A9AE-3436237C6D17" --Apple-Mail=_19A847C6-099A-4FA4-A9AE-3436237C6D17 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 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 It looks to me that the root cause of this issue might be that the GenFw i= s blindly writing the debug directory entry into the PE/COFF? For native PE= /COFF I think this is controlled by linker flags? For Xcode/clang it is con= trolled 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 release builds.=20 It seems kind of strange to insert a section and then zero it. Almost seem= s like the intent of =E2=80=94zero was to post process compare images?=20 -z, --zero Zero the Debug Data Fields in the PE input image f= ile. It also zeros the time stamp fields. This option can be used to compare the binary efi = 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 l= ater input action option will override the previous one= . And in case you are going to ask our fork uses relative paths from the Bui= ld directory and/or a UUID string for the Debug Directory entry file name s= o it is a constant value and does not impact build reproducibility.=20 >>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 handl= er 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 too= ls that natively produce PE/COFF and did not have a Debug Directory entry t= he same thing would happen prior to this change.=20 Status =3D PeCoffLoaderGetEntryPoint ((VOID *) Pe32Data, &EntryPoint); if (EFI_ERROR (Status)) { EntryPoint =3D NULL; } InternalPrintMessage ("!!!! Find image based on IP(0x%x) ", CurrentEip= ); 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 ); 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, bu= t that we have no way to turn off the Debug Directory Entry for ELF convers= ion, so we seem to be working around that issue with a bigger hammer? [1] https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/CpuEx= ceptionHandlerLib/CpuExceptionCommon.c#L117 Thanks, Andrew Fish > 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 fil= e, > which means the output files have build paths embedded in them. To redu= ce > information leakage and ensure reproducible builds, pass --zero in relea= se > 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.dsc > 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 "CLANGPDB" > 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 >=20 >=20 --Apple-Mail=_19A847C6-099A-4FA4-A9AE-3436237C6D17 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 This breaks some usage we= have have in our fork. We have symbols turned on for Release builds, so th= is change would break that. 

It looks to me that the root cause of this issue might be that t= he GenFw is blindly writing the debug directory entry into the PE/COFF? For= native PE/COFF I think this is controlled by linker flags? For Xcode/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 release= builds. 


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

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

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 Director= y entry file name so it is a constant value and does not impact build repro= ducibility. 

From a feature stand point this change will break any hope of sour= ce level debugging with RELEASE builds. I also think it changes the excepti= on handler 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= entry the same thing would happen prior to this change. 
=

}<= /td> = );=
Status =3D PeCoffLoaderGetEntryPoint ((VOID *) Pe32Data, = &EntryPoint);
if (EFI_ERROR (Stat= us)) {
EntryPoint =3D NULL;
Inter= nalPrintMessage ("!!!! Find image based on IP(0x%x) ", CurrentEip);
PdbPointer =3D PeCoffLoaderGetPdbPointer ((VOID *) Pe32Data);
if (PdbPointer= !=3D NULL) {
InternalPrintMessage ("%a", PdbPointer);
} else {
InternalPrintMessage ("(No PDB) " );
}
Internal= PrintMessage (
" (ImageBase=3D%016lp, EntryPoint=3D%016p) !!!!\n",
(VOID *) Pe32Data,
EntryPoint

Not say= ing we have to "stop the presses", but just trying to point out the side ef= fects of this change. It is not so much that this change is bad, but that w= e have no way to turn off the Debug Directory Entry for ELF conversion, so = we seem to be working around that issue with a bigger hammer?
<= div class=3D"">
[1] https://github.com/tianocor= e/edk2/blob/master/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCo= mmon.c#L117

Thanks,

Andrew Fish

On Mar 24, 2021, at 4:58 AM, Ross Burton <<= a href=3D"mailto:ross@burtonini.com" class=3D"">ross@burtonini.com> = wrote:

GenFw will embed a NB10 section which contains the path to the inp= ut file,
which means the output files have build paths embedd= ed in them.  To reduce
information leakage and ensure re= producible builds, pass --zero in release
builds to remove th= is information.

Ref: https://bugzilla.tianoc= ore.org/show_bug.cgi?id=3D3256
Signed-off-by: Ross Burton= <ross.burton@arm.com<= /a>>
---
OvmfPkg/AmdSev/AmdSevX64.dsc | 1 +=
OvmfPkg/Bhyve/BhyveX64.dsc   | 1 +
= OvmfPkg/OvmfPkgIa32.dsc      | 1 +
= OvmfPkg/OvmfPkgIa32X64.dsc   | 1 +
OvmfPkg/OvmfPk= gX64.dsc       | 1 +
OvmfPkg/O= vmfXen.dsc          | 1 +
6 files changed, 6 insertions(+)

di= ff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index 65c42284d9..69a05feea9 100644
--- a/OvmfPkg/A= mdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -78,6 +78,7 @@
  GCC:*_*_X64_GENFW_FLAG= S   =3D --keepexceptiontable
  INTEL:*_*= _X64_GENFW_FLAGS =3D --keepexceptiontable
!endif
+  RELEASE_*_*_GENFW_FLAGS =3D --zero

=   #
  # Disable deprecated APIs.
diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.ds= c
index 4a1cdf5aca..132f55cf69 100644
--- a/Ovm= fPkg/Bhyve/BhyveX64.dsc
+++ b/OvmfPkg/Bhyve/BhyveX64.dsc
@@ -76,6 +76,7 @@
  GCC:*_*_X64_GENFW_FL= AGS   =3D --keepexceptiontable
  INTEL:*= _*_X64_GENFW_FLAGS =3D --keepexceptiontable
!endif
+  RELEASE_*_*_GENFW_FLAGS =3D --zero

  #
  # Disable deprecated APIs.=
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.d= sc
index 1eaf3e99c6..93c209950c 100644
--- a/Ov= mfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -80,6 +80,7 @@
!if $(TOOL_CHAIN_TAG) !=3D "XCODE5"= && $(TOOL_CHAIN_TAG) !=3D "CLANGPDB"
  GC= C:*_*_*_CC_FLAGS          &nbs= p;        =3D -mno-mmx -mno-sse
!endif
+  RELEASE_*_*_GENFW_FLAGS =3D --zero=

  #
  # D= isable deprecated APIs.
diff --git a/OvmfPkg/OvmfPkgIa32X64.d= sc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 4a5a430147..97cc438250 = 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/O= vmfPkg/OvmfPkgIa32X64.dsc
@@ -84,6 +84,7 @@
&n= bsp; GCC:*_*_X64_GENFW_FLAGS   =3D --keepexceptiontable
  INTEL:*_*_X64_GENFW_FLAGS =3D --keepexceptiontable !endif
+  RELEASE_*_*_GENFW_FLAGS =3D --ze= ro

  #
  #= Disable deprecated APIs.
diff --git a/OvmfPkg/OvmfPkgX64.dsc= b/OvmfPkg/OvmfPkgX64.dsc
index d4d601b444..f544fb04bf 100644=
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/Ovm= fPkgX64.dsc
@@ -84,6 +84,7 @@
  GCC:= *_*_X64_GENFW_FLAGS   =3D --keepexceptiontable
&nb= sp; INTEL:*_*_X64_GENFW_FLAGS =3D --keepexceptiontable
= !endif
+  RELEASE_*_*_GENFW_FLAGS =3D --zero

  #
  # Disable depre= cated APIs.
diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXe= n.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_GEN= FW_FLAGS =3D --keepexceptiontable
!endif
+ &nb= sp;RELEASE_*_*_GENFW_FLAGS =3D --zero

 &= nbsp;#
  # Disable deprecated APIs.
= --
2.25.1







--Apple-Mail=_19A847C6-099A-4FA4-A9AE-3436237C6D17--