From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ma1-aaemail-dr-lapp02.apple.com (ma1-aaemail-dr-lapp02.apple.com [17.171.2.68]) by mx.groups.io with SMTP id smtpd.web10.3572.1654725615114872498 for ; Wed, 08 Jun 2022 15:00:15 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=RHbjuCmd; spf=pass (domain: apple.com, ip: 17.171.2.68, mailfrom: afish@apple.com) Received: from pps.filterd (ma1-aaemail-dr-lapp02.apple.com [127.0.0.1]) by ma1-aaemail-dr-lapp02.apple.com (8.16.0.42/8.16.0.42) with SMTP id 258LZSPk006822; Wed, 8 Jun 2022 15:00:14 -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=YAF/unypIvWd0+8peTXFl6C4Wb5npi1JU3mRNfgw1PQ=; b=RHbjuCmdM939aD4wj0mBgDeDBQ+ebKMGaBf6ex1eBMEjuMaEMxZcUbIL1ltpFnvSlv+x duG1NtCJ2vbjkuiBqRhfdF8+GHXfRJHkDESQNUB7xgqt/opBGjpW1RR8NnZBEwCbY0ce fy9tytpEpsoo0FudNSEC/1q69gvkBw5wA4zIqP8v9xrhYXksIlk5+rsfLZyy7Nt27Q8F CmJvNNsesSLdpiL3kwzZArVXq8hxlakRLYlM/jug+Y676SWJcpxO78UbJLhJuDzC+TB+ KX8vrf7dQH+muKnTKO2ugSprPmCF+Szk6jdsgbFe45MI89bOg+ePptRDqhu4z2u+Lir2 lQ== Received: from rn-mailsvcp-mta-lapp03.rno.apple.com (rn-mailsvcp-mta-lapp03.rno.apple.com [10.225.203.151]) by ma1-aaemail-dr-lapp02.apple.com with ESMTP id 3gg42w16cg-5 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Wed, 08 Jun 2022 15:00:14 -0700 Received: from rn-mailsvcp-mmp-lapp03.rno.apple.com (rn-mailsvcp-mmp-lapp03.rno.apple.com [17.179.253.16]) by rn-mailsvcp-mta-lapp03.rno.apple.com (Oracle Communications Messaging Server 8.1.0.18.20220407 64bit (built Apr 7 2022)) with ESMTPS id <0RD600ORTIGCLZ60@rn-mailsvcp-mta-lapp03.rno.apple.com>; Wed, 08 Jun 2022 15:00:12 -0700 (PDT) Received: from process_milters-daemon.rn-mailsvcp-mmp-lapp03.rno.apple.com by rn-mailsvcp-mmp-lapp03.rno.apple.com (Oracle Communications Messaging Server 8.1.0.18.20220407 64bit (built Apr 7 2022)) id <0RD600R00I83R100@rn-mailsvcp-mmp-lapp03.rno.apple.com>; Wed, 08 Jun 2022 15:00:12 -0700 (PDT) X-Va-A: X-Va-T-CD: 19f0bc11d43124e8b595ff154d2eb713 X-Va-E-CD: 656c33f328845d34dcd46237cb7e0482 X-Va-R-CD: f86156c9a5f3cda77129bbd90c65ad0b X-Va-CD: 0 X-Va-ID: 7062e416-b225-4313-a828-4ecedc7cb4fe X-V-A: X-V-T-CD: 19f0bc11d43124e8b595ff154d2eb713 X-V-E-CD: 656c33f328845d34dcd46237cb7e0482 X-V-R-CD: f86156c9a5f3cda77129bbd90c65ad0b X-V-CD: 0 X-V-ID: 9b26fd57-0eca-4fc2-8b0a-2715882b7162 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.517,18.0.874 definitions=2022-06-08_04:2022-06-07,2022-06-08 signatures=0 Received: from smtpclient.apple (unknown [17.235.37.147]) by rn-mailsvcp-mmp-lapp03.rno.apple.com (Oracle Communications Messaging Server 8.1.0.18.20220407 64bit (built Apr 7 2022)) with ESMTPSA id <0RD600AIDIG89S00@rn-mailsvcp-mmp-lapp03.rno.apple.com>; Wed, 08 Jun 2022 15:00:11 -0700 (PDT) From: "Andrew Fish" Message-id: <15C82A49-1C6E-4E4C-BA96-8DFA9141C8A6@apple.com> MIME-version: 1.0 (Mac OS X Mail 15.0 \(3693.20.0.1.32\)) Subject: Re: [edk2-devel] ovmf miscompiles with gcc-12 Date: Wed, 08 Jun 2022 15:00:07 -0700 In-reply-to: <4063953e-3c6a-8095-3023-9edd4469c35f@kernel.org> Cc: Gerd Hoffmann To: edk2-devel-groups-io , jirislaby@kernel.org, Mike Kinney References: <887c3f4f-c279-bd59-d92d-25922faae6dc@kernel.org> <20220607103120.zvgofggypzhdms5m@sirius.home.kraxel.org> <20220607110749.wyg7trlelht3cag5@sirius.home.kraxel.org> <4063953e-3c6a-8095-3023-9edd4469c35f@kernel.org> X-Mailer: Apple Mail (2.3693.20.0.1.32) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.517,18.0.874 definitions=2022-06-08_04:2022-06-07,2022-06-08 signatures=0 Content-type: multipart/alternative; boundary="Apple-Mail=_9B7B594E-EEE4-4F76-B25E-31AA77D82B2A" --Apple-Mail=_9B7B594E-EEE4-4F76-B25E-31AA77D82B2A Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Mike, This sounds like a conversation we had years ago? I think we concluded we n= eeded to write stuff in assembler and not depend on implementation choices = of the compiler with regard to registers usage? I want to say something bro= ke with Xcode clang. I think it might have been the Emulator code Temporary= RamMigration, so that was probably similar to this case? I=E2=80=99m a little unclear why this code is not using SwitchStack()[1]? I= thought one of the points of jumping to the new function was giving up of = old RBP usage? Also calling assembler and the indirect procedure call (SWIT= CH_STACK_ENTRY_POINT) force the complier to serialize to the ABI and thus g= reatly reduce the exposure to compiler optimization choices. Not sure if th= ere is something I=E2=80=99m missing and SwitchStack() can=E2=80=99t work? = Doh just realized to use SwitchStack the TemporaryRamMigration API would ne= ed to pass in the function to return too like SwitchStack. So that is a big= NO GO.=20 OK so this is like the Emulator case when the OS uses SYS V ABI and EFI doe= s not. OK I think the only pedantic way to fix this is to make the public A= PI call an assembler shim that calls the C code. Then the C code becomes.. EFI_STATUS EFIAPI TemporaryRamMigration ( IN CONST EFI_PEI_SERVICES **PeiServices, IN EFI_PHYSICAL_ADDRESS TemporaryMemoryBase, IN EFI_PHYSICAL_ADDRESS PermanentMemoryBase, IN UINTN CopySize IN OUT UINTN. *Rsp, IN OUT UINTN. *Rbp, OUT BOOLEAN *DebugTimerStatus ) The assembler shim can capture the actual RSP/RBP on entry and just fix the= m them on exit. The stub may need to call SaveAndSetDebugTimerInterrupt (De= bugTimerStatus); prior to exit. Then the existing SetJump/LongJump code becomes; *Rsp +=3D DebugAgentContext.StackMigrateOffset; *Rbp +=3D DebugAgentContext.StackMigrateOffset; return EFI_SUCCESS; The SaveAndSetDebugTimerInterrupt (OldStatus); call gets moved to the assem= bler code after the stack shift.=20 The tricky bit would seem to be the storage used for *Rsp, Rbp, and *DebugT= imerStauts. I guess worst case the C code could return DebugAgentContext.St= ackMigrateOffset and the assembly could =E2=80=9Cadjust=E2=80=9D. Then the = assembler code just hard codes a EFI_SUCCESS return; [1] https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseLib/X6= 4/SwitchStack.nasm#L35 /** Transfers control to a function starting with a new stack. Transfers control to the function specified by EntryPoint using the new stack specified by NewStack and passing in the parameters specified by Context1 and Context2. Context1 and Context2 are optional and may be NULL. The function EntryPoint must never return. This function supports a variable number of arguments following the NewStack parameter. These additional arguments are ignored on IA-32, x64, and EBC. IPF CPUs expect one additional parameter of type VOID * that specifies the new backing store pointer. If EntryPoint is NULL, then ASSERT(). If NewStack is NULL, then ASSERT(). @param EntryPoint A pointer to function to call with the new stack. @param Context1 A pointer to the context to pass into the EntryPoint function. @param Context2 A pointer to the context to pass into the EntryPoint function. @param NewStack A pointer to the new stack to use for the EntryPoint function. @param ... This variable argument list is ignored for IA32, x64,= and EBC. For IPF, this variable argument list is expected to c= ontain a single parameter of type VOID * that specifies the = new backing store pointer. **/ VOID EFIAPI SwitchStack ( IN SWITCH_STACK_ENTRY_POINT EntryPoint, IN VOID *Context1 OPTIONAL, IN VOID *Context2 OPTIONAL, IN VOID *NewStack, ... ) Thanks, Andrew Fish PS The Xcode clang always emits the frame pointer to enable stack walks. > On Jun 7, 2022, at 4:14 AM, Jiri Slaby wrote: >=20 > On 07. 06. 22, 13:07, Gerd Hoffmann wrote: >> On Tue, Jun 07, 2022 at 12:38:46PM +0200, Jiri Slaby wrote: >>> Hi, >>>=20 >>> On 07. 06. 22, 12:31, Gerd Hoffmann wrote: >>>>> The reason is TemporaryRamMigration() overwrites rbp unconditionally = -- it >>>>> adds an offset to rbp even if rbp is NOT used as a frame pointer >>>>=20 >>>>> Now, what is the right way to fix this? Do the SetJump/LongJump in as= sembly >>>>> and wrap it into push rbp/pop rbp? >>>>=20 >>>> push/pop rbp will break in case frame pointers are used, no? >>>=20 >>> Yes, see the downstream bug at: >>>=20 >>> https://bugzilla.suse.com/show_bug.cgi?id=3D1199597#c45 >>>=20 >>> and read further. >>>=20 >>>> I think essentially the code needs to know whenever frame pointers are >>>> used or not and then update (or not) rbp depending on that. Update >>>> compiler flags to explicitly set -f(no-)omit-frame-pointer, also add >>>> -D OMIT_FRAME_POINTER=3D1, the compile conditionally on OMIT_FRAME_POI= NTER? >>>=20 >>> Yes, the comment above mentions this too (cf. CONFIG_FRAME_POINTER in t= he >>> kernel). So see the downstream bugzilla for discussion. >> Ok. So what is the status here? Someone working on patches? >=20 > I don't know of anybody. I only tracked it down, reported and worked arou= nd locally by: > --- a/OvmfPkg/Sec/SecMain.c > +++ b/OvmfPkg/Sec/SecMain.c > @@ -928,7 +928,7 @@ SecStartupPhase2 ( > CpuDeadLoop (); > } >=20 > -EFI_STATUS > +EFI_STATUS __attribute__((optimize("-fno-omit-frame-pointer"))) > EFIAPI > TemporaryRamMigration ( > IN CONST EFI_PEI_SERVICES **PeiServices, >=20 >>> The upstream bugzilla needs an account which I don't have and cannot cr= eate >>> automatically. It needs manual intervention and I am too lazy to do so. >> It's just an email though: >> https://github.com/tianocore/tianocore.github.io/wiki/Reporting-Issues= >=20 > As I wrote earlier, there is a bug created by the openSUSE ovmf maintaine= r (Joey): > https://bugzilla.tianocore.org/show_bug.cgi?id=3D3934 >=20 > If there is any input needed from me, I might reconsider... So far, it's = stuck. >=20 > regards, > --=20 > js >=20 >=20 >=20 --Apple-Mail=_9B7B594E-EEE4-4F76-B25E-31AA77D82B2A Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Mike,

