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

["Adam Dunlap via groups.io" <acdunlap=google.com@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]
-=-=-=-=-=-=-=-=-=-=-=-