From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.151; helo=mga17.intel.com; envelope-from=star.zeng@intel.com; receiver=edk2-devel@lists.01.org Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) (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 65400202E53D9 for ; Wed, 27 Jun 2018 23:16:50 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 27 Jun 2018 23:16:49 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,282,1526367600"; d="scan'208";a="241297525" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by fmsmga006.fm.intel.com with ESMTP; 27 Jun 2018 23:16:33 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 27 Jun 2018 23:16:32 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.223]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.51]) with mapi id 14.03.0319.002; Thu, 28 Jun 2018 14:16:31 +0800 From: "Zeng, Star" To: Laszlo Ersek , Brijesh Singh , "edk2-devel@lists.01.org" CC: Tom Lendacky , "Dong, Eric" , "Justen, Jordan L" , "Zeng, Star" Thread-Topic: [edk2] [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime variable access when SEV is enabled Thread-Index: AQHUDYZkbjwQo2y44E66eDtVlTyMLqRziq6AgAGnluA= Date: Thu, 28 Jun 2018 06:16:31 +0000 Message-ID: <0C09AFA07DD0434D9E2A0C6AEB0483103BB5E06A@shsmsx102.ccr.corp.intel.com> References: <1530042365-9979-1-git-send-email-brijesh.singh@amd.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime variable access when SEV is enabled X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 28 Jun 2018 06:16:51 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 1) My understanding is Variable Driver is managing the variable region in f= lash although the flash read/write/erase operations are done in flash drive= r. Current Variable Driver needs the address (to variable region) be conver= ted to virtual address for runtime, and it does not assume flash driver to = set the runtime attribute for variable region, but do it by itself. 2) I agree this approach. If the runtime attribute has been set by flash dr= iver, Variable Driver has no need to do that. Thanks, Star -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Lasz= lo Ersek Sent: Wednesday, June 27, 2018 8:54 PM To: Brijesh Singh ; edk2-devel@lists.01.org Cc: Tom Lendacky ; Dong, Eric ; Zeng, Star ; Justen, Jordan L Subject: Re: [edk2] [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime variable= access when SEV is enabled On 06/26/18 21:46, Brijesh Singh wrote: > Problem statement: > ------------------ > Fedora-28 contains 4.16 kernel -- which has all the required support=20 > to run as an SEV guest. When the installer is launched from SEV guest=20 > then it fails to install the bootloader. The installer was failing to=20 > update the 'BootOrder' UEFI runtime variable. >=20 > Root Cause Analysis > -------------------- > Since QemuFlash storage memory is accessed by both guest and=20 > hypervisor hence we need to map this memory range as unencrypted.=20 > AmdSevDxe maps the range as "unencrypted" but later=20 > FtwNotificationEvent() in=20 > MdeModule/Universal/Variable/RuntimeDxe/VariableDxe.c resets the mapping = and the memory region gets remapped as "encrypted". Is this a new issue, or has it always been there, and we just failed to not= ice it? BTW, I don't understand why FtwNotificationEvent() in "MdeModulePkg/Univers= al/Variable/RuntimeDxe/VariableDxe.c" has to mark the flash range as EFI_ME= MORY_RUNTIME. I thought that this action belonged in the flash driver itsel= f, and we do that already in MarkMemoryRangeForRuntimeAccess(), in file "Ov= mfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c". The variable driver does not own the pflash chip (the pflash driver owns it= ), so I believe the variable driver shouldn't mess with the mapping attribu= tes. Here's a suggestion -- Star, Eric, can you please comment? In the FtwNotificationEvent() function, after we get the memory descriptor for the= pflash range, first check whether EFI_MEMORY_RUNTIME is already set. If it is, don't do anything; if it isn't, add the attribute. This should cause no observable change on any non-SEV platform, and it shou= ld remove the gDS->SetMemorySpaceAttributes() call for OVMF (where it break= s things for SEV). > After that, any access > to the flash will end up going through the encryption engine. I did=20 > try hacking EDK2 to restore the C-bit (I continue to be annoyed that the memory encryption bit is not exposed in = the GCD memory space attributes explicitly.) > but that was not sufficient because UEFI runtime services are mapped=20 > as "encrypted" in OS page table What do you mean here? Runtime services *code* or runtime services *data*? = Code must obviously be remain encrypted (otherwise we cannot execute it in = SEV). Runtime Services Data should also be mapped as encrypted (it is norma= l RAM that is not used for guest<->hypervisor exchange). > hence we end up accessing the flash as encrypted when OS requests to upda= te the variables. I don't understand the "hence" here; I don't see how the implication follow= s. runtime services code and data should be encrypted. Runtime MMIO should = be un-encrypted. Ohh, wait, in MarkMemoryRangeForRuntimeAccess(), we use "EfiGcdMemoryTypeSy= stemMemory". I don't have a clue why that is a good idea. That should have = been EfiGcdMemoryTypeMemoryMappedIo. ... Anyway, I think first we should go with the "check EFI_MEMORY_RUNTIME a= ttribute before setting it", in FtwNotificationEvent(). >=20 > A possible solution > --------------------- > To solve the issue, after QemuFlash is probed, I allocate an encrypted=20 > buffer and initialize this buffer with the contents from the flash memory= . > When SEV is enabled, we use newly allocated encrypted buffer in > FwInstance->FvBase instead of the original flash region. The idea is=20 > FwInstance->if > caller grabs the FwInstance->FvBase pointer and tries to access the=20 > FvVolumeHeader then it should get the data from the encrypted buffer. > But if caller wants read/writes to/from the flash device then we=20 > internally use the original "unencrypted" flash region to access the data= . No, this is neither safe, nor a desirable design. Safety: all accesses (via both pointers and FVB protocol members) that high= er-level drives *think* go to the pflash chip must *actually* go to the pfl= ash chip. Design: it had taken us years to get rid of various memory-emulated fake va= riable stores. They *all* suck in one way or another, with various obscure = UEFI spec incompatibilities and corner cases. A strictly pflash-based varst= ore is not what we should compromise on. > With this > patch, I have verified that OS is able to update the runtime variable=20 > and > FC-28 installer is successfully able to complete the installation process= . >=20 > If you all agree with approach then I can rework any feedbacks and=20 > remove the rfc tag from the patch. If you have better suggestions then=20 > I am open to explore those as well. I'd like to understand the following: (1) why does FtwNotificationEvent() set the EFI_MEMORY_RUNTIME attribute it= self, for the pflash range? -- in my opinion, that belongs in the flash dri= ver. (2) Whether Star and Eric agree with setting the EFI_MEMORY_RUNTIME attribu= te in FtwNotificationEvent() only if the attribute is not already present. (3) The implication that you describe, between runtime services/code being = mapped encrypted, and restoring the C-bit failing. (4) Whether we should modify MarkMemoryRangeForRuntimeAccess() to install t= he range into GCD as MMIO. (I feel *very* uncomfortable about this, however= ; the current code has existed as-is for years, and regressions look very r= isky.) My strong preference would be a patch for (2). Thanks, Laszlo > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Brijesh Singh > --- > .../FvbServicesRuntimeDxe.inf | 1 + > .../FwBlockService.c | 37 ++++++++++++++++= +++--- > 2 files changed, 34 insertions(+), 4 deletions(-) >=20 > diff --git=20 > a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf=20 > b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf > index d7b4ec06c4e6..6bb5c2093790 100644 > --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf > +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf > @@ -54,6 +54,7 @@ [LibraryClasses] > DevicePathLib > DxeServicesTableLib > MemoryAllocationLib > + MemEncryptSevLib > PcdLib > UefiBootServicesTableLib > UefiDriverEntryPoint > diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c=20 > b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c > index 558b395dff4a..e82b4ff70961 100644 > --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c > +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c > @@ -36,6 +36,7 @@ > #include #include=20 > #include=20 > > +#include > =20 > #include "FwBlockService.h" > #include "QemuFlash.h" > @@ -966,6 +967,7 @@ FvbInitialize ( > UINTN Length; > UINTN NumOfBlocks; > RETURN_STATUS PcdStatus; > + EFI_PHYSICAL_ADDRESS CryptedAddress; > =20 > if (EFI_ERROR (QemuFlashInitialize ())) { > // > @@ -986,6 +988,24 @@ FvbInitialize ( > BaseAddress =3D (UINTN) PcdGet32 (PcdOvmfFdBaseAddress); > Length =3D PcdGet32 (PcdOvmfFirmwareFdSize); > =20 > + // > + // When SEV is enabled, allocate a encrypted buffer which will=20 > + contain a // encrypted copy of the Flash image. > + // > + if (MemEncryptSevIsEnabled ()) { > + Status =3D gBS->AllocatePages ( > + AllocateAnyPages, > + EfiRuntimeServicesData, > + EFI_SIZE_TO_PAGES(PcdGet32 (PcdOvmfFirmwareFdSize)), > + &CryptedAddress > + ); > + ASSERT_EFI_ERROR (Status); > + > + CopyMem((VOID *)CryptedAddress, (VOID *)BaseAddress, Length); > + > + BaseAddress =3D CryptedAddress; > + } > + > Status =3D InitializeVariableFvHeader (); > if (EFI_ERROR (Status)) { > DEBUG ((EFI_D_INFO, > @@ -1091,24 +1111,33 @@ FvbInitialize ( > // > InstallProtocolInterfaces (FvbDevice); > =20 > - MarkMemoryRangeForRuntimeAccess (BaseAddress, Length); > + MarkMemoryRangeForRuntimeAccess ( > + (UINTN) PcdGet32 (PcdOvmfFdBaseAddress), > + Length > + ); > =20 > // > // Set several PCD values to point to flash > // > PcdStatus =3D PcdSet64S ( > PcdFlashNvStorageVariableBase64, > - (UINTN) PcdGet32 (PcdOvmfFlashNvStorageVariableBase) > + BaseAddress > ); > ASSERT_RETURN_ERROR (PcdStatus); > PcdStatus =3D PcdSet32S ( > PcdFlashNvStorageFtwWorkingBase, > - PcdGet32 (PcdOvmfFlashNvStorageFtwWorkingBase) > + BaseAddress + > + PcdGet32(PcdFlashNvStorageVariableSize) + > + PcdGet32(PcdOvmfFlashNvStorageEventLogSize) > ); > + > ASSERT_RETURN_ERROR (PcdStatus); > PcdStatus =3D PcdSet32S ( > PcdFlashNvStorageFtwSpareBase, > - PcdGet32 (PcdOvmfFlashNvStorageFtwSpareBase) > + BaseAddress + > + PcdGet32(PcdFlashNvStorageVariableSize) + > + PcdGet32(PcdOvmfFlashNvStorageEventLogSize) + > + PcdGet32(PcdFlashNvStorageFtwWorkingSize) > ); > ASSERT_RETURN_ERROR (PcdStatus); > =20 >=20 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel