From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 1946621EB88F5 for ; Sun, 3 Sep 2017 13:46:34 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id EBC18356E3; Sun, 3 Sep 2017 20:49:18 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com EBC18356E3 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-188.rdu2.redhat.com [10.10.120.188]) by smtp.corp.redhat.com (Postfix) with ESMTP id A96CC17A8E; Sun, 3 Sep 2017 20:49:16 +0000 (UTC) To: Ge Song , edk2-devel@lists.01.org Cc: Jordan Justen , Ard Biesheuvel References: <20170903021214.11516-1-ge.song@hxt-semitech.com> <20170903021214.11516-2-ge.song@hxt-semitech.com> From: Laszlo Ersek Message-ID: <8cf35d7b-9c1c-e1a8-5896-ca709e57fef6@redhat.com> Date: Sun, 3 Sep 2017 22:49:15 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20170903021214.11516-2-ge.song@hxt-semitech.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Sun, 03 Sep 2017 20:49:20 +0000 (UTC) Subject: Re: [PATCH v1 1/1] OvmfPkg/SecMain: Fix stack switching to permanent memory fail X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 03 Sep 2017 20:46:34 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 09/03/17 04:12, Ge Song wrote: > In earlier PEI stage, temporary memory(Cache As Ram) is employed as stack Please remove the "Cache As Ram" reference; it doesn't apply to OVMF. I suggest "temporary memory at PcdOvmfSecPeiTempRamBase" > 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. > > In the begining of the TemporaryRamMigration(), I suggest rewording this as "before entering TemporaryRamMigration()" -- when I read the above, I went looking for Ebp/Rbp manipulation within TemporaryRamMigration(). > Ebp/Rbp is populated with content of Esp/Rsp and used as frame pointer, > after the execution of SetJump/LongJump, stack migrates to new position Please start a new sentence with "after the execution of ...". > 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 momery. s/momery/memory/ But, more importantly, what are the practical consequences of this bug? Does it mean that the temporary SEC/PEI stack is used until the end of PEI, even after permanent RAM is installed, during both normal boot and S3 resume? If that's the case, I wonder how it was caught. If it was caught during S3 resume, then the only symptom could have been stack overflow. Because during normal boot, both temporary and permanent PEI RAM are reserved, so even if we stay on the temporary stack during S3, if we don't overflow it, we cannot corrupt data: > /** > Publish system RAM and reserve memory regions > > **/ > VOID > InitializeRamRegions ( > VOID > ) > { > if (!mXen) { > QemuInitializeRam (); > } else { > XenPublishRamRegions (); > } > > if (mS3Supported && mBootMode != BOOT_ON_S3_RESUME) { > // > // This is the memory range that will be used for PEI on S3 resume > // > BuildMemoryAllocationHob ( > mS3AcpiReservedMemoryBase, > mS3AcpiReservedMemorySize, > EfiACPIMemoryNVS > ); > > // > // Cover the initial RAM area used as stack and temporary PEI heap. > // > // This is reserved as ACPI NVS so it can be used on S3 resume. > // > BuildMemoryAllocationHob ( > PcdGet32 (PcdOvmfSecPeiTempRamBase), > PcdGet32 (PcdOvmfSecPeiTempRamSize), > EfiACPIMemoryNVS > ); With stack overflow during S3 resume, we may have corrupted OS data residing very low. However, if the bug was caught during normal boot, then I don't know what the symptom may have been. The permanent PEI RAM for normal boot is pretty close to the top of low RAM (and the default RAM size for QEMU is 128MB). The temporary stack is very low however ("Temp Stack : BaseAddress=0x814000 Length=0x4000"). So even if we overflow the temporary stack (i.e. go below the address 0x814000) during normal boot, I don't think anything allocated from the permanent PEI RAM could be corrupted by that. I'll do some testing in the next few days. The analysis and the patch look great (and impressive!) to me. I'd like the commit message to be cleaned up a bit -- please see my notes above, and please describe: - practical consequences of the bug, - how the bug was caught. I have one more (superficial) comment below: > More detailed information: >> TemporaryRamMigration (PeiServices=0x817790, >> TemporaryMemoryBase=8454144, PermanentMemoryBase=469016576, >> CopySize=32768) >> at /home/bird/src/edk2/OvmfPkg/Sec/SecMain.c:938 >> 938 LongJump (&JumpBuffer, (UINTN)-1); >> (gdb) info registers >> rax 0x1bf4d3b0 469029808 >> rbx 0x810248 8454728 >> rcx 0x8173e8 8483816 >> rdx 0x8173b0 8483760 >> rsi 0x1bf4a000 469016576 >> rdi 0x8000 32768 >> rbp 0x817520 0x817520 >> rsp 0x8173b0 0x8173b0 >> r8 0x0 0 >> ... >> rip 0xfffcd409 0xfffcd409 >> eflags 0x2 [ ] >> cs 0x18 24 >> ss 0x8 8 >> ... > > After execution of LongJump: >> 943 return EFI_SUCCESS; >> (gdb) info registers >> rax 0x0 0 >> rbx 0x810248 8454728 >> rcx 0x0 0 >> rdx 0xffffffffffffffff -1 >> rsi 0x1bf4a000 469016576 >> rdi 0x8000 32768 >> rbp 0x817520 0x817520 >> rsp 0x1bf4d3b0 0x1bf4d3b0 >> r8 0x0 0 >> ... >> rip 0xfffcd42a 0xfffcd42a >> eflags 0x86 [ PF SF ] >> cs 0x18 24 >> ss 0x8 8 >> ... > > We can find rsp has changed to new permanent memory > > When leaving TemporaryRamMigration(), the stack swithes back to previous > temporary memory: >> (gdb) finish >> Run till exit from #0 TemporaryRamMigration (PeiServices=0x817790, >> TemporaryMemoryBase=8454144, PermanentMemoryBase=469016576, >> CopySize=32768) >> at /home/bird/src/edk2/OvmfPkg/Sec/SecMain.c:943 >> PeiCheckAndSwitchStack (SecCoreData=0x1bf4df78, Private=0x1bf4d788) at >> /home/bird/src/edk2/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c:806 >> 806 PeiCore (SecCoreData, NULL, Private); >> Value returned is $1 = 0 >> (gdb) info registers >> rax 0x0 0 >> rbx 0x810248 8454728 >> rcx 0x0 0 >> rdx 0xffffffffffffffff -1 >> rsi 0x1bf4a000 469016576 >> rdi 0x8000 32768 >> rbp 0x817630 0x817630 >> rsp 0x817530 0x817530 >> r8 0x0 0 >> ... >> rip 0x828135 0x828135 >> eflags 0x86 [ PF SF ] >> cs 0x18 24 >> ss 0x8 8 >> ... > >> (gdb) disassemble /r >> Dump of assembler code for function TemporaryRamMigration: >> 0x00000000fffcd29c <+0>: 55 push %rbp >> 0x00000000fffcd29d <+1>: 48 89 e5 mov %rsp,%rbp >> 0x00000000fffcd2a0 <+4>: 48 81 ec 70 01 00 00 sub >> $0x170,%rsp >> ... >> ... >> 0x00000000fffcd425 <+393>: e8 80 10 00 00 callq 0xfffce4aa >> >> => 0x00000000fffcd42a <+398>: b8 00 00 00 00 mov $0x0,%eax >> 0x00000000fffcd42f <+403>: c9 leaveq >> 0x00000000fffcd430 <+404>: c3 retq >> End of assembler dump. > > See the description of leave(opcode: c9), from > Intel® 64 and IA-32 Architectures Software Developers Manual, Volume 2A I prefer keeping commit messages ASCII-clean. Thanks Laszlo > "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 > procedures stack frame." > > To solve this, update Ebp/Rbp too when Esp/Rsp is updated > > Cc: Jordan Justen > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ge Song > --- > 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); > } >