From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web12.5722.1574425217042979702 for ; Fri, 22 Nov 2019 04:20:17 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=fFhUj5u+; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1574425216; 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=NQyE7QVo/ITbgs+er/enIRbFXotXOAc0w1s4e7Gbl28=; b=fFhUj5u+d2xtokOC9RSik573UOAnuXbcydSM2gF1UACickTbbdDnBhvUlsJH9ZUOWDTsGR 5zkgmfJbMhCYa18HrwID5V7opJiKR2T06CgCZ5pdnWvt2N1kvbm7RtYUJ+/bAeykRgdcZb dcfQEG1bs4GVHbZY+ZfAdikbDFjzY4I= 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-34-Pcf_UJneM3C3np3-Icw7_g-1; Fri, 22 Nov 2019 07:20:13 -0500 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id B55E28CE664; Fri, 22 Nov 2019 12:20:11 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (unknown [10.36.118.23]) by smtp.corp.redhat.com (Postfix) with ESMTP id 916FF19C70; Fri, 22 Nov 2019 12:20:09 +0000 (UTC) Subject: Re: [edk2-devel] [RFC PATCH v3 32/43] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Bypass flash detection with SEV-ES is enabled To: Tom Lendacky , devel@edk2.groups.io Cc: Jordan Justen , Ard Biesheuvel , Michael D Kinney , Liming Gao , Eric Dong , Ray Ni , Brijesh Singh References: <9a6a012c-9bdd-b3cb-65b3-79fda0158e88@redhat.com> <3b5f9e44-2d41-23ca-ae58-b445094e3e26@amd.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 22 Nov 2019 13:20:08 +0100 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: <3b5f9e44-2d41-23ca-ae58-b445094e3e26@amd.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-MC-Unique: Pcf_UJneM3C3np3-Icw7_g-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: quoted-printable On 11/21/19 22:11, Tom Lendacky wrote: > On 11/21/19 6:31 AM, Laszlo Ersek via Groups.Io wrote: >> On 11/20/19 21:06, Lendacky, Thomas wrote: >>> 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-on= ly >>> by the hypervisor), this check may result in an infinite nested page f= ault >>> because of the attempted write. Since the instruction cannot be emulat= ed >>> when SEV-ES is enabled, the RIP is never advanced, resulting in repeat= ed >>> 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 ca= lled >>> to store EFI variables, which will also result in an infinite nested p= age >>> fault when the write is performed. In this case, update QemuFlashWrite= () >>> to use the VmgMmioWrite function from the VmgExitLib library to have t= he >>> hypervisor perform the write without having to emulate the instruction= . >> >> (1) Please include the Bugzilla link in the commit message: >> >> https://bugzilla.tianocore.org/show_bug.cgi?id=3D2198 >=20 > Yup, will do. I'm also missing the Cc:'s, so I'll add those as well. >=20 >> >>> >>> Signed-off-by: Tom Lendacky >>> --- >>> OvmfPkg/OvmfPkgIa32.dsc | 1 + >>> OvmfPkg/OvmfPkgIa32X64.dsc | 1 + >>> OvmfPkg/OvmfPkgX64.dsc | 1 + >>> .../FvbServicesRuntimeDxe.inf | 2 + >>> .../QemuFlash.c | 38 +++++++++++++++++-= - >>> 5 files changed, 40 insertions(+), 3 deletions(-) >>> >>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc >>> index 56670eefde6b..ff2814c6246e 100644 >>> --- a/OvmfPkg/OvmfPkgIa32.dsc >>> +++ b/OvmfPkg/OvmfPkgIa32.dsc >>> @@ -320,6 +320,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER] >>> BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf >>> PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf >>> QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCf= g.inf >>> + VmgExitLib|UefiCpuPkg/Library/VmgExitLib/VmgExitLib.inf >>> >>> [LibraryClasses.common.UEFI_DRIVER] >>> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf >>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc >>> index 9897e6889573..212952cfaacd 100644 >>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc >>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc >>> @@ -325,6 +325,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER] >>> BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf >>> PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf >>> QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCf= g.inf >>> + VmgExitLib|UefiCpuPkg/Library/VmgExitLib/VmgExitLib.inf >>> >>> [LibraryClasses.common.UEFI_DRIVER] >>> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf >>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc >>> index 59c4f9207fc3..8331fc0b663e 100644 >>> --- a/OvmfPkg/OvmfPkgX64.dsc >>> +++ b/OvmfPkg/OvmfPkgX64.dsc >>> @@ -325,6 +325,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER] >>> BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf >>> PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf >>> QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCf= g.inf >>> + VmgExitLib|UefiCpuPkg/Library/VmgExitLib/VmgExitLib.inf >>> >>> [LibraryClasses.common.UEFI_DRIVER] >>> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf >>> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntime= Dxe.inf b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf >>> index ca6326e833ed..0b7741ac07f8 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 >> >> These hunks look good. >> >>> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c b/Ovmf= Pkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c >>> index c81c58972bf2..4f3bf690fcad 100644 >>> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c >>> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c >>> @@ -9,7 +9,9 @@ >>> >>> #include >>> #include >>> +#include >>> #include >>> +#include >>> >>> #include "QemuFlash.h" >>> >>> @@ -80,6 +82,20 @@ QemuFlashDetected ( >>> >>> DEBUG ((EFI_D_INFO, "QEMU Flash: Attempting flash detection at %p\n= ", Ptr)); >>> >>> + if (MemEncryptSevEsIsEnabled()) { >>> + // >>> + // When SEV-ES is enabled, the check below can result in an infin= ite >>> + // loop with respect to a nested page fault. When the FD behaves = as >>> + // a ROM, the nested page table entry is read-only. The check bel= ow >> >> (2) I suggest a slight change to the wording above. Please replace >> >> when the FD behaves as a ROM >> >> with >> >> when the memslot is mapped read-only >> >> (It's OK to make qemu / kvm references in this code: the name of the >> file includes "Qemu".) >=20 > Ok. >=20 >> >>> + // will cause a nested page fault that cannot be emulated, causin= g >>> + // the instruction to retried over and over. For SEV-ES, assume t= hat >>> + // the FD does not behave as FLASH. >> >> (3) I would recommend >> >> For SEV-ES, acknowledge that the FD appears as ROM and not as FLASH, >> but report FLASH anyway. >=20 > How about if I expand on this to add "because FLASH behavior can be > simulated using VMGEXIT.", too. Sure! Thanks! Laszlo >=20 >> >>> + // >>> + DEBUG ((DEBUG_INFO, >>> + "QEMU Flash: SEV-ES enabled, assuming FD behaves as FLASH\n")); >>> + return TRUE; >>> + } >>> + >>> OriginalUint8 =3D *Ptr; >>> *Ptr =3D CLEAR_STATUS_CMD; >>> ProbeUint8 =3D *Ptr; >>> @@ -147,6 +163,21 @@ QemuFlashRead ( >>> } >>> >>> >>> +STATIC >>> +VOID >>> +QemuFlashPtrWrite ( >>> + IN volatile UINT8 *Ptr, >>> + IN UINT8 Value >>> + ) >>> +{ >>> + if (MemEncryptSevEsIsEnabled()) { >>> + VmgMmioWrite ((UINT8 *) Ptr, &Value, 1); >>> + } else { >>> + *Ptr =3D Value; >>> + } >>> +} >>> + >>> + >>> /** >>> Write to QEMU Flash >>> >> >> (4) This change will break the "FvbServicesSmm.inf" build of this modul= e. >> >> (Because, you have -- correctly -- added the VmgExitLib class to >> "FvbServicesRuntimeDxe.inf" only.) >> >> Please: >> >> - create two implementations of the QemuFlashPtrWrite() function, one i= n >> "QemuFlashDxe.c", and another in "QemuFlashSmm.c". >> >> - the former should be the function shown above, the latter should only >> perform the direct assignment. >> >> - the '#include ' directive should also be moved >> to "QemuFlashDxe.c". >> >> - the prototype for QemuFlashPtrWrite() should be added to "QemuFlash.h= " >> please. >=20 > Ah, nice catch. I'll do that. >=20 > Thanks, > Tom >=20 >> >>> @@ -181,8 +212,9 @@ QemuFlashWrite ( >>> // >>> Ptr =3D QemuFlashPtr (Lba, Offset); >>> for (Loop =3D 0; Loop < *NumBytes; Loop++) { >>> - *Ptr =3D WRITE_BYTE_CMD; >>> - *Ptr =3D Buffer[Loop]; >>> + QemuFlashPtrWrite (Ptr, WRITE_BYTE_CMD); >>> + QemuFlashPtrWrite (Ptr, Buffer[Loop]); >>> + >>> Ptr++; >>> } >>> >>> @@ -190,7 +222,7 @@ QemuFlashWrite ( >>> // Restore flash to read mode >>> // >>> if (*NumBytes > 0) { >>> - *(Ptr - 1) =3D READ_ARRAY_CMD; >>> + QemuFlashPtrWrite (Ptr - 1, READ_ARRAY_CMD); >>> } >>> >>> return EFI_SUCCESS; >>> >> >> With (1) and (4) fixed, and with (2) and (3) optionally fixed (i.e., if >> you agree): >> >> Reviewed-by: Laszlo Ersek >> >> Thanks >> Laszlo >> >> >>=20 >> >=20