From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web10.51535.1590502059512953172 for ; Tue, 26 May 2020 07:07:39 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=FMTvbuCB; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1590502058; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=uyUxg5pfCyE96mOTGy9PuM+syBeJjTFdz3Q0TUnzluA=; b=FMTvbuCB/EokeGZiuILJ4GkZJKN0NZxcF8FyeA+bbnMztNWCWPbejVHfQ6YYdpm8pgTCYz kWmXKwWAvu9CQNw6qnc25bvJnEgkFCOEfGiv/Uw3Jp9lH8031LBHXEbWqWyzsugG6IvJwg p6/usIs5GXRytkDVWsfa82z4CK6kN7c= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-70-7B40bGQnNDy9yJhFMb9KUg-1; Tue, 26 May 2020 10:07:34 -0400 X-MC-Unique: 7B40bGQnNDy9yJhFMb9KUg-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 46F0B18831E5; Tue, 26 May 2020 14:07:16 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-136.ams2.redhat.com [10.36.114.136]) by smtp.corp.redhat.com (Postfix) with ESMTP id 17A4D6199F; Tue, 26 May 2020 14:07:13 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v8 39/46] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Bypass flash detection with SEV-ES To: devel@edk2.groups.io, thomas.lendacky@amd.com Cc: Jordan Justen , Ard Biesheuvel , Michael D Kinney , Liming Gao , Eric Dong , Ray Ni , Brijesh Singh References: <4d35621f7ac33a99cd1d473adc559aa4a5aad8c0.1589925074.git.thomas.lendacky@amd.com> From: "Laszlo Ersek" Message-ID: <4ab25014-3f1e-829f-d54d-3c9b2ba9b1c4@redhat.com> Date: Tue, 26 May 2020 16:07:13 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <4d35621f7ac33a99cd1d473adc559aa4a5aad8c0.1589925074.git.thomas.lendacky@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 05/19/20 23:51, Lendacky, Thomas wrote: > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198 > > The flash detection routine will attempt to determine how the flash > device behaves (e.g. ROM, RAM, Flash). But when SEV-ES is enabled and > the flash device behaves as a ROM device (meaning it is marked read-only > by the hypervisor), this check may result in an infinite nested page fault > because of the attempted write. Since the instruction cannot be emulated > when SEV-ES is enabled, the RIP is never advanced, resulting in repeated > nested page faults. > > When SEV-ES is enabled, exit the flash detection early and assume that > the FD behaves as Flash. This will result in QemuFlashWrite() being called > to store EFI variables, which will also result in an infinite nested page > fault when the write is performed. In this case, update QemuFlashWrite() > to use the VMGEXIT MMIO write support to have the hypervisor perform the > write without having to emulate the instruction. > > Cc: Jordan Justen > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Reviewed-by: Laszlo Ersek > Signed-off-by: Tom Lendacky > --- > .../FvbServicesRuntimeDxe.inf | 2 + > .../QemuFlash.h | 13 ++++++ > .../QemuFlash.c | 23 +++++++++-- > .../QemuFlashDxe.c | 40 +++++++++++++++++++ > .../QemuFlashSmm.c | 16 ++++++++ > 5 files changed, 91 insertions(+), 3 deletions(-) - subject line has been cleaned up relative to v6, OK - commit message updated in sync with open-coding VmgMmioWrite() in this patch; and VmgMmioWrite() has been documented in the v8 blurb -- OK > > diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf > index 72cabba4357d..8bb2325157ea 100644 > --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf > +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf > @@ -38,6 +38,7 @@ [Sources] > [Packages] > MdePkg/MdePkg.dec > MdeModulePkg/MdeModulePkg.dec > + UefiCpuPkg/UefiCpuPkg.dec > OvmfPkg/OvmfPkg.dec > > [LibraryClasses] > @@ -52,6 +53,7 @@ [LibraryClasses] > UefiBootServicesTableLib > UefiDriverEntryPoint > UefiRuntimeLib > + VmgExitLib > > [Guids] > gEfiEventVirtualAddressChangeGuid # ALWAYS_CONSUMED > diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h > index f1afabcbe6ae..219d0d6e83cf 100644 > --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h > +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.h > @@ -89,5 +89,18 @@ QemuFlashBeforeProbe ( > IN UINTN FdBlockCount > ); > > +/** > + Write to QEMU Flash > + > + @param[in] Ptr Pointer to the location to write. > + @param[in] Value The value to write. > + > +**/ > +VOID > +QemuFlashPtrWrite ( > + IN volatile UINT8 *Ptr, > + IN UINT8 Value > + ); > + > #endif > - New comment, OK. > diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c > index 1b0d6c053f1a..0d29bf701aca 100644 > --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c > +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c > @@ -9,6 +9,7 @@ > > #include > #include > +#include > #include > > #include "QemuFlash.h" > @@ -80,6 +81,21 @@ QemuFlashDetected ( > > DEBUG ((DEBUG_INFO, "QEMU Flash: Attempting flash detection at %p\n", Ptr)); > > + if (MemEncryptSevEsIsEnabled ()) { > + // > + // When SEV-ES is enabled, the check below can result in an infinite > + // loop with respect to a nested page fault. When the memslot is mapped > + // read-only, the nested page table entry is read-only. The check below > + // will cause a nested page fault that cannot be emulated, causing > + // the instruction to retried over and over. For SEV-ES, acknowledge that > + // the FD appears as ROM and not as FLASH, but report FLASH anyway because > + // FLASH behavior can be simulated using VMGEXIT. > + // > + DEBUG ((DEBUG_INFO, > + "QEMU Flash: SEV-ES enabled, assuming FD behaves as FLASH\n")); > + return TRUE; > + } > + > OriginalUint8 = *Ptr; > *Ptr = CLEAR_STATUS_CMD; > ProbeUint8 = *Ptr; > @@ -181,8 +197,9 @@ QemuFlashWrite ( > // > Ptr = QemuFlashPtr (Lba, Offset); > for (Loop = 0; Loop < *NumBytes; Loop++) { > - *Ptr = WRITE_BYTE_CMD; > - *Ptr = Buffer[Loop]; > + QemuFlashPtrWrite (Ptr, WRITE_BYTE_CMD); > + QemuFlashPtrWrite (Ptr, Buffer[Loop]); > + > Ptr++; > } > > @@ -190,7 +207,7 @@ QemuFlashWrite ( > // Restore flash to read mode > // > if (*NumBytes > 0) { > - *(Ptr - 1) = READ_ARRAY_CMD; > + QemuFlashPtrWrite (Ptr - 1, READ_ARRAY_CMD); > } > > return EFI_SUCCESS; > diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c > index 5aabe9d7b59c..565383ee26d2 100644 > --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c > +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c > @@ -10,6 +10,9 @@ > **/ > > #include > +#include > +#include > +#include - new include directive, most likely due to flattening VmgMmioWrite; OK > > #include "QemuFlash.h" > > @@ -32,3 +35,40 @@ QemuFlashBeforeProbe ( > // Do nothing > // > } > + > +/** > + Write to QEMU Flash > + > + @param[in] Ptr Pointer to the location to write. > + @param[in] Value The value to write. > + > +**/ - new comment, OK > +VOID > +QemuFlashPtrWrite ( > + IN volatile UINT8 *Ptr, > + IN UINT8 Value > + ) > +{ > + if (MemEncryptSevEsIsEnabled ()) { > + MSR_SEV_ES_GHCB_REGISTER Msr; > + GHCB *Ghcb; > + > + Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB); > + Ghcb = Msr.Ghcb; > + > + // > + // Writing to flash is emulated by the hypervisor through the use of write > + // protection. This won't work for an SEV-ES guest because the write won't > + // be recognized as a true MMIO write, which would result in the required > + // #VC exception. Instead, use the the VMGEXIT MMIO write support directly > + // to perform the update. > + // > + VmgInit (Ghcb); > + Ghcb->SharedBuffer[0] = Value; > + Ghcb->SaveArea.SwScratch = (UINT64) (UINTN) Ghcb->SharedBuffer; > + VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, (UINT64) (UINTN) Ptr, 1); > + VmgDone (Ghcb); - this is what replaces VmgMmioWrite(); seems reasonable > + } else { > + *Ptr = Value; > + } > +} > diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c > index 7eb426e03855..7eb80bfeffae 100644 > --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c > +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashSmm.c > @@ -46,3 +46,19 @@ QemuFlashBeforeProbe ( > ); > ASSERT_EFI_ERROR (Status); > } > + > +/** > + Write to QEMU Flash > + > + @param[in] Ptr Pointer to the location to write. > + @param[in] Value The value to write. > + > +**/ - new comment. > +VOID > +QemuFlashPtrWrite ( > + IN volatile UINT8 *Ptr, > + IN UINT8 Value > + ) > +{ > + *Ptr = Value; > +} > Reviewed-by: Laszlo Ersek Thanks Laszlo