From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web08.3535.1609842542779256757 for ; Tue, 05 Jan 2021 02:29:02 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=ihJGW1NG; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1609842542; 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=Euj6OZ9tA+arAIxRZln50kj5W6v7c5owFCdgl5QIhVs=; b=ihJGW1NGWOS+ngYuJl0bPZpPU5nxXkQBdyFZaCzKwHsegyvzXqNXGLFQPgmkT48aBNAHDa G9batl4Cgg1iFE9NPsE4/7H/LjS8SyU0e1AFGgtZsi0rWRKoUrLv2E9hesMvExVZ6tjhBq 4YencVBzHN2qEnQ6Rv9qSO7x5nZLbao= 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-473-AZqeg2mcPEKkwACBguew-Q-1; Tue, 05 Jan 2021 05:28:58 -0500 X-MC-Unique: AZqeg2mcPEKkwACBguew-Q-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 71BAD107ACF9; Tue, 5 Jan 2021 10:28:56 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-233.ams2.redhat.com [10.36.112.233]) by smtp.corp.redhat.com (Postfix) with ESMTP id DE5AF5D9C6; Tue, 5 Jan 2021 10:28:54 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 12/12] OvfmPkg/VmgExitLib: Validate #VC MMIO is to un-encrypted memory To: devel@edk2.groups.io, thomas.lendacky@amd.com Cc: Brijesh Singh , James Bottomley , Jordan Justen , Ard Biesheuvel References: <36dee4fe5bcc0d982b25a429fd37269bce72346e.1608065471.git.thomas.lendacky@amd.com> From: "Laszlo Ersek" Message-ID: <6490ab16-b27b-1538-7cef-e05d7065efda@redhat.com> Date: Tue, 5 Jan 2021 11:28:53 +0100 MIME-Version: 1.0 In-Reply-To: <36dee4fe5bcc0d982b25a429fd37269bce72346e.1608065471.git.thomas.lendacky@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 12/15/20 21:51, Lendacky, Thomas wrote: > From: Tom Lendacky > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3108 > > When SEV-ES is active, and MMIO operation will trigger a #VC and the > VmgExitLib exception handler will process this MMIO operation. > > A malicious hypervisor could try to extract information from encrypted > memory by setting a reserved bit in the guests nested page tables for > a non-MMIO area. This can result in the encrypted data being copied into > the GHCB shared buffer area and accessed by the hypervisor. > > Prevent this by ensuring that the MMIO source/destination is un-encrypted > memory. For the APIC register space, access is allowed in general. > > Cc: Jordan Justen > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Cc: Brijesh Singh > Signed-off-by: Tom Lendacky > --- > OvmfPkg/AmdSev/AmdSevX64.dsc | 1 + > OvmfPkg/OvmfPkgX64.dsc | 1 + > .../DxeBaseMemEncryptSevLib.inf | 2 +- > OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf | 1 + > OvmfPkg/Library/VmgExitLib/VmgExitLib.inf | 2 + > OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 81 +++++++++++++++++++ > 6 files changed, 87 insertions(+), 1 deletion(-) > > diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc > index 3e5a3f648ad5..d0e9d28fc492 100644 > --- a/OvmfPkg/AmdSev/AmdSevX64.dsc > +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc > @@ -237,6 +237,7 @@ [LibraryClasses.common.SEC] > CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf > !endif > VmgExitLib|OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf > + MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/SecBaseMemEncryptSevLib.inf > > [LibraryClasses.common.PEI_CORE] > HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc > index 226b576545a9..2a230888c636 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -265,6 +265,7 @@ [LibraryClasses.common.SEC] > CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf > !endif > VmgExitLib|OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf > + MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/SecBaseMemEncryptSevLib.inf > > [LibraryClasses.common.PEI_CORE] > HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeBaseMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeBaseMemEncryptSevLib.inf > index 04728a5dd256..10f794759207 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeBaseMemEncryptSevLib.inf > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeBaseMemEncryptSevLib.inf > @@ -14,7 +14,7 @@ [Defines] > FILE_GUID = c1594631-3888-4be4-949f-9c630dbc842b > MODULE_TYPE = BASE > VERSION_STRING = 1.0 > - LIBRARY_CLASS = MemEncryptSevLib|DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_DRIVER > + LIBRARY_CLASS = MemEncryptSevLib|DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_DRIVER > > # > # The following information is for reference only and not required by the build > diff --git a/OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf b/OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf > index df14de3c21bc..9c8de326f3d1 100644 > --- a/OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf > +++ b/OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf > @@ -35,6 +35,7 @@ [LibraryClasses] > BaseLib > BaseMemoryLib > DebugLib > + MemEncryptSevLib > PcdLib > > [FixedPcd] > diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitLib.inf b/OvmfPkg/Library/VmgExitLib/VmgExitLib.inf > index b3c3e56ecff8..c66c68726cdb 100644 > --- a/OvmfPkg/Library/VmgExitLib/VmgExitLib.inf > +++ b/OvmfPkg/Library/VmgExitLib/VmgExitLib.inf > @@ -35,4 +35,6 @@ [LibraryClasses] > BaseLib > BaseMemoryLib > DebugLib > + LocalApicLib > + MemEncryptSevLib > (1) I don't understand why LocalApicLib is added only to "VmgExitLib.inf", and not "SecVmgExitLib.inf". The source file "VmgExitVcHandler.c" is shared between both INF files, and that file gets a GetLocalApicBaseAddress() call below. And, "SecVmgExitLib.inf" doesn't list the LocalApicLib class dependency from any earlier patch. ... Hm, the issue is masked because "OvmfPkg/Sec/SecMain.inf" lists LocalApicLib already, so when the SEC module is linked, the LocalApicLib dependency is ultimately (independently) noted/satisfied. But, that doesn't make this omission right; please amend the "SecVmgExitLib.inf" file. With that update: Acked-by: Laszlo Ersek Thanks Laszlo > diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c > index ce577e4677eb..24259060fd65 100644 > --- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c > +++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c > @@ -9,6 +9,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -595,6 +596,61 @@ UnsupportedExit ( > return Status; > } > > +/** > + Validate that the MMIO memory access is not to encrypted memory. > + > + Examine the pagetable entry for the memory specified. MMIO should not be > + performed against encrypted memory. MMIO to the APIC page is always allowed. > + > + @param[in] Ghcb Pointer to the Guest-Hypervisor Communication Block > + @param[in] MemoryAddress Memory address to validate > + @param[in] MemoryLength Memory length to validate > + > + @retval 0 Memory is not encrypted > + @return New exception value to propogate > + > +**/ > +STATIC > +UINT64 > +ValidateMmioMemory ( > + IN GHCB *Ghcb, > + IN UINTN MemoryAddress, > + IN UINTN MemoryLength > + ) > +{ > + MEM_ENCRYPT_SEV_ADDRESS_RANGE_STATE State; > + GHCB_EVENT_INJECTION GpEvent; > + UINTN Address; > + > + // > + // Allow APIC accesses (which will have the encryption bit set during > + // SEC and PEI phases). > + // > + Address = MemoryAddress & ~(SIZE_4KB - 1); > + if (Address == GetLocalApicBaseAddress ()) { > + return 0; > + } > + > + State = MemEncryptSevGetAddressRangeState ( > + 0, > + MemoryAddress, > + MemoryLength > + ); > + if (State == MemEncryptSevAddressRangeUnencrypted) { > + return 0; > + } > + > + // > + // Any state other than unencrypted is an error, issue a #GP. > + // > + GpEvent.Uint64 = 0; > + GpEvent.Elements.Vector = GP_EXCEPTION; > + GpEvent.Elements.Type = GHCB_EVENT_INJECTION_TYPE_EXCEPTION; > + GpEvent.Elements.Valid = 1; > + > + return GpEvent.Uint64; > +} > + > /** > Handle an MMIO event. > > @@ -653,6 +709,11 @@ MmioExit ( > return UnsupportedExit (Ghcb, Regs, InstructionData); > } > > + Status = ValidateMmioMemory (Ghcb, InstructionData->Ext.RmData, Bytes); > + if (Status != 0) { > + return Status; > + } > + > ExitInfo1 = InstructionData->Ext.RmData; > ExitInfo2 = Bytes; > CopyMem (Ghcb->SharedBuffer, &InstructionData->Ext.RegData, Bytes); > @@ -683,6 +744,11 @@ MmioExit ( > InstructionData->ImmediateSize = Bytes; > InstructionData->End += Bytes; > > + Status = ValidateMmioMemory (Ghcb, InstructionData->Ext.RmData, Bytes); > + if (Status != 0) { > + return Status; > + } > + > ExitInfo1 = InstructionData->Ext.RmData; > ExitInfo2 = Bytes; > CopyMem (Ghcb->SharedBuffer, InstructionData->Immediate, Bytes); > @@ -717,6 +783,11 @@ MmioExit ( > return UnsupportedExit (Ghcb, Regs, InstructionData); > } > > + Status = ValidateMmioMemory (Ghcb, InstructionData->Ext.RmData, Bytes); > + if (Status != 0) { > + return Status; > + } > + > ExitInfo1 = InstructionData->Ext.RmData; > ExitInfo2 = Bytes; > > @@ -748,6 +819,11 @@ MmioExit ( > case 0xB7: > Bytes = (Bytes != 0) ? Bytes : 2; > > + Status = ValidateMmioMemory (Ghcb, InstructionData->Ext.RmData, Bytes); > + if (Status != 0) { > + return Status; > + } > + > ExitInfo1 = InstructionData->Ext.RmData; > ExitInfo2 = Bytes; > > @@ -774,6 +850,11 @@ MmioExit ( > case 0xBF: > Bytes = (Bytes != 0) ? Bytes : 2; > > + Status = ValidateMmioMemory (Ghcb, InstructionData->Ext.RmData, Bytes); > + if (Status != 0) { > + return Status; > + } > + > ExitInfo1 = InstructionData->Ext.RmData; > ExitInfo2 = Bytes; > >