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.web12.79.1610040475958956927 for ; Thu, 07 Jan 2021 09:27:56 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=FJ/UBuvT; 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=1610040475; 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=sNnDvEt4BogdY6Wx6q4wR/5/0K3fWPG+1Unjh18b5DU=; b=FJ/UBuvTmmKUFPvDMfDEwfu3uc22jc15WcU12rTmynxvDt9p+GV1JReO5NH4ifWs4gtNRn S4wFoRIMVy1FoeNQ6ErDh5Uf+9osJkkhUKEPvsiQYbLt0ZuQHKLPz8A7Df2t6j0e6Sx+An 30ryaHN3SsyY+Dm/IvPMxgHqsM3QbDs= 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-545-YVHmCmk5P9yLUAivLj8Bnw-1; Thu, 07 Jan 2021 12:27:51 -0500 X-MC-Unique: YVHmCmk5P9yLUAivLj8Bnw-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 D9EF11922021; Thu, 7 Jan 2021 17:27:49 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-164.ams2.redhat.com [10.36.112.164]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2EACC60BF1; Thu, 7 Jan 2021 17:27:48 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 15/15] 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: From: "Laszlo Ersek" Message-ID: <066c0b78-2177-561a-6c62-e0ab9b83fca2@redhat.com> Date: Thu, 7 Jan 2021 18:27:47 +0100 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 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 01/06/21 22:21, 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 > Acked-by: Laszlo Ersek > Signed-off-by: Tom Lendacky > --- > OvmfPkg/AmdSev/AmdSevX64.dsc | 1 + > OvmfPkg/OvmfPkgX64.dsc | 1 + > OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf | 2 +- > OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf | 2 + > OvmfPkg/Library/VmgExitLib/VmgExitLib.inf | 2 + > OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 81 ++++++++++++++++++++ > 6 files changed, 88 insertions(+), 1 deletion(-) Looks OK, thanks. Laszlo > diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc > index c4d93f39b9f1..dad8635c3388 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/SecMemEncryptSevLib.inf > > [LibraryClasses.common.PEI_CORE] > HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc > index bfa9dd7cac1f..70ff2bcf2342 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -266,6 +266,7 @@ [LibraryClasses.common.SEC] > CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf > !endif > VmgExitLib|OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf > + MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLib.inf > > [LibraryClasses.common.PEI_CORE] > HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf > index 8e3b8ddd5a95..f2e162d68076 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.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..e6f6ea7972fd 100644 > --- a/OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf > +++ b/OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf > @@ -35,6 +35,8 @@ [LibraryClasses] > BaseLib > BaseMemoryLib > DebugLib > + LocalApicLib > + 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 > > 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; > >