This sounds like a conversation we had year= s ago? I think we concluded we needed to write stuff in assembler and not d= epend on implementation choices of the compiler with regard to registers us= age? I want to say something broke with Xcode clang. I think it might have = been the Emulator code TemporaryRamMigration, so that was probably sim= ilar to this case?

I=E2=80=99m a little unclear why this code is not using SwitchStack()[1]?= I thought one of the points of jumping to the new function was giving up o= f old RBP usage? Also calling assembler and the indirect procedure call (SW= ITCH_STACK_ENTRY_POINT) force the complier to serialize to the ABI and thus= greatly reduce the exposure to compiler optimization choices. Not sure if = there is something I=E2=80=99m missing and SwitchStack() can=E2=80=99t work= ? Doh just realized to use SwitchStack the TemporaryRamMigration API w= ould need to pass in the function to return too like SwitchStack. So that i= s a big NO GO. 

OK so this is like the Emulator case when the OS uses SYS V ABI and E= FI does not. OK I think the only pedantic way to fix this is to make the pu= blic API call an assembler shim that calls the C code. Then the C code beco= mes..

EFI_STATUS
EFIAPI
Temporary= RamMigration (
  IN CONST EFI_PEI_SERVICES  = **PeiServices,
  IN EFI_PHYSICAL_ADDRESS   &= nbsp;TemporaryMemoryBase,
  IN EFI_PHYSICAL_ADDRE= SS    PermanentMemoryBase,
  IN UINTN &= nbsp;                 CopySize
  IN OUT UINTN.        *Rsp,
  IN OUT UINTN.        *Rbp,
      OUT BOOLEAN   *DebugTimerStatus<= /div>
  )

