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.115; helo=mga14.intel.com; envelope-from=star.zeng@intel.com; receiver=edk2-devel@lists.01.org Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) (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 C3FA4202E53E8 for ; Thu, 28 Jun 2018 19:37:53 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 28 Jun 2018 19:37:53 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,285,1526367600"; d="scan'208";a="50760971" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga007.fm.intel.com with ESMTP; 28 Jun 2018 19:37:49 -0700 Received: from fmsmsx122.amr.corp.intel.com (10.18.125.37) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 28 Jun 2018 19:37:49 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by fmsmsx122.amr.corp.intel.com (10.18.125.37) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 28 Jun 2018 19:37:48 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.223]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.87]) with mapi id 14.03.0319.002; Fri, 29 Jun 2018 10:37:46 +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: AQHUDYZkbjwQo2y44E66eDtVlTyMLqRziq6AgAGnluD///BNAIABZgNA Date: Fri, 29 Jun 2018 02:37:46 +0000 Message-ID: <0C09AFA07DD0434D9E2A0C6AEB0483103BB5E5A7@shsmsx102.ccr.corp.intel.com> References: <1530042365-9979-1-git-send-email-brijesh.singh@amd.com> <0C09AFA07DD0434D9E2A0C6AEB0483103BB5E06A@shsmsx102.ccr.corp.intel.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: Fri, 29 Jun 2018 02:37:54 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable My understading: Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbSmmDxe.inf is DXE counterpart of Vlv2Tbl= tDevicePkg/FvbRuntimeDxe/FvbSmm.inf, to produce EfiFirmwareVolumeBlock2?Pro= tocolGuid based on gEfiSmmFirmwareVolumeBlockProtocolGuid by SMM comm. Platform can choose FvbSmmDxe + FvbSmm, or FvbRuntimeDxe + FvbSmm. Thanks, Star -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Lasz= lo Ersek Sent: Thursday, June 28, 2018 9:14 PM To: Zeng, Star ; Brijesh Singh = ; edk2-devel@lists.01.org Cc: Tom Lendacky ; Dong, Eric ; Justen, Jordan L Subject: Re: [edk2] [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime variable= access when SEV is enabled On 06/28/18 08:16, Zeng, Star wrote: > 1) My understanding is Variable Driver is managing the variable region in= flash although the flash read/write/erase operations are done in flash dri= ver. Current Variable Driver needs the address (to variable region) be conv= erted to virtual address for runtime, and it does not assume flash driver t= o set the runtime attribute for variable region, but do it by itself. >=20 > 2) I agree this approach. If the runtime attribute has been set by flash = driver, Variable Driver has no need to do that. Great, thank you! Yesterday, after I sent my email(s), I got concerned for a moment, because = it wasn't clear to me what *runtime driver* actually owned the flash range. However, the FVB protocol itself must be provided by a runtime driver on UE= FI/PI platforms where the variable write service implementation -- indirect= ly or directly -- depends on FVB, for flash writes. Indeed in edk2, we have= at least the following FVB drivers: $ git grep -l -E \ -e 'EfiFirmwareVolumeBlock2?ProtocolGuid' --and -e 'PROD' EmulatorPkg/FvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf Nt32Pkg/FvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbRuntimeDxe.inf Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbSmmDxe.inf All of them are DXE_RUNTIME_DRIVER modules [*] and they all call EfiConvert= Pointer(). [*] "Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbSmmDxe.inf" is marked as a DXE_DRIV= ER (not runtime), and indeed it doesn't call EfiConvertPointer(). I don't k= now what this driver is good for; the INF file is not included in any DSC f= ile in edk2. I guess it can be used for flash access on an SMM platform as = long as you never boot an OS / never call ExitBootServices(). In that case = however, runtime aspects have no meaning at all. Thanks! Laszlo >=20 >=20 > Thanks, > Star > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of=20 > Laszlo Ersek > Sent: Wednesday, June 27, 2018 8:54 PM > To: Brijesh Singh ; edk2-devel@lists.01.org > Cc: Tom Lendacky ; Dong, Eric=20 > ; Zeng, Star ; Justen,=20 > Jordan L > Subject: Re: [edk2] [RFC PATCH 1/1] OvmfPkg/QemuFlash: Fix Runtime=20 > variable access when SEV is enabled >=20 > 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=20 >> guest then it fails to install the bootloader. The installer was=20 >> failing to update the 'BootOrder' UEFI runtime variable. >> >> 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. >> AmdSevDxe maps the range as "unencrypted" but later >> FtwNotificationEvent() in >> MdeModule/Universal/Variable/RuntimeDxe/VariableDxe.c resets the mapping= and the memory region gets remapped as "encrypted". >=20 > Is this a new issue, or has it always been there, and we just failed to n= otice it? >=20 > BTW, I don't understand why FtwNotificationEvent() in "MdeModulePkg/Unive= rsal/Variable/RuntimeDxe/VariableDxe.c" has to mark the flash range as EFI_= MEMORY_RUNTIME. I thought that this action belonged in the flash driver its= elf, and we do that already in MarkMemoryRangeForRuntimeAccess(), in file "= OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c". >=20 > 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 attri= butes. >=20 > Here's a suggestion -- Star, Eric, can you please comment? In the > FtwNotificationEvent() function, after we get the memory descriptor for t= he 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. >=20 > This should cause no observable change on any non-SEV platform, and it sh= ould remove the gDS->SetMemorySpaceAttributes() call for OVMF (where it bre= aks things for SEV). >=20 >> 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 >=20 > (I continue to be annoyed that the memory encryption bit is not=20 > exposed in the GCD memory space attributes explicitly.) >=20 >> but that was not sufficient because UEFI runtime services are mapped=20 >> as "encrypted" in OS page table >=20 > What do you mean here? Runtime services *code* or runtime services *data*= ? Code must obviously be remain encrypted (otherwise we cannot execute it i= n SEV). Runtime Services Data should also be mapped as encrypted (it is nor= mal RAM that is not used for guest<->hypervisor exchange). >=20 >> hence we end up accessing the flash as encrypted when OS requests to upd= ate the variables. >=20 > I don't understand the "hence" here; I don't see how the implication foll= ows. runtime services code and data should be encrypted. Runtime MMIO shoul= d be un-encrypted. >=20 > Ohh, wait, in MarkMemoryRangeForRuntimeAccess(), we use "EfiGcdMemoryType= SystemMemory". I don't have a clue why that is a good idea. That should hav= e been EfiGcdMemoryTypeMemoryMappedIo. >=20 > ... Anyway, I think first we should go with the "check EFI_MEMORY_RUNTIME= attribute before setting it", in FtwNotificationEvent(). >=20 >> >> A possible solution >> --------------------- >> To solve the issue, after QemuFlash is probed, I allocate an=20 >> encrypted buffer and initialize this buffer with the contents from the f= lash 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 dat= a. >=20 > No, this is neither safe, nor a desirable design. >=20 > Safety: all accesses (via both pointers and FVB protocol members) that hi= gher-level drives *think* go to the pflash chip must *actually* go to the p= flash chip. >=20 > Design: it had taken us years to get rid of various memory-emulated fake = variable stores. They *all* suck in one way or another, with various obscur= e UEFI spec incompatibilities and corner cases. A strictly pflash-based var= store is not what we should compromise on. >=20 >> 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 proces= s. >> >> 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=20 >> then I am open to explore those as well. >=20 > I'd like to understand the following: >=20 > (1) why does FtwNotificationEvent() set the EFI_MEMORY_RUNTIME attribute = itself, for the pflash range? -- in my opinion, that belongs in the flash d= river. >=20 > (2) Whether Star and Eric agree with setting the EFI_MEMORY_RUNTIME attri= bute in FtwNotificationEvent() only if the attribute is not already present= . >=20 > (3) The implication that you describe, between runtime services/code bein= g mapped encrypted, and restoring the C-bit failing. >=20 > (4) Whether we should modify MarkMemoryRangeForRuntimeAccess() to=20 > install the range into GCD as MMIO. (I feel *very* uncomfortable about=20 > this, however; the current code has existed as-is for years, and=20 > regressions look very risky.) >=20 > My strong preference would be a patch for (2). >=20 > Thanks, > Laszlo >=20 >> 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(-) >> >> diff --git >> a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf >> b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf >> index d7b4ec06c4e6..6bb5c2093790 100644 >> ---=20 >> a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf >> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.in >> +++ f >> @@ -54,6 +54,7 @@ [LibraryClasses] >> DevicePathLib >> DxeServicesTableLib >> MemoryAllocationLib >> + MemEncryptSevLib >> PcdLib >> UefiBootServicesTableLib >> UefiDriverEntryPoint >> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c >> 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 >=20 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel