Re: [edk2-devel] [PATCH v3 10/11] UefiCpuPkg, OvmfPkg: Disable interrupts when using the GHCB

["Laszlo Ersek" <lersek@redhat.com>]

Ray, Eric, can one of you please ACK this patch as well, for the
UefiCpuPkg parts? I've reviewed the patch already (in full), but your
ACK would be appreciated, process-wise. I intend to merge the series on
Thursday at the latest (Nov 5th).

(Eric stated earlier that he was OK with modifying both UefiCpuPkg and
OvmfPkg: <https://edk2.groups.io/g/devel/message/66642>.)

Thank you.
Laszlo


On 10/29/20 15:18, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> 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 <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  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 07e8af6450b9..061948cf840d 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 0540df8a04d4..bc5cd61d751f 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 6d977d45bcdd..1f47ff3f73b5 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 b47e282aff82..89b065cb3ff3 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
>    )
>  {
>  }
>