From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web12.6036.1603141636358193275 for ; Mon, 19 Oct 2020 14:07:16 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=cyh66W5T; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1603141635; 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=3lbwqiCeeH3gXdp4Un6lvxJhltoXHTACo6DB8vF7ECY=; b=cyh66W5Tzjg3bS/8kRSGRS7MW2J07pqP2kC1ZxuGqAN3/cdUbXMJo0xBH1lgXPUofXZ8Gs 2CGlkuhBnIQexA2QfAXYPNHfzkQOAulCxDnLxJDzE9A3F+gigwpH+mNSqnjSO5sA7r9s1Z wb8gVlXlbeLKdj7bJoQFwdLjKBSC3KM= 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-16-cuEAnYYWPDyk0-jq5upe5Q-1; Mon, 19 Oct 2020 17:07:11 -0400 X-MC-Unique: cuEAnYYWPDyk0-jq5upe5Q-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 923B859; Mon, 19 Oct 2020 21:07:09 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-137.ams2.redhat.com [10.36.113.137]) by smtp.corp.redhat.com (Postfix) with ESMTP id 73F7760C05; Mon, 19 Oct 2020 21:07:07 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 10/11] UefiCpuPkg, OvmfPkg: Disable interrupts when using the GHCB To: devel@edk2.groups.io, thomas.lendacky@amd.com Cc: Brijesh Singh , Eric Dong , Ray Ni , Rahul Kumar , Jordan Justen , Ard Biesheuvel References: <817ead39c5331b3b10055c32c36aa55fdbe2f8b1.1602864557.git.thomas.lendacky@amd.com> From: "Laszlo Ersek" Message-ID: Date: Mon, 19 Oct 2020 23:07:06 +0200 MIME-Version: 1.0 In-Reply-To: <817ead39c5331b3b10055c32c36aa55fdbe2f8b1.1602864557.git.thomas.lendacky@amd.com> 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 10/16/20 18:09, Lendacky, Thomas wrote: > From: Tom Lendacky > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3008 > > The QemuFlashPtrWrite() flash services runtime uses the GHCB and VmgExit() > directly to perform the flash write when running as an SEV-ES guest. If an > interrupt arrives between VmgInit() and VmgExit(), the Dr7 read in the > interrupt handler will generate a #VC, which can overwrite information in > the GHCB that QemuFlashPtrWrite() has set. This has been seen with the > timer interrupt firing and the CpuExceptionHandlerLib library code, > UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ > Xcode5ExceptionHandlerAsm.nasm and > ExceptionHandlerAsm.nasm > reading the Dr7 register while QemuFlashPtrWrite() is using the GHCB. In > general, it is necessary to protect the GHCB whenever it is used, not just > in QemuFlashPtrWrite(). > > Disable interrupts around the usage of the GHCB by modifying the VmgInit() > and VmgDone() interfaces: > - VmgInit() will take an extra parameter that is a pointer to a BOOLEAN > that will hold the interrupt state at the time of invocation. VmgInit() > will get and save this interrupt state before updating the GHCB. > - VmgDone() will take an extra parameter that is used to indicate whether > interrupts are to be (re)enabled. Before exiting, VmgDone() will enable > interrupts if that is requested. > > Fixes: 437eb3f7a8db7681afe0e6064d3a8edb12abb766 > Cc: Eric Dong > Cc: Ray Ni > Cc: Laszlo Ersek > Cc: Rahul Kumar > Cc: Jordan Justen > Cc: Ard Biesheuvel > Cc: Tom Lendacky > Cc: Brijesh Singh > Signed-off-by: Tom Lendacky > --- > UefiCpuPkg/Include/Library/VmgExitLib.h | 14 ++++++++--- > OvmfPkg/Library/VmgExitLib/VmgExitLib.c | 26 +++++++++++++++++--- > OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 5 ++-- > OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c | 5 ++-- > UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 5 ++-- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 7 +++--- > UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c | 18 ++++++++------ > 7 files changed, 55 insertions(+), 25 deletions(-) > > diff --git a/UefiCpuPkg/Include/Library/VmgExitLib.h b/UefiCpuPkg/Include/Library/VmgExitLib.h > index ba5ea024839e..617b6cf8d2e7 100644 > --- a/UefiCpuPkg/Include/Library/VmgExitLib.h > +++ b/UefiCpuPkg/Include/Library/VmgExitLib.h > @@ -50,13 +50,16 @@ VmgExit ( > Performs the necessary steps in preparation for invoking VMGEXIT. Must be > called before setting any fields within the GHCB. > > - @param[in, out] Ghcb A pointer to the GHCB > + @param[in, out] Ghcb A pointer to the GHCB > + @param[in, out] InterruptState A pointer to hold the current interrupt > + state, used for restoring in VmgDone () > > **/ > VOID > EFIAPI > VmgInit ( > - IN OUT GHCB *Ghcb > + IN OUT GHCB *Ghcb, > + IN OUT BOOLEAN *InterruptState > ); > > /** > @@ -65,13 +68,16 @@ VmgInit ( > Performs the necessary steps to cleanup after invoking VMGEXIT. Must be > called after obtaining needed fields within the GHCB. > > - @param[in, out] Ghcb A pointer to the GHCB > + @param[in, out] Ghcb A pointer to the GHCB > + @param[in] InterruptState An indicator to conditionally (re)enable > + interrupts > > **/ > VOID > EFIAPI > VmgDone ( > - IN OUT GHCB *Ghcb > + IN OUT GHCB *Ghcb, > + IN BOOLEAN InterruptState > ); > > /** > diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c > index ae86d850ba61..18b102df5f9a 100644 > --- a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c > +++ b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c > @@ -132,15 +132,27 @@ VmgExit ( > Performs the necessary steps in preparation for invoking VMGEXIT. Must be > called before setting any fields within the GHCB. > > - @param[in, out] Ghcb A pointer to the GHCB > + @param[in, out] Ghcb A pointer to the GHCB > + @param[in, out] InterruptState A pointer to hold the current interrupt > + state, used for restoring in VmgDone () > > **/ > VOID > EFIAPI > VmgInit ( > - IN OUT GHCB *Ghcb > + IN OUT GHCB *Ghcb, > + IN OUT BOOLEAN *InterruptState > ) > { > + // > + // Be sure that an interrupt can't cause a #VC while the GHCB is > + // being used. > + // > + *InterruptState = GetInterruptState (); > + if (*InterruptState) { > + DisableInterrupts (); > + } > + > SetMem (&Ghcb->SaveArea, sizeof (Ghcb->SaveArea), 0); > } > > @@ -150,15 +162,21 @@ VmgInit ( > Performs the necessary steps to cleanup after invoking VMGEXIT. Must be > called after obtaining needed fields within the GHCB. > > - @param[in, out] Ghcb A pointer to the GHCB > + @param[in, out] Ghcb A pointer to the GHCB > + @param[in] InterruptState An indicator to conditionally (re)enable > + interrupts > > **/ > VOID > EFIAPI > VmgDone ( > - IN OUT GHCB *Ghcb > + IN OUT GHCB *Ghcb, > + IN BOOLEAN InterruptState > ) > { > + if (InterruptState) { > + EnableInterrupts (); > + } > } > > /** > diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c > index 9bf9d160179c..1671db3a01b1 100644 > --- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c > +++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c > @@ -1568,6 +1568,7 @@ VmgExitHandleVc ( > SEV_ES_INSTRUCTION_DATA InstructionData; > UINT64 ExitCode, Status; > EFI_STATUS VcRet; > + BOOLEAN InterruptState; > > VcRet = EFI_SUCCESS; > > @@ -1578,7 +1579,7 @@ VmgExitHandleVc ( > Regs = SystemContext.SystemContextX64; > Ghcb = Msr.Ghcb; > > - VmgInit (Ghcb); > + VmgInit (Ghcb, &InterruptState); > > ExitCode = Regs->ExceptionData; > switch (ExitCode) { > @@ -1662,7 +1663,7 @@ VmgExitHandleVc ( > VcRet = EFI_PROTOCOL_ERROR; > } > > - VmgDone (Ghcb); > + VmgDone (Ghcb, InterruptState); > > return VcRet; > } > diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c > index f9b21b54137d..1b0742967f71 100644 > --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c > +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c > @@ -52,6 +52,7 @@ QemuFlashPtrWrite ( > if (MemEncryptSevEsIsEnabled ()) { > MSR_SEV_ES_GHCB_REGISTER Msr; > GHCB *Ghcb; > + BOOLEAN InterruptState; > > Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB); > Ghcb = Msr.Ghcb; > @@ -63,12 +64,12 @@ QemuFlashPtrWrite ( > // #VC exception. Instead, use the the VMGEXIT MMIO write support directly > // to perform the update. > // > - VmgInit (Ghcb); > + VmgInit (Ghcb, &InterruptState); > Ghcb->SharedBuffer[0] = Value; > Ghcb->SaveArea.SwScratch = (UINT64) (UINTN) Ghcb->SharedBuffer; > VmgSetOffsetValid (Ghcb, GhcbSwScratch); > VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, (UINT64) (UINTN) Ptr, 1); > - VmgDone (Ghcb); > + VmgDone (Ghcb, InterruptState); > } else { > *Ptr = Value; > } > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > index 2c00d72ddefe..7839c249760e 100644 > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > @@ -171,6 +171,7 @@ GetSevEsAPMemory ( > EFI_PHYSICAL_ADDRESS StartAddress; > MSR_SEV_ES_GHCB_REGISTER Msr; > GHCB *Ghcb; > + BOOLEAN InterruptState; > > // > // Allocate 1 page for AP jump table page > @@ -192,9 +193,9 @@ GetSevEsAPMemory ( > Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB); > Ghcb = Msr.Ghcb; > > - VmgInit (Ghcb); > + VmgInit (Ghcb, &InterruptState); > VmgExit (Ghcb, SVM_EXIT_AP_JUMP_TABLE, 0, (UINT64) (UINTN) StartAddress); > - VmgDone (Ghcb); > + VmgDone (Ghcb, InterruptState); > > return (UINTN) StartAddress; > } > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c > index 07426274f639..4f4b26a7c196 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -884,6 +884,7 @@ ApWakeupFunction ( > GHCB *Ghcb; > UINT64 Status; > BOOLEAN DoDecrement; > + BOOLEAN InterruptState; > > DoDecrement = (BOOLEAN) (CpuMpData->InitFlag == ApInitConfig); > > @@ -891,7 +892,7 @@ ApWakeupFunction ( > Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB); > Ghcb = Msr.Ghcb; > > - VmgInit (Ghcb); > + VmgInit (Ghcb, &InterruptState); > > if (DoDecrement) { > DoDecrement = FALSE; > @@ -905,11 +906,11 @@ ApWakeupFunction ( > > Status = VmgExit (Ghcb, SVM_EXIT_AP_RESET_HOLD, 0, 0); > if ((Status == 0) && (Ghcb->SaveArea.SwExitInfo2 != 0)) { > - VmgDone (Ghcb); > + VmgDone (Ghcb, InterruptState); > break; > } > > - VmgDone (Ghcb); > + VmgDone (Ghcb, InterruptState); > } > > // > diff --git a/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c b/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c > index b000232c472e..24defd624c63 100644 > --- a/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c > +++ b/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c > @@ -57,15 +57,16 @@ VmgExit ( > Performs the necessary steps in preparation for invoking VMGEXIT. Must be > called before setting any fields within the GHCB. > > - The base library function does nothing. > - > - @param[in, out] Ghcb A pointer to the GHCB > + @param[in, out] Ghcb A pointer to the GHCB > + @param[in, out] InterruptState A pointer to hold the current interrupt > + state, used for restoring in VmgDone () > > **/ > VOID > EFIAPI > VmgInit ( > - IN OUT GHCB *Ghcb > + IN OUT GHCB *Ghcb, > + IN OUT BOOLEAN *InterruptState > ) > { > } > @@ -76,15 +77,16 @@ VmgInit ( > Performs the necessary steps to cleanup after invoking VMGEXIT. Must be > called after obtaining needed fields within the GHCB. > > - The base library function does nothing. > - > - @param[in, out] Ghcb A pointer to the GHCB > + @param[in, out] Ghcb A pointer to the GHCB > + @param[in] InterruptState An indicator to conditionally (re)enable > + interrupts > > **/ > VOID > EFIAPI > VmgDone ( > - IN OUT GHCB *Ghcb > + IN OUT GHCB *Ghcb, > + IN BOOLEAN InterruptState > ) > { > } > Reviewed-by: Laszlo Ersek