From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (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 409E22034D8C5 for ; Thu, 22 Feb 2018 03:16:00 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2446D40FB639; Thu, 22 Feb 2018 11:22:00 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-138.rdu2.redhat.com [10.10.120.138]) by smtp.corp.redhat.com (Postfix) with ESMTP id DC8AFAE7A5; Thu, 22 Feb 2018 11:21:58 +0000 (UTC) From: Laszlo Ersek To: Brijesh Singh , edk2-devel@lists.01.org Cc: Tom Lendacky , Jordan Justen , Ard Biesheuvel , Paolo Bonzini , Michael Kinney References: <20180221165212.6643-1-brijesh.singh@amd.com> <20180221165212.6643-2-brijesh.singh@amd.com> <294c83a2-bda9-392a-aceb-19f170946d57@redhat.com> Message-ID: Date: Thu, 22 Feb 2018 12:21:58 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <294c83a2-bda9-392a-aceb-19f170946d57@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Thu, 22 Feb 2018 11:22:00 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Thu, 22 Feb 2018 11:22:00 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH 1/2] OvmfPkg/AmdSevDxe: Clear the C-bit from SMM Saved State X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 22 Feb 2018 11:16:00 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 02/22/18 12:20, Laszlo Ersek wrote: > Hi Brijesh! > > (adding Paolo and Mike; more comments below) > > On 02/21/18 17:52, Brijesh Singh wrote: >> When OVMF is built with SMM, SMMSaved State area (SMM_DEFAULT_SMBASE + >> SMRAM_SAVE_STATE_MAP_OFFSET) contains data which need to be accessed by >> both guest and hypervisor. Since the data need to be accessed by both >> hence we must map the SMMSaved State area as unencrypted (i.e C-bit >> cleared). >> >> Cc: Jordan Justen >> Cc: Laszlo Ersek >> Cc: Ard Biesheuvel >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Brijesh Singh >> --- >> OvmfPkg/AmdSevDxe/AmdSevDxe.inf | 4 ++++ >> OvmfPkg/AmdSevDxe/AmdSevDxe.c | 19 +++++++++++++++++++ >> 2 files changed, 23 insertions(+) >> >> diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf >> index 41635a57a454..162ed98a2fbe 100644 >> --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf >> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf >> @@ -29,6 +29,7 @@ [Packages] >> MdePkg/MdePkg.dec >> MdeModulePkg/MdeModulePkg.dec >> OvmfPkg/OvmfPkg.dec >> + UefiCpuPkg/UefiCpuPkg.dec >> >> [LibraryClasses] >> BaseLib >> @@ -41,3 +42,6 @@ [LibraryClasses] >> >> [Depex] >> TRUE >> + >> +[FeaturePcd] >> + gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire >> diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c >> index e472096320ea..5ec727456526 100644 >> --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c >> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c >> @@ -25,6 +25,8 @@ >> #include >> #include >> #include >> +#include >> +#include >> >> EFI_STATUS >> EFIAPI >> @@ -71,5 +73,22 @@ AmdSevDxeEntryPoint ( >> FreePool (AllDescMap); >> } >> >> + // >> + // When SMM is enabled, clear the C-bit from SMM Saved State Area >> + // >> + if (FeaturePcdGet (PcdSmmSmramRequire)) { >> + EFI_PHYSICAL_ADDRESS SmmSavedStateAreaAddress; >> + >> + SmmSavedStateAreaAddress = SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET; >> + >> + Status = MemEncryptSevClearPageEncMask ( >> + 0, >> + SmmSavedStateAreaAddress, >> + EFI_SIZE_TO_PAGES (sizeof(QEMU_SMRAM_SAVE_STATE_MAP)), >> + FALSE >> + ); >> + ASSERT_EFI_ERROR (Status); >> + } >> + >> return EFI_SUCCESS; >> } >> > > First, this SMBASE address is only correct before SMBASE relocation. > > - What guarantees that AmdSevDxe is dispatched, and the new code is > executed, before PiSmmCpuDxeSmm performs the SMBASE relocation? Apologies, I forgot (and missed) that AmdSevDxe is listed in APRIORI DXE. So consider this part answered. Thanks! Laszlo > > - Where/when do we clear encryption for the state map that is used after > SMBASE relocation? If we don't do that at all, then why do things > work? (I see a whole lot of "mAddressEncMask" in PiSmmCpuDxeSmm; some > help would be appreciated.) > > Second, after SMBASE relocation, when/where do we restore the C bit on > the original (default) SMBASE? I think we should do that, otherwise > we'll have an info leak there. > > Third, why is it OK to pass Flush=FALSE? The documentation says, "mostly > TRUE except MMIO addresses". The default SMBASE points into memory, not > MMIO; there could be normal data there. PiSmmCpuDxeSmm backs up the area > before SMBASE relocation, and restores it after. See SmmRelocateBases() > in "UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c": > > // > // Backup original contents at address 0x38000 > // > CopyMem (BakBuf, U8Ptr, sizeof (BakBuf)); > CopyMem (&BakBuf2, CpuStatePtr, sizeof (BakBuf2)); > > I wonder if we should introduce new SmmCpuFeaturesLib APIs, for managing > memory encryption settings around SMBASE relocation. > - PiSmmCpuDxeSemm could call these APIs in "strategic places". > - The SmmCpuFeaturesLib instances in UefiCpuPkg and QuarkSocPkg would do > nothing. > - The instace under OvmfPkg would handle the C-bit, dependent on SEV > presence. > > The lib class already has a function called > SmmCpuFeaturesSmmRelocationComplete(); we might be able to use that (too). > > Thanks, > Laszlo >