Re: [PATCH v3 1/1] OvmfPkg/SecMain: Fix stack switching to permanent memory

[Jordan Justen <jordan.l.justen@intel.com>]

On 2017-09-05 20:13:19, Song, Ge wrote:
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ge Song <ge.song@hxt-semitech.com>
> 

Pushed as 89796c69d9fdaa9bd13d37b6b1687df5e0104ed5. Thanks for
noticing that bug and fixing it!

I also hope your IT might be to help you configure git send-email to
make future contributions from your hxt-semitech.com email a bit
easier.

Thanks again!

-Jordan

> 
> -----Original Message-----
> From: songgebird@gmail.com [mailto:songgebird@gmail.com]
> Sent: 2017年9月6日 11:12
> To: edk2-devel@lists.01.org
> Cc: Jordan Justen <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Song, Ge <ge.song@hxt-semitech.com>
> Subject: [PATCH v3 1/1] OvmfPkg/SecMain: Fix stack switching to permanent memory
> 
> From: Ge Song <ge.song@hxt-semitech.com>
> 
> In earlier PEI stage, temporary memory at PcdOvmfSecPeiTempRamBase is
> employed as stack and heap. We move them to the new room and do some
> relocation fixup when permanent memory becomes available.
> TemporaryRamMigration() is responsible for switching the stack.
> 
> Before entering TemporaryRamMigration(), Ebp/Rbp is populated with the
> content of Esp/Rsp and used as frame pointer.
> 
> After the execution of SetJump/LongJump, stack migrates to new position
> while the context keeps unchanged.
> 
> But when TemporaryRamMigration() exits, Esp/Rsp is filled with
> the content of Ebp/Rbp to destroy this stack frame.
> 
> The result is, stack switches back to previous temporary memory.
> 
> When permanent memory becomes available, modules that have registered
> themselves for shadowing will be scheduled to execute. Some of them
> need to consume more memory(heap/stack). Contrast to temporary stack,
> permanent stack possesses larger space.
> 
> The potential risk is overflowing the stack if stack staying in
> temporary memory. When it happens, system may crash during S3 resume.
> 
> More detailed information:
> > (gdb) disassemble /r
> > Dump of assembler code for function TemporaryRamMigration:
> >   0x00000000fffcd29c <+0>:55push   %rbp
> >   0x00000000fffcd29d <+1>:48 89 e5mov    %rsp,%rbp
> >   0x00000000fffcd2a0 <+4>:48 81 ec 70 01 00 00sub
> > $0x170,%rsp
> >    ...
> >    ...
> >    0x00000000fffcd425 <+393>:e8 80 10 00 00callq  0xfffce4aa
> > <SaveAndSetDebugTimerInterrupt>
> > => 0x00000000fffcd42a <+398>:b8 00 00 00 00mov    $0x0,%eax
> >    0x00000000fffcd42f <+403>:c9leaveq
> >    0x00000000fffcd430 <+404>:c3retq
> > End of assembler dump.
> 
> See the description of leave(opcode: c9), from
> Intel 64 and IA-32 Architectures Software Developer's Manual, Volume 2A
> 
> "Releases the stack frame set up by an earlier ENTER instruction. The
> LEAVE instruction copies the frame pointer (in the EBP register) into
> the stack pointer register (ESP), which releases the stack space
> allocated to the stack frame. The old frame pointer (the frame pointer
> for the calling procedure that was saved by the ENTER instruction) is
> then popped from the stack into the EBP register, restoring the calling
> procedure’s stack frame."
> 
> To solve this, update Ebp/Rbp too when Esp/Rsp is updated
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ge Song <ge.song@hxt-semitech.com>
> Tested-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  OvmfPkg/Sec/SecMain.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
> index e1993ec347b5..f7fec3d8c03b 100644
> --- a/OvmfPkg/Sec/SecMain.c
> +++ b/OvmfPkg/Sec/SecMain.c
> @@ -931,9 +931,11 @@ TemporaryRamMigration (
>    if (SetJump (&JumpBuffer) == 0) {
>  #if defined (MDE_CPU_IA32)
>      JumpBuffer.Esp = JumpBuffer.Esp + DebugAgentContext.StackMigrateOffset;
> +    JumpBuffer.Ebp = JumpBuffer.Ebp + DebugAgentContext.StackMigrateOffset;
>  #endif
>  #if defined (MDE_CPU_X64)
>      JumpBuffer.Rsp = JumpBuffer.Rsp + DebugAgentContext.StackMigrateOffset;
> +    JumpBuffer.Rbp = JumpBuffer.Rbp + DebugAgentContext.StackMigrateOffset;
>  #endif
>      LongJump (&JumpBuffer, (UINTN)-1);
>    }
> --
> 2.11.0
> 
> 
> 
> 
> This email is intended only for the named addressee. It may contain information that is confidential/private, legally privileged, or copyright-protected, and you should handle it accordingly. If you are not the intended recipient, you do not have legal rights to retain, copy, or distribute this email or its contents, and should promptly delete the email and all electronic copies in your system; do not retain copies in any media. If you have received this email in error, please notify the sender promptly. Thank you.
> 
>