[edk2-devel] [PATCH] OvmfPkg: Harden #VC instruction emulation somewhat (CVE-2024-25742)

[edk2-devel] [PATCH] OvmfPkg: Harden #VC instruction emulation somewhat (CVE-2024-25742)

[Adam Dunlap via groups.io]

Ensure that when a #VC exception happens, the instruction at the
instruction pointer matches the instruction that is expected given the
error code. This is to mitigate the ahoi WeSee attack [1] that could
allow hypervisors to breach integrity and confidentiality of the
firmware by maliciously injecting interrupts. This change is a
translated version of a linux patch e3ef461af35a ("x86/sev: Harden #VC
instruction emulation somewhat")

[1] https://ahoi-attacks.github.io/wesee/

Cc: Borislav Petkov (AMD) <bp@alien8.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Adam Dunlap <acdunlap@google.com>
---
 OvmfPkg/Library/CcExitLib/CcExitVcHandler.c | 171 ++++++++++++++++++--
 1 file changed, 160 insertions(+), 11 deletions(-)

diff --git a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
index 0fc30f7bc4..bd3e9f304a 100644
--- a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
+++ b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
@@ -532,8 +532,6 @@ MwaitExit (
   IN     CC_INSTRUCTION_DATA     *InstructionData
   )
 {
-  CcDecodeModRm (Regs, InstructionData);
-
   Ghcb->SaveArea.Rax = Regs->Rax;
   CcExitVmgSetOffsetValid (Ghcb, GhcbRax);
   Ghcb->SaveArea.Rcx = Regs->Rcx;
@@ -564,8 +562,6 @@ MonitorExit (
   IN     CC_INSTRUCTION_DATA     *InstructionData
   )
 {
-  CcDecodeModRm (Regs, InstructionData);
-
   Ghcb->SaveArea.Rax = Regs->Rax;  // Identity mapped, so VA = PA
   CcExitVmgSetOffsetValid (Ghcb, GhcbRax);
   Ghcb->SaveArea.Rcx = Regs->Rcx;
@@ -670,8 +666,6 @@ VmmCallExit (
 {
   UINT64  Status;
 
-  CcDecodeModRm (Regs, InstructionData);
-
   Ghcb->SaveArea.Rax = Regs->Rax;
   CcExitVmgSetOffsetValid (Ghcb, GhcbRax);
   Ghcb->SaveArea.Cpl = (UINT8)(Regs->Cs & 0x3);
@@ -1603,8 +1597,6 @@ Dr7WriteExit (
   Ext       = &InstructionData->Ext;
   SevEsData = (SEV_ES_PER_CPU_DATA *)(Ghcb + 1);
 
-  CcDecodeModRm (Regs, InstructionData);
-
   //
   // MOV DRn always treats MOD == 3 no matter how encoded
   //
@@ -1655,8 +1647,6 @@ Dr7ReadExit (
   Ext       = &InstructionData->Ext;
   SevEsData = (SEV_ES_PER_CPU_DATA *)(Ghcb + 1);
 
-  CcDecodeModRm (Regs, InstructionData);
-
   //
   // MOV DRn always treats MOD == 3 no matter how encoded
   //
@@ -1671,6 +1661,160 @@ Dr7ReadExit (
   return 0;
 }
 
+/**
+  Check that the opcode matches the exit code for a #VC.
+
+  Each exit code should only be raised while executing certain instructions.
+  Verify that rIP points to a correct instruction based on the exit code to
+  protect against maliciously injected interrupts via the hypervisor. If it does
+  not, report an unsupported event to the hypervisor.
+
+  Decodes the ModRm byte into InstructionData if necessary.
+
+  @param[in, out] Ghcb             Pointer to the Guest-Hypervisor Communication
+                                   Block
+  @param[in, out] Regs             x64 processor context
+  @param[in, out] InstructionData  Instruction parsing context
+  @param[in]      ExitCode         Exit code given by #VC.
+
+  @retval 0                        No problems detected.
+  @return                          New exception value to propagate
+
+
+**/
+STATIC
+UINT64
+VcCheckOpcodeBytes (
+  IN OUT GHCB                    *Ghcb,
+  IN OUT EFI_SYSTEM_CONTEXT_X64  *Regs,
+  IN OUT CC_INSTRUCTION_DATA     *InstructionData,
+  IN     UINT64                  ExitCode
+  )
+{
+  UINT8  OpCode;
+
+  //
+  // Expected opcodes are either 1 or 2 bytes. If they are 2 bytes, they always
+  // start with TWO_BYTE_OPCODE_ESCAPE (0x0f), so skip over that.
+  //
+  OpCode = *(InstructionData->OpCodes);
+  if (OpCode == TWO_BYTE_OPCODE_ESCAPE) {
+    OpCode = *(InstructionData->OpCodes + 1);
+  }
+
+  switch (ExitCode) {
+    case SVM_EXIT_IOIO_PROT:
+    case SVM_EXIT_NPF:
+      /* handled separately */
+      return 0;
+
+    case SVM_EXIT_CPUID:
+      if (OpCode == 0xa2) {
+        return 0;
+      }
+
+      break;
+
+    case SVM_EXIT_INVD:
+      break;
+
+    case SVM_EXIT_MONITOR:
+      CcDecodeModRm (Regs, InstructionData);
+
+      if ((OpCode == 0x01) && (InstructionData->ModRm.Uint8 == 0xc8)) {
+        return 0;
+      }
+
+      break;
+
+    case SVM_EXIT_MWAIT:
+      CcDecodeModRm (Regs, InstructionData);
+
+      if ((OpCode == 0x01) && (InstructionData->ModRm.Uint8 == 0xc9)) {
+        return 0;
+      }
+
+      break;
+
+    case SVM_EXIT_MSR:
+      /* RDMSR */
+      if ((OpCode == 0x32) ||
+          /* WRMSR */
+          (OpCode == 0x30))
+      {
+        return 0;
+      }
+
+      break;
+
+    case SVM_EXIT_RDPMC:
+      if (OpCode == 0x33) {
+        return 0;
+      }
+
+      break;
+
+    case SVM_EXIT_RDTSC:
+      if (OpCode == 0x31) {
+        return 0;
+      }
+
+      break;
+
+    case SVM_EXIT_RDTSCP:
+      CcDecodeModRm (Regs, InstructionData);
+
+      if ((OpCode == 0x01) && (InstructionData->ModRm.Uint8 == 0xf9)) {
+        return 0;
+      }
+
+      break;
+
+    case SVM_EXIT_DR7_READ:
+      CcDecodeModRm (Regs, InstructionData);
+
+      if ((OpCode == 0x21) &&
+          (InstructionData->Ext.ModRm.Reg == 7))
+      {
+        return 0;
+      }
+
+      break;
+
+    case SVM_EXIT_VMMCALL:
+      CcDecodeModRm (Regs, InstructionData);
+
+      if ((OpCode == 0x01) && (InstructionData->ModRm.Uint8 == 0xd9)) {
+        return 0;
+      }
+
+      break;
+
+    case SVM_EXIT_DR7_WRITE:
+      CcDecodeModRm (Regs, InstructionData);
+
+      if ((OpCode == 0x23) &&
+          (InstructionData->Ext.ModRm.Reg == 7))
+      {
+        return 0;
+      }
+
+      break;
+
+    case SVM_EXIT_WBINVD:
+      if (OpCode == 0x9) {
+        return 0;
+      }
+
+      break;
+
+    default:
+      break;
+  }
+
+  return UnsupportedExit (Ghcb, Regs, InstructionData);
+}
+
 /**
   Handle a #VC exception.
 
@@ -1773,7 +1917,12 @@ InternalVmgExitHandleVc (
 
   CcInitInstructionData (&InstructionData, Ghcb, Regs);
 
-  Status = NaeExit (Ghcb, Regs, &InstructionData);
+  Status = VcCheckOpcodeBytes (Ghcb, Regs, &InstructionData, ExitCode);
+
+  if (Status == 0) {
+    Status = NaeExit (Ghcb, Regs, &InstructionData);
+  }
+
   if (Status == 0) {
     Regs->Rip += CcInstructionLength (&InstructionData);
   } else {
-- 
2.44.0.683.g7961c838ac-goog



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117926): https://edk2.groups.io/g/devel/message/117926
Mute This Topic: https://groups.io/mt/105581633/7686176
Mute #vc:https://edk2.groups.io/g/devel/mutehashtag/vc
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] OvmfPkg: Harden #VC instruction emulation somewhat (CVE-2024-25742)

[Ard Biesheuvel]

(cc Jiewen)

Please cc the OVMF maintainers when you send edk2 patches. (There is a
Maintainers file in the root of the repo)


On Wed, 17 Apr 2024 at 18:54, Adam Dunlap via groups.io
<acdunlap=google.com@groups.io> wrote:
>
> Ensure that when a #VC exception happens, the instruction at the
> instruction pointer matches the instruction that is expected given the
> error code. This is to mitigate the ahoi WeSee attack [1] that could
> allow hypervisors to breach integrity and confidentiality of the
> firmware by maliciously injecting interrupts. This change is a
> translated version of a linux patch e3ef461af35a ("x86/sev: Harden #VC
> instruction emulation somewhat")
>
> [1] https://ahoi-attacks.github.io/wesee/
>
> Cc: Borislav Petkov (AMD) <bp@alien8.de>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Adam Dunlap <acdunlap@google.com>
> ---
>  OvmfPkg/Library/CcExitLib/CcExitVcHandler.c | 171 ++++++++++++++++++--
>  1 file changed, 160 insertions(+), 11 deletions(-)
>
> diff --git a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
> index 0fc30f7bc4..bd3e9f304a 100644
> --- a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
> +++ b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
> @@ -532,8 +532,6 @@ MwaitExit (
>    IN     CC_INSTRUCTION_DATA     *InstructionData
>    )
>  {
> -  CcDecodeModRm (Regs, InstructionData);
> -
>    Ghcb->SaveArea.Rax = Regs->Rax;
>    CcExitVmgSetOffsetValid (Ghcb, GhcbRax);
>    Ghcb->SaveArea.Rcx = Regs->Rcx;
> @@ -564,8 +562,6 @@ MonitorExit (
>    IN     CC_INSTRUCTION_DATA     *InstructionData
>    )
>  {
> -  CcDecodeModRm (Regs, InstructionData);
> -
>    Ghcb->SaveArea.Rax = Regs->Rax;  // Identity mapped, so VA = PA
>    CcExitVmgSetOffsetValid (Ghcb, GhcbRax);
>    Ghcb->SaveArea.Rcx = Regs->Rcx;
> @@ -670,8 +666,6 @@ VmmCallExit (
>  {
>    UINT64  Status;
>
> -  CcDecodeModRm (Regs, InstructionData);
> -
>    Ghcb->SaveArea.Rax = Regs->Rax;
>    CcExitVmgSetOffsetValid (Ghcb, GhcbRax);
>    Ghcb->SaveArea.Cpl = (UINT8)(Regs->Cs & 0x3);
> @@ -1603,8 +1597,6 @@ Dr7WriteExit (
>    Ext       = &InstructionData->Ext;
>    SevEsData = (SEV_ES_PER_CPU_DATA *)(Ghcb + 1);
>
> -  CcDecodeModRm (Regs, InstructionData);
> -
>    //
>    // MOV DRn always treats MOD == 3 no matter how encoded
>    //
> @@ -1655,8 +1647,6 @@ Dr7ReadExit (
>    Ext       = &InstructionData->Ext;
>    SevEsData = (SEV_ES_PER_CPU_DATA *)(Ghcb + 1);
>
> -  CcDecodeModRm (Regs, InstructionData);
> -
>    //
>    // MOV DRn always treats MOD == 3 no matter how encoded
>    //
> @@ -1671,6 +1661,160 @@ Dr7ReadExit (
>    return 0;
>  }
>
> +/**
> +  Check that the opcode matches the exit code for a #VC.
> +
> +  Each exit code should only be raised while executing certain instructions.
> +  Verify that rIP points to a correct instruction based on the exit code to
> +  protect against maliciously injected interrupts via the hypervisor. If it does
> +  not, report an unsupported event to the hypervisor.
> +
> +  Decodes the ModRm byte into InstructionData if necessary.
> +
> +  @param[in, out] Ghcb             Pointer to the Guest-Hypervisor Communication
> +                                   Block
> +  @param[in, out] Regs             x64 processor context
> +  @param[in, out] InstructionData  Instruction parsing context
> +  @param[in]      ExitCode         Exit code given by #VC.
> +
> +  @retval 0                        No problems detected.
> +  @return                          New exception value to propagate
> +
> +
> +**/
> +STATIC
> +UINT64
> +VcCheckOpcodeBytes (
> +  IN OUT GHCB                    *Ghcb,
> +  IN OUT EFI_SYSTEM_CONTEXT_X64  *Regs,
> +  IN OUT CC_INSTRUCTION_DATA     *InstructionData,
> +  IN     UINT64                  ExitCode
> +  )
> +{
> +  UINT8  OpCode;
> +
> +  //
> +  // Expected opcodes are either 1 or 2 bytes. If they are 2 bytes, they always
> +  // start with TWO_BYTE_OPCODE_ESCAPE (0x0f), so skip over that.
> +  //
> +  OpCode = *(InstructionData->OpCodes);
> +  if (OpCode == TWO_BYTE_OPCODE_ESCAPE) {
> +    OpCode = *(InstructionData->OpCodes + 1);
> +  }
> +
> +  switch (ExitCode) {
> +    case SVM_EXIT_IOIO_PROT:
> +    case SVM_EXIT_NPF:
> +      /* handled separately */
> +      return 0;
> +
> +    case SVM_EXIT_CPUID:
> +      if (OpCode == 0xa2) {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_INVD:
> +      break;
> +
> +    case SVM_EXIT_MONITOR:
> +      CcDecodeModRm (Regs, InstructionData);
> +
> +      if ((OpCode == 0x01) && (InstructionData->ModRm.Uint8 == 0xc8)) {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_MWAIT:
> +      CcDecodeModRm (Regs, InstructionData);
> +
> +      if ((OpCode == 0x01) && (InstructionData->ModRm.Uint8 == 0xc9)) {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_MSR:
> +      /* RDMSR */
> +      if ((OpCode == 0x32) ||
> +          /* WRMSR */
> +          (OpCode == 0x30))
> +      {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_RDPMC:
> +      if (OpCode == 0x33) {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_RDTSC:
> +      if (OpCode == 0x31) {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_RDTSCP:
> +      CcDecodeModRm (Regs, InstructionData);
> +
> +      if ((OpCode == 0x01) && (InstructionData->ModRm.Uint8 == 0xf9)) {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_DR7_READ:
> +      CcDecodeModRm (Regs, InstructionData);
> +
> +      if ((OpCode == 0x21) &&
> +          (InstructionData->Ext.ModRm.Reg == 7))
> +      {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_VMMCALL:
> +      CcDecodeModRm (Regs, InstructionData);
> +
> +      if ((OpCode == 0x01) && (InstructionData->ModRm.Uint8 == 0xd9)) {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_DR7_WRITE:
> +      CcDecodeModRm (Regs, InstructionData);
> +
> +      if ((OpCode == 0x23) &&
> +          (InstructionData->Ext.ModRm.Reg == 7))
> +      {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_WBINVD:
> +      if (OpCode == 0x9) {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    default:
> +      break;
> +  }
> +
> +  return UnsupportedExit (Ghcb, Regs, InstructionData);
> +}
> +
>  /**
>    Handle a #VC exception.
>
> @@ -1773,7 +1917,12 @@ InternalVmgExitHandleVc (
>
>    CcInitInstructionData (&InstructionData, Ghcb, Regs);
>
> -  Status = NaeExit (Ghcb, Regs, &InstructionData);
> +  Status = VcCheckOpcodeBytes (Ghcb, Regs, &InstructionData, ExitCode);
> +
> +  if (Status == 0) {
> +    Status = NaeExit (Ghcb, Regs, &InstructionData);
> +  }
> +

This looks a bit dodgy. First of all, I have a personal dislike of
this 'success handling' anti-pattern, but more importantly, it seems
like we are relying here on VcCheckOpcodeBytes() never returning on
failure, right? If so, that at least needs a comment.

>    if (Status == 0) {
>      Regs->Rip += CcInstructionLength (&InstructionData);
>    } else {
> --
> 2.44.0.683.g7961c838ac-goog
>
>
>
> 
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117927): https://edk2.groups.io/g/devel/message/117927
Mute This Topic: https://groups.io/mt/105581633/7686176
Mute #vc:https://edk2.groups.io/g/devel/mutehashtag/vc
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] OvmfPkg: Harden #VC instruction emulation somewhat (CVE-2024-25742)

[Adam Dunlap via groups.io]

On Wed, Apr 17, 2024 at 10:08 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> (cc Jiewen)
>
> Please cc the OVMF maintainers when you send edk2 patches. (There is a
> Maintainers file in the root of the repo)

Thanks, I added everyone returned from the GetMaintainer.py script.

> On Wed, 17 Apr 2024 at 18:54, Adam Dunlap via groups.io
> <acdunlap=google.com@groups.io> wrote:
> >
> > Ensure that when a #VC exception happens, the instruction at the
> > instruction pointer matches the instruction that is expected given the
> > error code. This is to mitigate the ahoi WeSee attack [1] that could
> > allow hypervisors to breach integrity and confidentiality of the
> > firmware by maliciously injecting interrupts. This change is a
> > translated version of a linux patch e3ef461af35a ("x86/sev: Harden #VC
> > instruction emulation somewhat")
> >
> > [1] https://ahoi-attacks.github.io/wesee/
> >
> > Cc: Borislav Petkov (AMD) <bp@alien8.de>
> > Cc: Tom Lendacky <thomas.lendacky@amd.com>
> > Signed-off-by: Adam Dunlap <acdunlap@google.com>
> > ---
> >  OvmfPkg/Library/CcExitLib/CcExitVcHandler.c | 171 ++++++++++++++++++--
> >  1 file changed, 160 insertions(+), 11 deletions(-)
> >
> > diff --git a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
> > index 0fc30f7bc4..bd3e9f304a 100644
> > --- a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
> > +++ b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
> > @@ -532,8 +532,6 @@ MwaitExit (
> >    IN     CC_INSTRUCTION_DATA     *InstructionData
> >    )
> >  {
> > -  CcDecodeModRm (Regs, InstructionData);
> > -
> >    Ghcb->SaveArea.Rax = Regs->Rax;
> >    CcExitVmgSetOffsetValid (Ghcb, GhcbRax);
> >    Ghcb->SaveArea.Rcx = Regs->Rcx;
> > @@ -564,8 +562,6 @@ MonitorExit (
> >    IN     CC_INSTRUCTION_DATA     *InstructionData
> >    )
> >  {
> > -  CcDecodeModRm (Regs, InstructionData);
> > -
> >    Ghcb->SaveArea.Rax = Regs->Rax;  // Identity mapped, so VA = PA
> >    CcExitVmgSetOffsetValid (Ghcb, GhcbRax);
> >    Ghcb->SaveArea.Rcx = Regs->Rcx;
> > @@ -670,8 +666,6 @@ VmmCallExit (
> >  {
> >    UINT64  Status;
> >
> > -  CcDecodeModRm (Regs, InstructionData);
> > -
> >    Ghcb->SaveArea.Rax = Regs->Rax;
> >    CcExitVmgSetOffsetValid (Ghcb, GhcbRax);
> >    Ghcb->SaveArea.Cpl = (UINT8)(Regs->Cs & 0x3);
> > @@ -1603,8 +1597,6 @@ Dr7WriteExit (
> >    Ext       = &InstructionData->Ext;
> >    SevEsData = (SEV_ES_PER_CPU_DATA *)(Ghcb + 1);
> >
> > -  CcDecodeModRm (Regs, InstructionData);
> > -
> >    //
> >    // MOV DRn always treats MOD == 3 no matter how encoded
> >    //
> > @@ -1655,8 +1647,6 @@ Dr7ReadExit (
> >    Ext       = &InstructionData->Ext;
> >    SevEsData = (SEV_ES_PER_CPU_DATA *)(Ghcb + 1);
> >
> > -  CcDecodeModRm (Regs, InstructionData);
> > -
> >    //
> >    // MOV DRn always treats MOD == 3 no matter how encoded
> >    //
> > @@ -1671,6 +1661,160 @@ Dr7ReadExit (
> >    return 0;
> >  }
> >
> > +/**
> > +  Check that the opcode matches the exit code for a #VC.
> > +
> > +  Each exit code should only be raised while executing certain instructions.
> > +  Verify that rIP points to a correct instruction based on the exit code to
> > +  protect against maliciously injected interrupts via the hypervisor. If it does
> > +  not, report an unsupported event to the hypervisor.
> > +
> > +  Decodes the ModRm byte into InstructionData if necessary.
> > +
> > +  @param[in, out] Ghcb             Pointer to the Guest-Hypervisor Communication
> > +                                   Block
> > +  @param[in, out] Regs             x64 processor context
> > +  @param[in, out] InstructionData  Instruction parsing context
> > +  @param[in]      ExitCode         Exit code given by #VC.
> > +
> > +  @retval 0                        No problems detected.
> > +  @return                          New exception value to propagate
> > +
> > +
> > +**/
> > +STATIC
> > +UINT64
> > +VcCheckOpcodeBytes (
> > +  IN OUT GHCB                    *Ghcb,
> > +  IN OUT EFI_SYSTEM_CONTEXT_X64  *Regs,
> > +  IN OUT CC_INSTRUCTION_DATA     *InstructionData,
> > +  IN     UINT64                  ExitCode
> > +  )
> > +{
> > +  UINT8  OpCode;
> > +
> > +  //
> > +  // Expected opcodes are either 1 or 2 bytes. If they are 2 bytes, they always
> > +  // start with TWO_BYTE_OPCODE_ESCAPE (0x0f), so skip over that.
> > +  //
> > +  OpCode = *(InstructionData->OpCodes);
> > +  if (OpCode == TWO_BYTE_OPCODE_ESCAPE) {
> > +    OpCode = *(InstructionData->OpCodes + 1);
> > +  }
> > +
> > +  switch (ExitCode) {
> > +    case SVM_EXIT_IOIO_PROT:
> > +    case SVM_EXIT_NPF:
> > +      /* handled separately */
> > +      return 0;
> > +
> > +    case SVM_EXIT_CPUID:
> > +      if (OpCode == 0xa2) {
> > +        return 0;
> > +      }
> > +
> > +      break;
> > +
> > +    case SVM_EXIT_INVD:
> > +      break;
> > +
> > +    case SVM_EXIT_MONITOR:
> > +      CcDecodeModRm (Regs, InstructionData);
> > +
> > +      if ((OpCode == 0x01) && (InstructionData->ModRm.Uint8 == 0xc8)) {
> > +        return 0;
> > +      }
> > +
> > +      break;
> > +
> > +    case SVM_EXIT_MWAIT:
> > +      CcDecodeModRm (Regs, InstructionData);
> > +
> > +      if ((OpCode == 0x01) && (InstructionData->ModRm.Uint8 == 0xc9)) {
> > +        return 0;
> > +      }
> > +
> > +      break;
> > +
> > +    case SVM_EXIT_MSR:
> > +      /* RDMSR */
> > +      if ((OpCode == 0x32) ||
> > +          /* WRMSR */
> > +          (OpCode == 0x30))
> > +      {
> > +        return 0;
> > +      }
> > +
> > +      break;
> > +
> > +    case SVM_EXIT_RDPMC:
> > +      if (OpCode == 0x33) {
> > +        return 0;
> > +      }
> > +
> > +      break;
> > +
> > +    case SVM_EXIT_RDTSC:
> > +      if (OpCode == 0x31) {
> > +        return 0;
> > +      }
> > +
> > +      break;
> > +
> > +    case SVM_EXIT_RDTSCP:
> > +      CcDecodeModRm (Regs, InstructionData);
> > +
> > +      if ((OpCode == 0x01) && (InstructionData->ModRm.Uint8 == 0xf9)) {
> > +        return 0;
> > +      }
> > +
> > +      break;
> > +
> > +    case SVM_EXIT_DR7_READ:
> > +      CcDecodeModRm (Regs, InstructionData);
> > +
> > +      if ((OpCode == 0x21) &&
> > +          (InstructionData->Ext.ModRm.Reg == 7))
> > +      {
> > +        return 0;
> > +      }
> > +
> > +      break;
> > +
> > +    case SVM_EXIT_VMMCALL:
> > +      CcDecodeModRm (Regs, InstructionData);
> > +
> > +      if ((OpCode == 0x01) && (InstructionData->ModRm.Uint8 == 0xd9)) {
> > +        return 0;
> > +      }
> > +
> > +      break;
> > +
> > +    case SVM_EXIT_DR7_WRITE:
> > +      CcDecodeModRm (Regs, InstructionData);
> > +
> > +      if ((OpCode == 0x23) &&
> > +          (InstructionData->Ext.ModRm.Reg == 7))
> > +      {
> > +        return 0;
> > +      }
> > +
> > +      break;
> > +
> > +    case SVM_EXIT_WBINVD:
> > +      if (OpCode == 0x9) {
> > +        return 0;
> > +      }
> > +
> > +      break;
> > +
> > +    default:
> > +      break;
> > +  }
> > +
> > +  return UnsupportedExit (Ghcb, Regs, InstructionData);
> > +}
> > +
> >  /**
> >    Handle a #VC exception.
> >
> > @@ -1773,7 +1917,12 @@ InternalVmgExitHandleVc (
> >
> >    CcInitInstructionData (&InstructionData, Ghcb, Regs);
> >
> > -  Status = NaeExit (Ghcb, Regs, &InstructionData);
> > +  Status = VcCheckOpcodeBytes (Ghcb, Regs, &InstructionData, ExitCode);
> > +
> > +  if (Status == 0) {
> > +    Status = NaeExit (Ghcb, Regs, &InstructionData);
> > +  }
> > +
>
> This looks a bit dodgy. First of all, I have a personal dislike of
> this 'success handling' anti-pattern, but more importantly, it seems
> like we are relying here on VcCheckOpcodeBytes() never returning on
> failure, right? If so, that at least needs a comment.
>

If VcCheckOpcodeBytes() returns failure, that means it thinks that the #VC was
invalid/injected maliciously and that the guest should abort. From reading the
code in this file, it looks like calling UnsupportedExit() and returning its
return value is the standard way of doing this. If UnsupportedExit() doesn't
abort and instead returns normally for whatever reason, it will just ignore the
exception which seems like acceptable behavior. Maybe add a comment like

/* If the opcode does not match the exit code, do not process the exception */

If we could ensure that UnsupportedExit() always diverged (i.e. never returned)
then the code could be a bit simpler since it wouldn't need to handle error
cases.

> >    if (Status == 0) {
> >      Regs->Rip += CcInstructionLength (&InstructionData);
> >    } else {
> > --
> > 2.44.0.683.g7961c838ac-goog
> >
> >
> >
> > 
> >
> >


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117928): https://edk2.groups.io/g/devel/message/117928
Mute This Topic: https://groups.io/mt/105581633/7686176
Mute #vc:https://edk2.groups.io/g/devel/mutehashtag/vc
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] OvmfPkg: Harden #VC instruction emulation somewhat (CVE-2024-25742)

[Yao, Jiewen]

Thanks Adam and Ard.

Since this #VC specific hardening, I would rely on AMD people's expertise to fix it.
I have no objection for the patch.

Thank you
Yao, Jiewen

> -----Original Message-----
> From: Adam Dunlap <acdunlap@google.com>
> Sent: Thursday, April 18, 2024 1:45 AM
> To: Ard Biesheuvel <ardb@kernel.org>
> Cc: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>; Borislav Petkov
> <bp@alien8.de>; Peter Gonda <pgonda@google.com>; Tom Lendacky
> <thomas.lendacky@amd.com>; Aktas, Erdem <erdemaktas@google.com>; Gerd
> Hoffmann <kraxel@redhat.com>; Michael Roth <michael.roth@amd.com>; Xu,
> Min M <min.m.xu@intel.com>
> Subject: Re: [edk2-devel] [PATCH] OvmfPkg: Harden #VC instruction emulation
> somewhat (CVE-2024-25742)
> 
> On Wed, Apr 17, 2024 at 10:08 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > (cc Jiewen)
> >
> > Please cc the OVMF maintainers when you send edk2 patches. (There is a
> > Maintainers file in the root of the repo)
> 
> Thanks, I added everyone returned from the GetMaintainer.py script.
> 
> > On Wed, 17 Apr 2024 at 18:54, Adam Dunlap via groups.io
> > <acdunlap=google.com@groups.io> wrote:
> > >
> > > Ensure that when a #VC exception happens, the instruction at the
> > > instruction pointer matches the instruction that is expected given the
> > > error code. This is to mitigate the ahoi WeSee attack [1] that could
> > > allow hypervisors to breach integrity and confidentiality of the
> > > firmware by maliciously injecting interrupts. This change is a
> > > translated version of a linux patch e3ef461af35a ("x86/sev: Harden #VC
> > > instruction emulation somewhat")
> > >
> > > [1] https://ahoi-attacks.github.io/wesee/
> > >
> > > Cc: Borislav Petkov (AMD) <bp@alien8.de>
> > > Cc: Tom Lendacky <thomas.lendacky@amd.com>
> > > Signed-off-by: Adam Dunlap <acdunlap@google.com>
> > > ---
> > >  OvmfPkg/Library/CcExitLib/CcExitVcHandler.c | 171 ++++++++++++++++++--
> > >  1 file changed, 160 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
> b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
> > > index 0fc30f7bc4..bd3e9f304a 100644
> > > --- a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
> > > +++ b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
> > > @@ -532,8 +532,6 @@ MwaitExit (
> > >    IN     CC_INSTRUCTION_DATA     *InstructionData
> > >    )
> > >  {
> > > -  CcDecodeModRm (Regs, InstructionData);
> > > -
> > >    Ghcb->SaveArea.Rax = Regs->Rax;
> > >    CcExitVmgSetOffsetValid (Ghcb, GhcbRax);
> > >    Ghcb->SaveArea.Rcx = Regs->Rcx;
> > > @@ -564,8 +562,6 @@ MonitorExit (
> > >    IN     CC_INSTRUCTION_DATA     *InstructionData
> > >    )
> > >  {
> > > -  CcDecodeModRm (Regs, InstructionData);
> > > -
> > >    Ghcb->SaveArea.Rax = Regs->Rax;  // Identity mapped, so VA = PA
> > >    CcExitVmgSetOffsetValid (Ghcb, GhcbRax);
> > >    Ghcb->SaveArea.Rcx = Regs->Rcx;
> > > @@ -670,8 +666,6 @@ VmmCallExit (
> > >  {
> > >    UINT64  Status;
> > >
> > > -  CcDecodeModRm (Regs, InstructionData);
> > > -
> > >    Ghcb->SaveArea.Rax = Regs->Rax;
> > >    CcExitVmgSetOffsetValid (Ghcb, GhcbRax);
> > >    Ghcb->SaveArea.Cpl = (UINT8)(Regs->Cs & 0x3);
> > > @@ -1603,8 +1597,6 @@ Dr7WriteExit (
> > >    Ext       = &InstructionData->Ext;
> > >    SevEsData = (SEV_ES_PER_CPU_DATA *)(Ghcb + 1);
> > >
> > > -  CcDecodeModRm (Regs, InstructionData);
> > > -
> > >    //
> > >    // MOV DRn always treats MOD == 3 no matter how encoded
> > >    //
> > > @@ -1655,8 +1647,6 @@ Dr7ReadExit (
> > >    Ext       = &InstructionData->Ext;
> > >    SevEsData = (SEV_ES_PER_CPU_DATA *)(Ghcb + 1);
> > >
> > > -  CcDecodeModRm (Regs, InstructionData);
> > > -
> > >    //
> > >    // MOV DRn always treats MOD == 3 no matter how encoded
> > >    //
> > > @@ -1671,6 +1661,160 @@ Dr7ReadExit (
> > >    return 0;
> > >  }
> > >
> > > +/**
> > > +  Check that the opcode matches the exit code for a #VC.
> > > +
> > > +  Each exit code should only be raised while executing certain instructions.
> > > +  Verify that rIP points to a correct instruction based on the exit code to
> > > +  protect against maliciously injected interrupts via the hypervisor. If it does
> > > +  not, report an unsupported event to the hypervisor.
> > > +
> > > +  Decodes the ModRm byte into InstructionData if necessary.
> > > +
> > > +  @param[in, out] Ghcb             Pointer to the Guest-Hypervisor
> Communication
> > > +                                   Block
> > > +  @param[in, out] Regs             x64 processor context
> > > +  @param[in, out] InstructionData  Instruction parsing context
> > > +  @param[in]      ExitCode         Exit code given by #VC.
> > > +
> > > +  @retval 0                        No problems detected.
> > > +  @return                          New exception value to propagate
> > > +
> > > +
> > > +**/
> > > +STATIC
> > > +UINT64
> > > +VcCheckOpcodeBytes (
> > > +  IN OUT GHCB                    *Ghcb,
> > > +  IN OUT EFI_SYSTEM_CONTEXT_X64  *Regs,
> > > +  IN OUT CC_INSTRUCTION_DATA     *InstructionData,
> > > +  IN     UINT64                  ExitCode
> > > +  )
> > > +{
> > > +  UINT8  OpCode;
> > > +
> > > +  //
> > > +  // Expected opcodes are either 1 or 2 bytes. If they are 2 bytes, they always
> > > +  // start with TWO_BYTE_OPCODE_ESCAPE (0x0f), so skip over that.
> > > +  //
> > > +  OpCode = *(InstructionData->OpCodes);
> > > +  if (OpCode == TWO_BYTE_OPCODE_ESCAPE) {
> > > +    OpCode = *(InstructionData->OpCodes + 1);
> > > +  }
> > > +
> > > +  switch (ExitCode) {
> > > +    case SVM_EXIT_IOIO_PROT:
> > > +    case SVM_EXIT_NPF:
> > > +      /* handled separately */
> > > +      return 0;
> > > +
> > > +    case SVM_EXIT_CPUID:
> > > +      if (OpCode == 0xa2) {
> > > +        return 0;
> > > +      }
> > > +
> > > +      break;
> > > +
> > > +    case SVM_EXIT_INVD:
> > > +      break;
> > > +
> > > +    case SVM_EXIT_MONITOR:
> > > +      CcDecodeModRm (Regs, InstructionData);
> > > +
> > > +      if ((OpCode == 0x01) && (InstructionData->ModRm.Uint8 == 0xc8)) {
> > > +        return 0;
> > > +      }
> > > +
> > > +      break;
> > > +
> > > +    case SVM_EXIT_MWAIT:
> > > +      CcDecodeModRm (Regs, InstructionData);
> > > +
> > > +      if ((OpCode == 0x01) && (InstructionData->ModRm.Uint8 == 0xc9)) {
> > > +        return 0;
> > > +      }
> > > +
> > > +      break;
> > > +
> > > +    case SVM_EXIT_MSR:
> > > +      /* RDMSR */
> > > +      if ((OpCode == 0x32) ||
> > > +          /* WRMSR */
> > > +          (OpCode == 0x30))
> > > +      {
> > > +        return 0;
> > > +      }
> > > +
> > > +      break;
> > > +
> > > +    case SVM_EXIT_RDPMC:
> > > +      if (OpCode == 0x33) {
> > > +        return 0;
> > > +      }
> > > +
> > > +      break;
> > > +
> > > +    case SVM_EXIT_RDTSC:
> > > +      if (OpCode == 0x31) {
> > > +        return 0;
> > > +      }
> > > +
> > > +      break;
> > > +
> > > +    case SVM_EXIT_RDTSCP:
> > > +      CcDecodeModRm (Regs, InstructionData);
> > > +
> > > +      if ((OpCode == 0x01) && (InstructionData->ModRm.Uint8 == 0xf9)) {
> > > +        return 0;
> > > +      }
> > > +
> > > +      break;
> > > +
> > > +    case SVM_EXIT_DR7_READ:
> > > +      CcDecodeModRm (Regs, InstructionData);
> > > +
> > > +      if ((OpCode == 0x21) &&
> > > +          (InstructionData->Ext.ModRm.Reg == 7))
> > > +      {
> > > +        return 0;
> > > +      }
> > > +
> > > +      break;
> > > +
> > > +    case SVM_EXIT_VMMCALL:
> > > +      CcDecodeModRm (Regs, InstructionData);
> > > +
> > > +      if ((OpCode == 0x01) && (InstructionData->ModRm.Uint8 == 0xd9)) {
> > > +        return 0;
> > > +      }
> > > +
> > > +      break;
> > > +
> > > +    case SVM_EXIT_DR7_WRITE:
> > > +      CcDecodeModRm (Regs, InstructionData);
> > > +
> > > +      if ((OpCode == 0x23) &&
> > > +          (InstructionData->Ext.ModRm.Reg == 7))
> > > +      {
> > > +        return 0;
> > > +      }
> > > +
> > > +      break;
> > > +
> > > +    case SVM_EXIT_WBINVD:
> > > +      if (OpCode == 0x9) {
> > > +        return 0;
> > > +      }
> > > +
> > > +      break;
> > > +
> > > +    default:
> > > +      break;
> > > +  }
> > > +
> > > +  return UnsupportedExit (Ghcb, Regs, InstructionData);
> > > +}
> > > +
> > >  /**
> > >    Handle a #VC exception.
> > >
> > > @@ -1773,7 +1917,12 @@ InternalVmgExitHandleVc (
> > >
> > >    CcInitInstructionData (&InstructionData, Ghcb, Regs);
> > >
> > > -  Status = NaeExit (Ghcb, Regs, &InstructionData);
> > > +  Status = VcCheckOpcodeBytes (Ghcb, Regs, &InstructionData, ExitCode);
> > > +
> > > +  if (Status == 0) {
> > > +    Status = NaeExit (Ghcb, Regs, &InstructionData);
> > > +  }
> > > +
> >
> > This looks a bit dodgy. First of all, I have a personal dislike of
> > this 'success handling' anti-pattern, but more importantly, it seems
> > like we are relying here on VcCheckOpcodeBytes() never returning on
> > failure, right? If so, that at least needs a comment.
> >
> 
> If VcCheckOpcodeBytes() returns failure, that means it thinks that the #VC was
> invalid/injected maliciously and that the guest should abort. From reading the
> code in this file, it looks like calling UnsupportedExit() and returning its
> return value is the standard way of doing this. If UnsupportedExit() doesn't
> abort and instead returns normally for whatever reason, it will just ignore the
> exception which seems like acceptable behavior. Maybe add a comment like
> 
> /* If the opcode does not match the exit code, do not process the exception */
> 
> If we could ensure that UnsupportedExit() always diverged (i.e. never returned)
> then the code could be a bit simpler since it wouldn't need to handle error
> cases.
> 
> > >    if (Status == 0) {
> > >      Regs->Rip += CcInstructionLength (&InstructionData);
> > >    } else {
> > > --
> > > 2.44.0.683.g7961c838ac-goog
> > >
> > >
> > >
> > > 
> > >
> > >


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117966): https://edk2.groups.io/g/devel/message/117966
Mute This Topic: https://groups.io/mt/105581633/7686176
Mute #vc:https://edk2.groups.io/g/devel/mutehashtag/vc
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] OvmfPkg: Harden #VC instruction emulation somewhat (CVE-2024-25742)

[Gerd Hoffmann]

On Wed, Apr 17, 2024 at 09:54:00AM -0700, Adam Dunlap via groups.io wrote:
> Ensure that when a #VC exception happens, the instruction at the
> instruction pointer matches the instruction that is expected given the
> error code. This is to mitigate the ahoi WeSee attack [1] that could
> allow hypervisors to breach integrity and confidentiality of the
> firmware by maliciously injecting interrupts. This change is a
> translated version of a linux patch e3ef461af35a ("x86/sev: Harden #VC
> instruction emulation somewhat")

> +**/
> +STATIC
> +UINT64
> +VcCheckOpcodeBytes (
> +  IN OUT GHCB                    *Ghcb,
> +  IN OUT EFI_SYSTEM_CONTEXT_X64  *Regs,
> +  IN OUT CC_INSTRUCTION_DATA     *InstructionData,
> +  IN     UINT64                  ExitCode
> +  )
> +{
> +  UINT8  OpCode;

The linux kernel patch uses "unsigned int opcode" and apparently
checks more than just the first byte for multi-byte opcodes.  Why
do it differently here?

On the bigger picture:  I'm wondering why SNP allows external #VC
injections in the first place?

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117985): https://edk2.groups.io/g/devel/message/117985
Mute This Topic: https://groups.io/mt/105581633/7686176
Mute #vc:https://edk2.groups.io/g/devel/mutehashtag/vc
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] OvmfPkg: Harden #VC instruction emulation somewhat (CVE-2024-25742)

[Adam Dunlap via groups.io]

On Thu, Apr 18, 2024 at 5:15 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Wed, Apr 17, 2024 at 09:54:00AM -0700, Adam Dunlap via groups.io wrote:
> > +  UINT8  OpCode;
>
> The linux kernel patch uses "unsigned int opcode" and apparently
> checks more than just the first byte for multi-byte opcodes.  Why
> do it differently here?

Good question. This patch does check for two-byte opcodes with this snippet:

+  OpCode = *(InstructionData->OpCodes);
+  if (OpCode == TWO_BYTE_OPCODE_ESCAPE) {
+    OpCode = *(InstructionData->OpCodes + 1);
+  }

This works because the first byte of two-byte opcodes is always 0x0f in the
cases that we're checking for. I was wary about blindly dereferencing two
bytes since that could cause a page fault if it was actually a 1 byte opcode
that was at the very end of an allocated region. This is also what is done in
the MmioExit function in this file. The linux kernel instruction decoder is much
more extensive than what is done here and I didn't want to duplicate the
whole thing.

> On the bigger picture:  I'm wondering why SNP allows external #VC
> injections in the first place?

Yup, I think it'd be better if it didn't.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117995): https://edk2.groups.io/g/devel/message/117995
Mute This Topic: https://groups.io/mt/105581633/7686176
Mute #vc:https://edk2.groups.io/g/devel/mutehashtag/vc
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] OvmfPkg: Harden #VC instruction emulation somewhat (CVE-2024-25742)

[Peter Gonda via groups.io]

On Thu, Apr 18, 2024 at 9:39 AM Adam Dunlap <acdunlap@google.com> wrote:
>
> On Thu, Apr 18, 2024 at 5:15 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > On Wed, Apr 17, 2024 at 09:54:00AM -0700, Adam Dunlap via groups.io wrote:
> > > +  UINT8  OpCode;
> >
> > The linux kernel patch uses "unsigned int opcode" and apparently
> > checks more than just the first byte for multi-byte opcodes.  Why
> > do it differently here?
>
> Good question. This patch does check for two-byte opcodes with this snippet:
>
> +  OpCode = *(InstructionData->OpCodes);
> +  if (OpCode == TWO_BYTE_OPCODE_ESCAPE) {
> +    OpCode = *(InstructionData->OpCodes + 1);
> +  }
>
> This works because the first byte of two-byte opcodes is always 0x0f in the
> cases that we're checking for. I was wary about blindly dereferencing two
> bytes since that could cause a page fault if it was actually a 1 byte opcode
> that was at the very end of an allocated region. This is also what is done in
> the MmioExit function in this file. The linux kernel instruction decoder is much
> more extensive than what is done here and I didn't want to duplicate the
> whole thing.
>
> > On the bigger picture:  I'm wondering why SNP allows external #VC
> > injections in the first place?
>
> Yup, I think it'd be better if it didn't.

I think this is a small mitigation until linux + edk2 guest's support
restricted or alternate interrupt injection. I suggested Adam send
this just to have parity between edk2 and linux.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117996): https://edk2.groups.io/g/devel/message/117996
Mute This Topic: https://groups.io/mt/105581633/7686176
Mute #vc:https://edk2.groups.io/g/devel/mutehashtag/vc
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] OvmfPkg: Harden #VC instruction emulation somewhat (CVE-2024-25742)

[Gerd Hoffmann]

On Thu, Apr 18, 2024 at 08:39:20AM -0700, Adam Dunlap wrote:
> On Thu, Apr 18, 2024 at 5:15 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > On Wed, Apr 17, 2024 at 09:54:00AM -0700, Adam Dunlap via groups.io wrote:
> > > +  UINT8  OpCode;
> >
> > The linux kernel patch uses "unsigned int opcode" and apparently
> > checks more than just the first byte for multi-byte opcodes.  Why
> > do it differently here?
> 
> Good question. This patch does check for two-byte opcodes with this snippet:
> 
> +  OpCode = *(InstructionData->OpCodes);
> +  if (OpCode == TWO_BYTE_OPCODE_ESCAPE) {
> +    OpCode = *(InstructionData->OpCodes + 1);
> +  }
> 
> This works because the first byte of two-byte opcodes is always 0x0f in the
> cases that we're checking for.

Ok, missed that little detail.  Thanks for explaining.

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118018): https://edk2.groups.io/g/devel/message/118018
Mute This Topic: https://groups.io/mt/105581633/7686176
Mute #vc:https://edk2.groups.io/g/devel/mutehashtag/vc
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] OvmfPkg: Harden #VC instruction emulation somewhat (CVE-2024-25742)

[Lendacky, Thomas via groups.io]

On 4/18/24 07:15, Gerd Hoffmann via groups.io wrote:
> On Wed, Apr 17, 2024 at 09:54:00AM -0700, Adam Dunlap via groups.io wrote:
>> Ensure that when a #VC exception happens, the instruction at the
>> instruction pointer matches the instruction that is expected given the
>> error code. This is to mitigate the ahoi WeSee attack [1] that could
>> allow hypervisors to breach integrity and confidentiality of the
>> firmware by maliciously injecting interrupts. This change is a
>> translated version of a linux patch e3ef461af35a ("x86/sev: Harden #VC
>> instruction emulation somewhat")
> 
>> +**/
>> +STATIC
>> +UINT64
>> +VcCheckOpcodeBytes (
>> +  IN OUT GHCB                    *Ghcb,
>> +  IN OUT EFI_SYSTEM_CONTEXT_X64  *Regs,
>> +  IN OUT CC_INSTRUCTION_DATA     *InstructionData,
>> +  IN     UINT64                  ExitCode
>> +  )
>> +{
>> +  UINT8  OpCode;
> 
> The linux kernel patch uses "unsigned int opcode" and apparently
> checks more than just the first byte for multi-byte opcodes.  Why
> do it differently here?
> 
> On the bigger picture:  I'm wondering why SNP allows external #VC
> injections in the first place?

It does and it doesn't. It doesn't allow #VC when injected as an 
exception. But the case of #VC injected as an interrupt was missed (see 
the event injection type field). It will be fixed in hardware going 
forward, but for now...

Thanks,
Tom

> 
> take care,
>    Gerd
> 
> 
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118030): https://edk2.groups.io/g/devel/message/118030
Mute This Topic: https://groups.io/mt/105581633/7686176
Mute #vc:https://edk2.groups.io/g/devel/mutehashtag/vc
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] OvmfPkg: Harden #VC instruction emulation somewhat (CVE-2024-25742)

[Lendacky, Thomas via groups.io]

On 4/17/24 11:54, Adam Dunlap wrote:
> Ensure that when a #VC exception happens, the instruction at the
> instruction pointer matches the instruction that is expected given the
> error code. This is to mitigate the ahoi WeSee attack [1] that could
> allow hypervisors to breach integrity and confidentiality of the
> firmware by maliciously injecting interrupts. This change is a
> translated version of a linux patch e3ef461af35a ("x86/sev: Harden #VC
> instruction emulation somewhat")
> 
> [1] https://ahoi-attacks.github.io/wesee/
> 
> Cc: Borislav Petkov (AMD) <bp@alien8.de>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Adam Dunlap <acdunlap@google.com>
> ---
>   OvmfPkg/Library/CcExitLib/CcExitVcHandler.c | 171 ++++++++++++++++++--
>   1 file changed, 160 insertions(+), 11 deletions(-)
> 
> diff --git a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
> index 0fc30f7bc4..bd3e9f304a 100644
> --- a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
> +++ b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
> @@ -532,8 +532,6 @@ MwaitExit (
>     IN     CC_INSTRUCTION_DATA     *InstructionData
>     )
>   {
> -  CcDecodeModRm (Regs, InstructionData);
> -
>     Ghcb->SaveArea.Rax = Regs->Rax;
>     CcExitVmgSetOffsetValid (Ghcb, GhcbRax);
>     Ghcb->SaveArea.Rcx = Regs->Rcx;
> @@ -564,8 +562,6 @@ MonitorExit (
>     IN     CC_INSTRUCTION_DATA     *InstructionData
>     )
>   {
> -  CcDecodeModRm (Regs, InstructionData);
> -
>     Ghcb->SaveArea.Rax = Regs->Rax;  // Identity mapped, so VA = PA
>     CcExitVmgSetOffsetValid (Ghcb, GhcbRax);
>     Ghcb->SaveArea.Rcx = Regs->Rcx;
> @@ -670,8 +666,6 @@ VmmCallExit (
>   {
>     UINT64  Status;
>   
> -  CcDecodeModRm (Regs, InstructionData);
> -
>     Ghcb->SaveArea.Rax = Regs->Rax;
>     CcExitVmgSetOffsetValid (Ghcb, GhcbRax);
>     Ghcb->SaveArea.Cpl = (UINT8)(Regs->Cs & 0x3);
> @@ -1603,8 +1597,6 @@ Dr7WriteExit (
>     Ext       = &InstructionData->Ext;
>     SevEsData = (SEV_ES_PER_CPU_DATA *)(Ghcb + 1);
>   
> -  CcDecodeModRm (Regs, InstructionData);
> -
>     //
>     // MOV DRn always treats MOD == 3 no matter how encoded
>     //
> @@ -1655,8 +1647,6 @@ Dr7ReadExit (
>     Ext       = &InstructionData->Ext;
>     SevEsData = (SEV_ES_PER_CPU_DATA *)(Ghcb + 1);
>   
> -  CcDecodeModRm (Regs, InstructionData);
> -
>     //
>     // MOV DRn always treats MOD == 3 no matter how encoded
>     //
> @@ -1671,6 +1661,160 @@ Dr7ReadExit (
>     return 0;
>   }
>   
> +/**
> +  Check that the opcode matches the exit code for a #VC.
> +
> +  Each exit code should only be raised while executing certain instructions.
> +  Verify that rIP points to a correct instruction based on the exit code to
> +  protect against maliciously injected interrupts via the hypervisor. If it does
> +  not, report an unsupported event to the hypervisor.
> +
> +  Decodes the ModRm byte into InstructionData if necessary.
> +
> +  @param[in, out] Ghcb             Pointer to the Guest-Hypervisor Communication
> +                                   Block
> +  @param[in, out] Regs             x64 processor context
> +  @param[in, out] InstructionData  Instruction parsing context
> +  @param[in]      ExitCode         Exit code given by #VC.
> +
> +  @retval 0                        No problems detected.
> +  @return                          New exception value to propagate
> +
> +
> +**/
> +STATIC
> +UINT64
> +VcCheckOpcodeBytes (
> +  IN OUT GHCB                    *Ghcb,
> +  IN OUT EFI_SYSTEM_CONTEXT_X64  *Regs,
> +  IN OUT CC_INSTRUCTION_DATA     *InstructionData,
> +  IN     UINT64                  ExitCode
> +  )
> +{
> +  UINT8  OpCode;
> +
> +  //
> +  // Expected opcodes are either 1 or 2 bytes. If they are 2 bytes, they always
> +  // start with TWO_BYTE_OPCODE_ESCAPE (0x0f), so skip over that.
> +  //
> +  OpCode = *(InstructionData->OpCodes);
> +  if (OpCode == TWO_BYTE_OPCODE_ESCAPE) {
> +    OpCode = *(InstructionData->OpCodes + 1);
> +  }
> +
> +  switch (ExitCode) {
> +    case SVM_EXIT_IOIO_PROT:
> +    case SVM_EXIT_NPF:
> +      /* handled separately */
> +      return 0;
> +
> +    case SVM_EXIT_CPUID:
> +      if (OpCode == 0xa2) {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_INVD:
> +      break;

This changes the current behavior today, but I'm ok with that.

> +
> +    case SVM_EXIT_MONITOR:
> +      CcDecodeModRm (Regs, InstructionData);
> +
> +      if ((OpCode == 0x01) && (InstructionData->ModRm.Uint8 == 0xc8)) {

This should also handle the MONITORX opcode (hmmm... I need to send a 
patch to the kernel).

> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_MWAIT:
> +      CcDecodeModRm (Regs, InstructionData);
> +
> +      if ((OpCode == 0x01) && (InstructionData->ModRm.Uint8 == 0xc9)) {

Same here for MWAITX.

Thanks,
Tom

> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_MSR:
> +      /* RDMSR */
> +      if ((OpCode == 0x32) ||
> +          /* WRMSR */
> +          (OpCode == 0x30))
> +      {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_RDPMC:
> +      if (OpCode == 0x33) {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_RDTSC:
> +      if (OpCode == 0x31) {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_RDTSCP:
> +      CcDecodeModRm (Regs, InstructionData);
> +
> +      if ((OpCode == 0x01) && (InstructionData->ModRm.Uint8 == 0xf9)) {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_DR7_READ:
> +      CcDecodeModRm (Regs, InstructionData);
> +
> +      if ((OpCode == 0x21) &&
> +          (InstructionData->Ext.ModRm.Reg == 7))
> +      {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_VMMCALL:
> +      CcDecodeModRm (Regs, InstructionData);
> +
> +      if ((OpCode == 0x01) && (InstructionData->ModRm.Uint8 == 0xd9)) {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_DR7_WRITE:
> +      CcDecodeModRm (Regs, InstructionData);
> +
> +      if ((OpCode == 0x23) &&
> +          (InstructionData->Ext.ModRm.Reg == 7))
> +      {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_WBINVD:
> +      if (OpCode == 0x9) {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    default:
> +      break;
> +  }
> +
> +  return UnsupportedExit (Ghcb, Regs, InstructionData);
> +}
> +
>   /**
>     Handle a #VC exception.
>   
> @@ -1773,7 +1917,12 @@ InternalVmgExitHandleVc (
>   
>     CcInitInstructionData (&InstructionData, Ghcb, Regs);
>   
> -  Status = NaeExit (Ghcb, Regs, &InstructionData);
> +  Status = VcCheckOpcodeBytes (Ghcb, Regs, &InstructionData, ExitCode);
> +
> +  if (Status == 0) {
> +    Status = NaeExit (Ghcb, Regs, &InstructionData);
> +  }
> +
>     if (Status == 0) {
>       Regs->Rip += CcInstructionLength (&InstructionData);
>     } else {


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118031): https://edk2.groups.io/g/devel/message/118031
Mute This Topic: https://groups.io/mt/105581633/7686176
Mute #vc:https://edk2.groups.io/g/devel/mutehashtag/vc
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] OvmfPkg: Harden #VC instruction emulation somewhat (CVE-2024-25742)

[Adam Dunlap via groups.io]

On Fri, Apr 19, 2024 at 8:13 AM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 4/17/24 11:54, Adam Dunlap wrote:
> > +
> > +    case SVM_EXIT_INVD:
> > +      break;
>
> This changes the current behavior today, but I'm ok with that.
>

Whoops, I should've checked that. Should we delete InvdExit() then, if
it's dead code?

> > +
> > +    case SVM_EXIT_MONITOR:
> > +      CcDecodeModRm (Regs, InstructionData);
> > +
> > +      if ((OpCode == 0x01) && (InstructionData->ModRm.Uint8 == 0xc8)) {
>
> This should also handle the MONITORX opcode (hmmm... I need to send a
> patch to the kernel).
>
> > +        return 0;
> > +      }
> > +
> > +      break;
> > +
> > +    case SVM_EXIT_MWAIT:
> > +      CcDecodeModRm (Regs, InstructionData);
> > +
> > +      if ((OpCode == 0x01) && (InstructionData->ModRm.Uint8 == 0xc9)) {
>
> Same here for MWAITX.
>
> Thanks,
> Tom

Got it! I'll send out a new patch shortly if I can figure out how to
use git send-email correctly.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118037): https://edk2.groups.io/g/devel/message/118037
Mute This Topic: https://groups.io/mt/105581633/7686176
Mute #vc:https://edk2.groups.io/g/devel/mutehashtag/vc
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2] OvmfPkg: Harden #VC instruction emulation somewhat (CVE-2024-25742)

[Adam Dunlap via groups.io]

Ensure that when a #VC exception happens, the instruction at the
instruction pointer matches the instruction that is expected given the
error code. This is to mitigate the ahoi WeSee attack [1] that could
allow hypervisors to breach integrity and confidentiality of the
firmware by maliciously injecting interrupts. This change is a
translated version of a linux patch e3ef461af35a ("x86/sev: Harden #VC
instruction emulation somewhat")

[1] https://ahoi-attacks.github.io/wesee/

Cc: Borislav Petkov (AMD) <bp@alien8.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Adam Dunlap <acdunlap@google.com>
---

Patch changelog:
V1 -> V2: Added the MWAITX/MONITORX opcodes. Added the opcode for INVD.
Added a comment to explain skipping the exit handling if the opcode does
not match.

 OvmfPkg/Library/CcExitLib/CcExitVcHandler.c | 184 ++++++++++++++++++--
 1 file changed, 173 insertions(+), 11 deletions(-)

diff --git a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
index 0fc30f7bc4..31587586fe 100644
--- a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
+++ b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
@@ -532,8 +532,6 @@ MwaitExit (
   IN     CC_INSTRUCTION_DATA     *InstructionData
   )
 {
-  CcDecodeModRm (Regs, InstructionData);
-
   Ghcb->SaveArea.Rax = Regs->Rax;
   CcExitVmgSetOffsetValid (Ghcb, GhcbRax);
   Ghcb->SaveArea.Rcx = Regs->Rcx;
@@ -564,8 +562,6 @@ MonitorExit (
   IN     CC_INSTRUCTION_DATA     *InstructionData
   )
 {
-  CcDecodeModRm (Regs, InstructionData);
-
   Ghcb->SaveArea.Rax = Regs->Rax;  // Identity mapped, so VA = PA
   CcExitVmgSetOffsetValid (Ghcb, GhcbRax);
   Ghcb->SaveArea.Rcx = Regs->Rcx;
@@ -670,8 +666,6 @@ VmmCallExit (
 {
   UINT64  Status;
 
-  CcDecodeModRm (Regs, InstructionData);
-
   Ghcb->SaveArea.Rax = Regs->Rax;
   CcExitVmgSetOffsetValid (Ghcb, GhcbRax);
   Ghcb->SaveArea.Cpl = (UINT8)(Regs->Cs & 0x3);
@@ -1603,8 +1597,6 @@ Dr7WriteExit (
   Ext       = &InstructionData->Ext;
   SevEsData = (SEV_ES_PER_CPU_DATA *)(Ghcb + 1);
 
-  CcDecodeModRm (Regs, InstructionData);
-
   //
   // MOV DRn always treats MOD == 3 no matter how encoded
   //
@@ -1655,8 +1647,6 @@ Dr7ReadExit (
   Ext       = &InstructionData->Ext;
   SevEsData = (SEV_ES_PER_CPU_DATA *)(Ghcb + 1);
 
-  CcDecodeModRm (Regs, InstructionData);
-
   //
   // MOV DRn always treats MOD == 3 no matter how encoded
   //
@@ -1671,6 +1661,170 @@ Dr7ReadExit (
   return 0;
 }
 
+/**
+  Check that the opcode matches the exit code for a #VC.
+
+  Each exit code should only be raised while executing certain instructions.
+  Verify that rIP points to a correct instruction based on the exit code to
+  protect against maliciously injected interrupts via the hypervisor. If it does
+  not, report an unsupported event to the hypervisor.
+
+  Decodes the ModRm byte into InstructionData if necessary.
+
+  @param[in, out] Ghcb             Pointer to the Guest-Hypervisor Communication
+                                   Block
+  @param[in, out] Regs             x64 processor context
+  @param[in, out] InstructionData  Instruction parsing context
+  @param[in]      ExitCode         Exit code given by #VC.
+
+  @retval 0                        No problems detected.
+  @return                          New exception value to propagate
+
+
+**/
+STATIC
+UINT64
+VcCheckOpcodeBytes (
+  IN OUT GHCB                    *Ghcb,
+  IN OUT EFI_SYSTEM_CONTEXT_X64  *Regs,
+  IN OUT CC_INSTRUCTION_DATA     *InstructionData,
+  IN     UINT64                  ExitCode
+  )
+{
+  UINT8  OpCode;
+
+  //
+  // Expected opcodes are either 1 or 2 bytes. If they are 2 bytes, they always
+  // start with TWO_BYTE_OPCODE_ESCAPE (0x0f), so skip over that.
+  //
+  OpCode = *(InstructionData->OpCodes);
+  if (OpCode == TWO_BYTE_OPCODE_ESCAPE) {
+    OpCode = *(InstructionData->OpCodes + 1);
+  }
+
+  switch (ExitCode) {
+    case SVM_EXIT_IOIO_PROT:
+    case SVM_EXIT_NPF:
+      /* handled separately */
+      return 0;
+
+    case SVM_EXIT_CPUID:
+      if (OpCode == 0xa2) {
+        return 0;
+      }
+
+      break;
+
+    case SVM_EXIT_INVD:
+      if (OpCode == 0x08) {
+        return 0;
+      }
+
+      break;
+
+    case SVM_EXIT_MONITOR:
+      CcDecodeModRm (Regs, InstructionData);
+
+      if ((OpCode == 0x01) &&
+          (  (InstructionData->ModRm.Uint8 == 0xc8)   /* MONITOR */
+          || (InstructionData->ModRm.Uint8 == 0xfa))) /* MONITORX */
+      {
+        return 0;
+      }
+
+      break;
+
+    case SVM_EXIT_MWAIT:
+      CcDecodeModRm (Regs, InstructionData);
+
+      if ((OpCode == 0x01) &&
+          (  (InstructionData->ModRm.Uint8 == 0xc9)   /* MWAIT */
+          || (InstructionData->ModRm.Uint8 == 0xfb))) /* MWAITX */
+      {
+        return 0;
+      }
+
+      break;
+
+    case SVM_EXIT_MSR:
+      /* RDMSR */
+      if ((OpCode == 0x32) ||
+          /* WRMSR */
+          (OpCode == 0x30))
+      {
+        return 0;
+      }
+
+      break;
+
+    case SVM_EXIT_RDPMC:
+      if (OpCode == 0x33) {
+        return 0;
+      }
+
+      break;
+
+    case SVM_EXIT_RDTSC:
+      if (OpCode == 0x31) {
+        return 0;
+      }
+
+      break;
+
+    case SVM_EXIT_RDTSCP:
+      CcDecodeModRm (Regs, InstructionData);
+
+      if ((OpCode == 0x01) && (InstructionData->ModRm.Uint8 == 0xf9)) {
+        return 0;
+      }
+
+      break;
+
+    case SVM_EXIT_DR7_READ:
+      CcDecodeModRm (Regs, InstructionData);
+
+      if ((OpCode == 0x21) &&
+          (InstructionData->Ext.ModRm.Reg == 7))
+      {
+        return 0;
+      }
+
+      break;
+
+    case SVM_EXIT_VMMCALL:
+      CcDecodeModRm (Regs, InstructionData);
+
+      if ((OpCode == 0x01) && (InstructionData->ModRm.Uint8 == 0xd9)) {
+        return 0;
+      }
+
+      break;
+
+    case SVM_EXIT_DR7_WRITE:
+      CcDecodeModRm (Regs, InstructionData);
+
+      if ((OpCode == 0x23) &&
+          (InstructionData->Ext.ModRm.Reg == 7))
+      {
+        return 0;
+      }
+
+      break;
+
+    case SVM_EXIT_WBINVD:
+      if (OpCode == 0x9) {
+        return 0;
+      }
+
+      break;
+
+    default:
+      break;
+  }
+
+  return UnsupportedExit (Ghcb, Regs, InstructionData);
+}
+
 /**
   Handle a #VC exception.
 
@@ -1773,7 +1927,15 @@ InternalVmgExitHandleVc (
 
   CcInitInstructionData (&InstructionData, Ghcb, Regs);
 
-  Status = NaeExit (Ghcb, Regs, &InstructionData);
+  Status = VcCheckOpcodeBytes (Ghcb, Regs, &InstructionData, ExitCode);
+
+  //
+  // If the opcode does not match the exit code, do not process the exception
+  //
+  if (Status == 0) {
+    Status = NaeExit (Ghcb, Regs, &InstructionData);
+  }
+
   if (Status == 0) {
     Regs->Rip += CcInstructionLength (&InstructionData);
   } else {
-- 
2.44.0.769.g3c40516874-goog



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118038): https://edk2.groups.io/g/devel/message/118038
Mute This Topic: https://groups.io/mt/105623545/7686176
Mute #vc:https://edk2.groups.io/g/devel/mutehashtag/vc
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2] OvmfPkg: Harden #VC instruction emulation somewhat (CVE-2024-25742)

[Lendacky, Thomas via groups.io]

On 4/19/24 13:21, Adam Dunlap via groups.io wrote:
> Ensure that when a #VC exception happens, the instruction at the
> instruction pointer matches the instruction that is expected given the
> error code. This is to mitigate the ahoi WeSee attack [1] that could
> allow hypervisors to breach integrity and confidentiality of the
> firmware by maliciously injecting interrupts. This change is a
> translated version of a linux patch e3ef461af35a ("x86/sev: Harden #VC
> instruction emulation somewhat")
> 
> [1] https://ahoi-attacks.github.io/wesee/
> 
> Cc: Borislav Petkov (AMD) <bp@alien8.de>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Adam Dunlap <acdunlap@google.com>

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
> 
> Patch changelog:
> V1 -> V2: Added the MWAITX/MONITORX opcodes. Added the opcode for INVD.
> Added a comment to explain skipping the exit handling if the opcode does
> not match.
> 
>   OvmfPkg/Library/CcExitLib/CcExitVcHandler.c | 184 ++++++++++++++++++--
>   1 file changed, 173 insertions(+), 11 deletions(-)
> 
> diff --git a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
> index 0fc30f7bc4..31587586fe 100644
> --- a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
> +++ b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
> @@ -532,8 +532,6 @@ MwaitExit (
>     IN     CC_INSTRUCTION_DATA     *InstructionData
>     )
>   {
> -  CcDecodeModRm (Regs, InstructionData);
> -
>     Ghcb->SaveArea.Rax = Regs->Rax;
>     CcExitVmgSetOffsetValid (Ghcb, GhcbRax);
>     Ghcb->SaveArea.Rcx = Regs->Rcx;
> @@ -564,8 +562,6 @@ MonitorExit (
>     IN     CC_INSTRUCTION_DATA     *InstructionData
>     )
>   {
> -  CcDecodeModRm (Regs, InstructionData);
> -
>     Ghcb->SaveArea.Rax = Regs->Rax;  // Identity mapped, so VA = PA
>     CcExitVmgSetOffsetValid (Ghcb, GhcbRax);
>     Ghcb->SaveArea.Rcx = Regs->Rcx;
> @@ -670,8 +666,6 @@ VmmCallExit (
>   {
>     UINT64  Status;
>   
> -  CcDecodeModRm (Regs, InstructionData);
> -
>     Ghcb->SaveArea.Rax = Regs->Rax;
>     CcExitVmgSetOffsetValid (Ghcb, GhcbRax);
>     Ghcb->SaveArea.Cpl = (UINT8)(Regs->Cs & 0x3);
> @@ -1603,8 +1597,6 @@ Dr7WriteExit (
>     Ext       = &InstructionData->Ext;
>     SevEsData = (SEV_ES_PER_CPU_DATA *)(Ghcb + 1);
>   
> -  CcDecodeModRm (Regs, InstructionData);
> -
>     //
>     // MOV DRn always treats MOD == 3 no matter how encoded
>     //
> @@ -1655,8 +1647,6 @@ Dr7ReadExit (
>     Ext       = &InstructionData->Ext;
>     SevEsData = (SEV_ES_PER_CPU_DATA *)(Ghcb + 1);
>   
> -  CcDecodeModRm (Regs, InstructionData);
> -
>     //
>     // MOV DRn always treats MOD == 3 no matter how encoded
>     //
> @@ -1671,6 +1661,170 @@ Dr7ReadExit (
>     return 0;
>   }
>   
> +/**
> +  Check that the opcode matches the exit code for a #VC.
> +
> +  Each exit code should only be raised while executing certain instructions.
> +  Verify that rIP points to a correct instruction based on the exit code to
> +  protect against maliciously injected interrupts via the hypervisor. If it does
> +  not, report an unsupported event to the hypervisor.
> +
> +  Decodes the ModRm byte into InstructionData if necessary.
> +
> +  @param[in, out] Ghcb             Pointer to the Guest-Hypervisor Communication
> +                                   Block
> +  @param[in, out] Regs             x64 processor context
> +  @param[in, out] InstructionData  Instruction parsing context
> +  @param[in]      ExitCode         Exit code given by #VC.
> +
> +  @retval 0                        No problems detected.
> +  @return                          New exception value to propagate
> +
> +
> +**/
> +STATIC
> +UINT64
> +VcCheckOpcodeBytes (
> +  IN OUT GHCB                    *Ghcb,
> +  IN OUT EFI_SYSTEM_CONTEXT_X64  *Regs,
> +  IN OUT CC_INSTRUCTION_DATA     *InstructionData,
> +  IN     UINT64                  ExitCode
> +  )
> +{
> +  UINT8  OpCode;
> +
> +  //
> +  // Expected opcodes are either 1 or 2 bytes. If they are 2 bytes, they always
> +  // start with TWO_BYTE_OPCODE_ESCAPE (0x0f), so skip over that.
> +  //
> +  OpCode = *(InstructionData->OpCodes);
> +  if (OpCode == TWO_BYTE_OPCODE_ESCAPE) {
> +    OpCode = *(InstructionData->OpCodes + 1);
> +  }
> +
> +  switch (ExitCode) {
> +    case SVM_EXIT_IOIO_PROT:
> +    case SVM_EXIT_NPF:
> +      /* handled separately */
> +      return 0;
> +
> +    case SVM_EXIT_CPUID:
> +      if (OpCode == 0xa2) {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_INVD:
> +      if (OpCode == 0x08) {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_MONITOR:
> +      CcDecodeModRm (Regs, InstructionData);
> +
> +      if ((OpCode == 0x01) &&
> +          (  (InstructionData->ModRm.Uint8 == 0xc8)   /* MONITOR */
> +          || (InstructionData->ModRm.Uint8 == 0xfa))) /* MONITORX */
> +      {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_MWAIT:
> +      CcDecodeModRm (Regs, InstructionData);
> +
> +      if ((OpCode == 0x01) &&
> +          (  (InstructionData->ModRm.Uint8 == 0xc9)   /* MWAIT */
> +          || (InstructionData->ModRm.Uint8 == 0xfb))) /* MWAITX */
> +      {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_MSR:
> +      /* RDMSR */
> +      if ((OpCode == 0x32) ||
> +          /* WRMSR */
> +          (OpCode == 0x30))
> +      {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_RDPMC:
> +      if (OpCode == 0x33) {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_RDTSC:
> +      if (OpCode == 0x31) {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_RDTSCP:
> +      CcDecodeModRm (Regs, InstructionData);
> +
> +      if ((OpCode == 0x01) && (InstructionData->ModRm.Uint8 == 0xf9)) {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_DR7_READ:
> +      CcDecodeModRm (Regs, InstructionData);
> +
> +      if ((OpCode == 0x21) &&
> +          (InstructionData->Ext.ModRm.Reg == 7))
> +      {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_VMMCALL:
> +      CcDecodeModRm (Regs, InstructionData);
> +
> +      if ((OpCode == 0x01) && (InstructionData->ModRm.Uint8 == 0xd9)) {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_DR7_WRITE:
> +      CcDecodeModRm (Regs, InstructionData);
> +
> +      if ((OpCode == 0x23) &&
> +          (InstructionData->Ext.ModRm.Reg == 7))
> +      {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    case SVM_EXIT_WBINVD:
> +      if (OpCode == 0x9) {
> +        return 0;
> +      }
> +
> +      break;
> +
> +    default:
> +      break;
> +  }
> +
> +  return UnsupportedExit (Ghcb, Regs, InstructionData);
> +}
> +
>   /**
>     Handle a #VC exception.
>   
> @@ -1773,7 +1927,15 @@ InternalVmgExitHandleVc (
>   
>     CcInitInstructionData (&InstructionData, Ghcb, Regs);
>   
> -  Status = NaeExit (Ghcb, Regs, &InstructionData);
> +  Status = VcCheckOpcodeBytes (Ghcb, Regs, &InstructionData, ExitCode);
> +
> +  //
> +  // If the opcode does not match the exit code, do not process the exception
> +  //
> +  if (Status == 0) {
> +    Status = NaeExit (Ghcb, Regs, &InstructionData);
> +  }
> +
>     if (Status == 0) {
>       Regs->Rip += CcInstructionLength (&InstructionData);
>     } else {


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118096): https://edk2.groups.io/g/devel/message/118096
Mute This Topic: https://groups.io/mt/105623545/7686176
Mute #vc:https://edk2.groups.io/g/devel/mutehashtag/vc
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2] OvmfPkg: Harden #VC instruction emulation somewhat (CVE-2024-25742)

[Gerd Hoffmann]

On Fri, Apr 19, 2024 at 11:21:46AM -0700, Adam Dunlap wrote:
> Ensure that when a #VC exception happens, the instruction at the
> instruction pointer matches the instruction that is expected given the
> error code. This is to mitigate the ahoi WeSee attack [1] that could
> allow hypervisors to breach integrity and confidentiality of the
> firmware by maliciously injecting interrupts. This change is a
> translated version of a linux patch e3ef461af35a ("x86/sev: Harden #VC
> instruction emulation somewhat")
> 
> [1] https://ahoi-attacks.github.io/wesee/
> 
> Cc: Borislav Petkov (AMD) <bp@alien8.de>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Adam Dunlap <acdunlap@google.com>

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118132): https://edk2.groups.io/g/devel/message/118132
Mute This Topic: https://groups.io/mt/105623545/7686176
Mute #vc:https://edk2.groups.io/g/devel/mutehashtag/vc
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2] OvmfPkg: Harden #VC instruction emulation somewhat (CVE-2024-25742)

[Ard Biesheuvel]

On Tue, 23 Apr 2024 at 11:28, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Fri, Apr 19, 2024 at 11:21:46AM -0700, Adam Dunlap wrote:
> > Ensure that when a #VC exception happens, the instruction at the
> > instruction pointer matches the instruction that is expected given the
> > error code. This is to mitigate the ahoi WeSee attack [1] that could
> > allow hypervisors to breach integrity and confidentiality of the
> > firmware by maliciously injecting interrupts. This change is a
> > translated version of a linux patch e3ef461af35a ("x86/sev: Harden #VC
> > instruction emulation somewhat")
> >
> > [1] https://ahoi-attacks.github.io/wesee/
> >
> > Cc: Borislav Petkov (AMD) <bp@alien8.de>
> > Cc: Tom Lendacky <thomas.lendacky@amd.com>
> > Signed-off-by: Adam Dunlap <acdunlap@google.com>
>
> Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
>

Thanks all, I've merged this now.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118221): https://edk2.groups.io/g/devel/message/118221
Mute This Topic: https://groups.io/mt/105623545/7686176
Mute #vc:https://edk2.groups.io/g/devel/mutehashtag/vc
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-