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 0825A202E53F1 for ; Thu, 28 Jun 2018 05:57:39 -0700 (PDT) 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 8D65B400B45F; Thu, 28 Jun 2018 12:57:38 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-10.rdu2.redhat.com [10.10.120.10]) by smtp.corp.redhat.com (Postfix) with ESMTP id E5AE22026D69; Thu, 28 Jun 2018 12:57:36 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org Cc: Tom Lendacky , Star Zeng , Eric Dong , "Jordan Justen (Intel address)" References: <1530042365-9979-1-git-send-email-brijesh.singh@amd.com> <272c4a0f-fcc1-2899-e31d-a3207feb51ed@redhat.com> <2a662245-bb03-e742-1403-4d0a47bffda7@amd.com> From: Laszlo Ersek Message-ID: Date: Thu, 28 Jun 2018 14:57:36 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <2a662245-bb03-e742-1403-4d0a47bffda7@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.6]); Thu, 28 Jun 2018 12:57:38 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Thu, 28 Jun 2018 12:57:38 +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: [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 12:57:40 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 06/27/18 19:49, Brijesh Singh wrote: > > > On 06/27/2018 11:59 AM, Laszlo Ersek wrote: >> On 06/27/18 18:34, Brijesh Singh wrote: >>> On 06/27/2018 07:54 AM, Laszlo Ersek wrote: >>>> On 06/26/18 21:46, Brijesh Singh wrote: >> >>>>> After that, any access >>>>> to the flash will end up going through the encryption engine. I did >>>>> 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 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 normal RAM that is not used for guest<->hypervisor >>>> exchange). >>> >>> Sorry, I was meaning to say both the "code" and "data" are mapped as >>> encrypted by the OS. >>> >>>>> hence we end up accessing the flash as encrypted when OS requests to >>>>> update the variables. >>>> >>>> I don't understand the "hence" here; I don't see how the implication >>>> follows. runtime services code and data should be encrypted. Runtime >>>> MMIO should be un-encrypted. >>>> >>>> Ohh, wait, in MarkMemoryRangeForRuntimeAccess(), we use >>>> "EfiGcdMemoryTypeSystemMemory". I don't have a clue why that is a good >>>> idea. That should have been EfiGcdMemoryTypeMemoryMappedIo. >>> >>> Right, the memory is marked as 'system ram' and not 'mmio'. >>> Just to experiment, I did try changing it to 'mmio' to see if OS will >>> map this  region as "unencrypted" but ovmf fails with below error >>> message after changing it from systemRAM->mmio >>> >>> ConvertPages: failed to find range FFC00000 - FFFFFFFF >>> ASSERT_EFI_ERROR (Status = Not Found) >>> ASSERT [FvbServicesRuntimeDxe] >>> /home/amd/workdir/upstream/edk2/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockServie.c(864): >>> >>> !EFI_ERROR (Status) >> >> This error occurs because (I think) you modified only the AddMemorySpace >> call. If you change the GCD type on that, then please update the >> subsequent AllocatePages as well, from EfiRuntimeServicesData to >> EfiMemoryMappedIO. >> > > Here is what I have. > > --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c > +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c > @@ -847,7 +847,7 @@ MarkMemoryRangeForRuntimeAccess ( >                    ); > >    Status = gDS->AddMemorySpace ( > -                  EfiGcdMemoryTypeSystemMemory, > +                  EfiGcdMemoryTypeMemoryMappedIo, >                    BaseAddress, >                    Length, >                    EFI_MEMORY_UC | EFI_MEMORY_RUNTIME > @@ -856,7 +856,7 @@ MarkMemoryRangeForRuntimeAccess ( > >    Status = gBS->AllocatePages ( >                    AllocateAddress, > -                  EfiRuntimeServicesData, > +                  EfiMemoryMappedIO, >                    EFI_SIZE_TO_PAGES (Length), >                    &BaseAddress >                    ); > > I am still getting the error assertion failure. I can debug to see what > is going on. Hmmm. Indeed, memory space added to GCD need not immediately show up in the UEFI memory map, for the UEFI memory allocation services to allocate from. IIRC, the PI spec says that *system memory* added like this *may* show up immediately: """ If the memory range specified by BaseAddress and Length is of type EfiGcdMemoryTypeSystemMemory or EfiGcdMemoryTypeMoreReliable, then the memory range may be automatically allocated for use by the UEFI memory services. """ and, indeed in edk2, it happens at once; but it doesn't apply to MMIO at all. So, can you replace the AllocatePages call with the following: Status = gDS->AllocateMemorySpace ( EfiGcdAllocateAddress, EfiGcdMemoryTypeMemoryMappedIo, 0, // Alignment EFI_SIZE_TO_PAGES (Length), &BaseAddress, gImageHandle, NULL // DeviceHandle ); >> The spec says about the latter enum constant, "Used by system firmware >> to request that a memory-mapped IO region be mapped by the OS to a >> virtual address so it can be accessed by EFI runtime services." It seems >> appropriate (and I'm a bit confused why we haven't used the MMIO GCD and >> UEFI enum values for the memory type, all this time.) >> >>> Since this efi runtime data is mapped as C=1 by the OS, hence when OS >>> asks efi to update the runtime variable we end up accessing the memory >>> region with C=1 (runtime services are executed using OS pagetable). >> >> Indeed. >> >> (And, this is only a problem when SMM is not used, i.e. when the full >> variable driver stack is non-SMM, just DXE. In the SMM case, the SMM >> page tables are used, and the OS cannot interfere with that.) >> > > Good point, I will try it and let you know. As you say since SMM uses > UEFI page table More correctly: SMM drivers use, at runtime as well, the page table in SMRAM that was created by the firmware. > hence after fixing FtwNotificationEvent(..) we should be > good. No, that's not precise -- FtwNotificationEvent() is not used in SMM *at all*. I checked that before I sent my previous email; namely, FtwNotificationEvent() is in "MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c", which is not built into the SMM variant of the variable driver. Please see the variable driver stacks in OVMF: - non-SMM: OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf In the non-SMM case, we have a normal runtime driver, VariableRuntimeDxe.inf, that implements the variable service. It relies on the FTW driver (another runtime driver) for the FTW protocol. Finally, both the variable driver and the FTW driver rely on the FVB (firmware volume block) protocol, which may come from the QEMU flash driver, or from the emulation driver. Again, all of these are runtime drivers. "VariableRuntimeDxe.inf" includes the "VariableDxe.c" file, hence FtwNotificationEvent() is relevant. - SMM: OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf In the SMM case, we have a *split* variable driver, "VariableSmmRuntimeDxe.inf" (which is a normal runtime driver) and "VariableSmm.inf" (a "traditional SMM" driver). The runtime half is unprivileged and only formats a message for the privileged SMM half, submits the message, and then parses the response. The rest of the variable stack, namely the privileged half of the variable driver, and the FTW driver, and the QEMU flash (FVB) driver, all stay within SMM -- they are traditional SMM drivers too. "VariableSmmRuntimeDxe.inf" does not include "VariableDxe.c". Instead, "VariableSmm.inf" (the privileged half) includes "VariableSmm.c", and that source file waits for the *SMM* FVB protocol, in the SmmFtwNotificationEvent() function. But, this notification function, unlike the DXE counterpart FtwNotificationEvent(), does not care about the GCD entry attributes -- because in SMM, separate (protected) page tables are used anyway. If you put FtwNotificationEvent() and SmmFtwNotificationEvent() side by side, you'll see that they perform almost identical steps: - "Ensure [SMM] FTW protocol is installed" - "Find the proper [SMM] FVB protocol for variable" - "Mark the variable storage region of the FLASH as RUNTIME" -- this is DXE only! - VariableWriteServiceInitialize - "Install the Variable Write Architectural protocol" (I skipped RecordSecureBootPolicyVarData(), because that always happens in the DXE half. Anyway it's irrelevant here.) Thus, my expectation is that the current issue manifests itself only if you build OVMF without -D SMM_REQUIRE. >> Anyway, in the pure DXE / runtime driver case, do you think a guest >> kernel patch will be necessary too? Perhaps if you change the UEFI >> memmap entry type (see AllocatePages above) to MMIO, then the guest >> kernel could technically honor that. >> > > > Theoretically speaking, if we are able to make this memory region as > mmio then OS should be able to map it with C=0. Thanks Laszlo