The assembler shim can capture the actual RSP/RBP on ent= ry and just fix them them on exit. The stub may need to call SaveAndSe= tDebugTimerInterrupt (DebugTimerStatus); prior to exit.

Then the existing SetJump/LongJump c= ode becomes;
*Rsp +=3D DebugAgent= Context.StackMigrateOffset;
*Rbp= +=3D DebugAgentContext.StackMigrateOffset;
return EFI_SUCCESS;

The SaveAndSetDebugTimerInterrupt (OldStatus); call gets moved t= o the assembler code after the stack shift. 

The tricky bit would seem to be the storag= e used for *Rsp, Rbp, and *DebugTimerStauts. I guess worst case the C code = could return DebugAgentContext.StackMigrateOffset and the assembly cou= ld =E2=80=9Cadjust=E2=80=9D. Then the assembler code just hard codes a = ;EFI_SUCCESS return;

[1] https://github.com= /tianocore/edk2/blob/master/MdePkg/Library/BaseLib/X64/SwitchStack.nasm#L35=

/**
  Transfers control to a function start= ing with a new stack.

  Transfers control to the function specified by EntryPoint usin= g the
  new stack specified by NewStack and passi= ng in the parameters specified
  by Context1 and = Context2.  Context1 and Context2 are optional and may
  be NULL.  The function EntryPoint must never return. &nbs= p;This function
  supports a variable number of a= rguments following the NewStack parameter.
  Thes= e additional arguments are ignored on IA-32, x64, and EBC.
  IPF CPUs expect one additional parameter of type VOID * that s= pecifies
  the new backing store pointer.

  If EntryPoint is = NULL, then ASSERT().
  If NewStack is NULL, then = ASSERT().

  = @param  EntryPoint  A pointer to function to call with the new st= ack.
  @param  Context1    A point= er to the context to pass into the EntryPoint
  &= nbsp;                   functi= on.
  @param  Context2    A pointe= r to the context to pass into the EntryPoint
  &n= bsp;                   functio= n.
  @param  NewStack    A pointer= to the new stack to use for the EntryPoint
  &nb= sp;                   function= .
  @param  ...         = This variable argument list is ignored for IA32, x64, and EBC.
                  &nb= sp;   For IPF, this variable argument list is expected to contain
                &n= bsp;     a single parameter of type VOID * that specifies the new= backing
            &nb= sp;         store pointer.


**/
VOID
EFIAPI
Swit= chStack (
  IN      SWITCH_STACK_E= NTRY_POINT  EntryPoint,
  IN     &= nbsp;VOID                   &n= bsp;  *Context1   OPTIONAL,
  IN  =    VOID                 =      *Context2   OPTIONAL,
  = IN      VOID              = ;        *NewStack,
  ...
  )

=

Thanks,

Andrew Fish

PS The Xcode clang always emits the = frame pointer to enable stack walks.

On Jun 7, 2022, at 4:14 AM= , Jiri Slaby <jirisla= by@kernel.org> wrote:

<= div class=3D"">On 07. 06. 22, 13:= 07, Gerd Hoffmann wrote:
On Tue, Jun 07, 2022 at 12:38:46PM +0200, Jiri Slaby wrote:<= br class=3D"">
Hi,

On 07. 06. 22, 12:31, Gerd Hoffmann wrote:
The reason= is TemporaryRamMigration() overwrites rbp unconditionally -- it
adds an offset to rbp even if rbp is NOT used as a frame pointer

= Now, what is the right way to fix this? Do the SetJump/LongJump in assembly=
and wrap it into push rbp/pop rbp?

push/pop rbp will break in case frame pointers are used, n= o?

Yes, see the downstream bug at= :

https://bugzilla.suse.com/show_bug.cgi?i= d=3D1199597#c45

and read further.

I think essential= ly the code needs to know whenever frame pointers are
used or= not and then update (or not) rbp depending on that.  Update
compiler flags to explicitly set -f(no-)omit-frame-pointer, also add<= br class=3D"">-D OMIT_FRAME_POINTER=3D1, the compile conditionally on OMIT_= FRAME_POINTER?

Yes, the comment a= bove mentions this too (cf. CONFIG_FRAME_POINTER in the
kerne= l). So see the downstream bugzilla for discussion.
Ok.  So what is the status here?  Someone working on patches?<= br class=3D"">

I = don't know of anybody. I only tracked it down, reported and worked around l= ocally by:
--- a/OvmfPk= g/Sec/SecMain.c
+++ b/O= vmfPkg/Sec/SecMain.c
@@= -928,7 +928,7 @@ SecStartupPhase2 (
  CpuDeadLoop ();
}

-EFI_STATUS
+EFI_STATUS __attribute__((optimize("-fno-omit-fr= ame-pointer")))
EFIAPI<= /span>
TemporaryRamMigration (=
  IN CONST E= FI_PEI_SERVICES  **PeiServices,

The upstream bugzilla needs an account which I = don't have and cannot create
automatically. It needs manual i= ntervention and I am too lazy to do so.
It's jus= t an email though:
  https://git= hub.com/tianocore/tianocore.github.io/wiki/Reporting-Issues

As I wrote ea= rlier, there is a bug created by the openSUSE ovmf maintainer (Joey):
https://bugzilla.tianocore.org/show_bug.cgi?id=3D3934

If t= here is any input needed from me, I might reconsider... So far, it's stuck.=

regards,
-- 
js



--Apple-Mail=_9B7B594E-EEE4-4F76-B25E-31AA77D82B2A--