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 898F7220F33E9 for ; Thu, 22 Feb 2018 03:14:14 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1F6514084FEC; Thu, 22 Feb 2018 11:20:14 +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 832AA2024CA9; Thu, 22 Feb 2018 11:20:10 +0000 (UTC) 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> From: Laszlo Ersek Message-ID: <294c83a2-bda9-392a-aceb-19f170946d57@redhat.com> Date: Thu, 22 Feb 2018 12:20:09 +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: <20180221165212.6643-2-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Thu, 22 Feb 2018 11:20:14 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Thu, 22 Feb 2018 11:20:14 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.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:14:15 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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? - 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