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.web10.23063.1574339520579180599 for ; Thu, 21 Nov 2019 04:32:00 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=N5TT2qOL; 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=1574339519; 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=1prpfOrScaSru8cc8vBU7t/TqKZ8XWtZe2pnBJCrDcw=; b=N5TT2qOLogS14GYHV4Sq5l69zCmbfcrAEdPpbdbiFVHzVyv6wAr7EUBChpD0RtzcXn3eU7 KWvNHZoUov4g+1r7PchoAWR14RRXlCMcdapXcYzdQmayzPLTCIJeX5jZJHZuv2m/XLyDYP rtzydtyKP6vVG27GR+ZmCfzXKiAK9ZA= 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-232-Wd3j9_yJOaGwEnqCGrIbYg-1; Thu, 21 Nov 2019 07:31:56 -0500 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 216A35B380; Thu, 21 Nov 2019 12:31:54 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-197.ams2.redhat.com [10.36.116.197]) by smtp.corp.redhat.com (Postfix) with ESMTP id EEB20107A43C; Thu, 21 Nov 2019 12:31:51 +0000 (UTC) Subject: Re: [edk2-devel] [RFC PATCH v3 32/43] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Bypass flash detection with SEV-ES is enabled 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: From: "Laszlo Ersek" Message-ID: <9a6a012c-9bdd-b3cb-65b3-79fda0158e88@redhat.com> Date: Thu, 21 Nov 2019 13:31:50 +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: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-MC-Unique: Wd3j9_yJOaGwEnqCGrIbYg-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: quoted-printable 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-only > by the hypervisor), this check may result in an infinite nested page faul= t > 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. >=20 > When SEV-ES is enabled, exit the flash detection early and assume that > the FD behaves as Flash. This will result in QemuFlashWrite() being calle= d > 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 VmgMmioWrite function from the VmgExitLib library to have the > 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 > 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(-) >=20 > 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/DxeQemuFwCfgS3LibFwCfg.i= nf > + VmgExitLib|UefiCpuPkg/Library/VmgExitLib/VmgExitLib.inf > =20 > [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/DxeQemuFwCfgS3LibFwCfg.i= nf > + VmgExitLib|UefiCpuPkg/Library/VmgExitLib/VmgExitLib.inf > =20 > [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/DxeQemuFwCfgS3LibFwCfg.i= nf > + VmgExitLib|UefiCpuPkg/Library/VmgExitLib/VmgExitLib.inf > =20 > [LibraryClasses.common.UEFI_DRIVER] > PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf > diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe= .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 > =20 > [LibraryClasses] > @@ -52,6 +53,7 @@ [LibraryClasses] > UefiBootServicesTableLib > UefiDriverEntryPoint > UefiRuntimeLib > + VmgExitLib > =20 > [Guids] > gEfiEventVirtualAddressChangeGuid # ALWAYS_CONSUMED These hunks look good. > diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c b/OvmfPkg= /QemuFlashFvbServicesRuntimeDxe/QemuFlash.c > index c81c58972bf2..4f3bf690fcad 100644 > --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c > +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c > @@ -9,7 +9,9 @@ > =20 > #include > #include > +#include > #include > +#include > =20 > #include "QemuFlash.h" > =20 > @@ -80,6 +82,20 @@ QemuFlashDetected ( > =20 > DEBUG ((EFI_D_INFO, "QEMU Flash: Attempting flash detection at %p\n", = Ptr)); > =20 > + 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 FD behaves as > + // a ROM, the nested page table entry is read-only. The check below (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".) > + // will cause a nested page fault that cannot be emulated, causing > + // the instruction to retried over and over. For SEV-ES, assume that > + // 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. > + // > + 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 ( > } > =20 > =20 > +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 > =20 (4) This change will break the "FvbServicesSmm.inf" build of this module. (Because, you have -- correctly -- added the VmgExitLib class to "FvbServicesRuntimeDxe.inf" only.) Please: - create two implementations of the QemuFlashPtrWrite() function, one in "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. > @@ -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++; > } > =20 > @@ -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); > } > =20 > return EFI_SUCCESS; >=20 With (1) and (4) fixed, and with (2) and (3) optionally fixed (i.e., if you agree): Reviewed-by: Laszlo Ersek Thanks Laszlo