From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <bounce+27952+117927+7686176+12367111@groups.io>
Received: from mail04.groups.io (mail04.groups.io [45.79.224.9])
	by spool.mail.gandi.net (Postfix) with ESMTPS id ADC57D8042C
	for <rebecca@openfw.io>; Wed, 17 Apr 2024 17:08:37 +0000 (UTC)
DKIM-Signature: a=rsa-sha256; bh=q/7td0L4T0O8/dkqRRLq5uBmJ+yxGQGJXqTYWBwo5HA=;
 c=relaxed/simple; d=groups.io;
 h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Resent-Date:Resent-From:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type;
 s=20240206; t=1713373716; v=1;
 b=fvZoHh0OwqpHsxKxfPdnxHWXtcN0yZPSi03H+yIsUO/kioLyVnOA75pJ1g7i4apulCTvVjTL
 5yaI6gLiA8KGGX6HDdX1ee71JCg3Be9k17+hVqsBvGy4yHW8TVJ0mfAmeJzJh6Qc0lIsJUsko1z
 FVSD1VEfDp+UzDwfHd0lenlN1DU9pRiWfIhgm6z0NKSUD9xq+cAgj1mbKoxyF9XrGY0DT1altSF
 h+Mb/zM0VQsR3EE5FzNHfUk8Ka8LWmLU9igOkHIdBn08IAVBNCyQFTljuIr5Y63kAAVPR34ALOY
 lANcr9+0Iq6ixp4RD0ElhI/ftRxbsA9HOqiEXZkbnLEog==
X-Received: by 127.0.0.2 with SMTP id gZ4EYY7687511xfql9zOOFV4; Wed, 17 Apr 2024 10:08:36 -0700
X-Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217])
 by mx.groups.io with SMTP id smtpd.web11.19795.1713373715593464503
 for <devel@edk2.groups.io>;
 Wed, 17 Apr 2024 10:08:35 -0700
X-Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58])
	by dfw.source.kernel.org (Postfix) with ESMTP id 15E04614AE
	for <devel@edk2.groups.io>; Wed, 17 Apr 2024 17:08:35 +0000 (UTC)
X-Received: by smtp.kernel.org (Postfix) with ESMTPSA id B82A5C32783
	for <devel@edk2.groups.io>; Wed, 17 Apr 2024 17:08:34 +0000 (UTC)
X-Received: by mail-lj1-f177.google.com with SMTP id 38308e7fff4ca-2d895e2c6efso75806861fa.0
        for <devel@edk2.groups.io>; Wed, 17 Apr 2024 10:08:34 -0700 (PDT)
X-Gm-Message-State: zRUZ7eudBIpnmUS5NvO5tKj3x7686176AA=
X-Google-Smtp-Source: AGHT+IHuI4nFrURyoWMGjPC4SY6T0b+TEdin8GmnNCuHq6Z2JAWyn5lfLO7pY/YZSvs2c0n848wVinqjdg1QPqSoCoY=
X-Received: by 2002:a2e:9ada:0:b0:2d8:6a04:6ac4 with SMTP id
 p26-20020a2e9ada000000b002d86a046ac4mr10771982ljj.28.1713373712861; Wed, 17
 Apr 2024 10:08:32 -0700 (PDT)
MIME-Version: 1.0
References: <20240417165400.3615824-1-acdunlap@google.com>
In-Reply-To: <20240417165400.3615824-1-acdunlap@google.com>
From: "Ard Biesheuvel" <ardb@kernel.org>
Date: Wed, 17 Apr 2024 19:08:21 +0200
X-Gmail-Original-Message-ID: <CAMj1kXHuvz7wBPaTQFN9T03xZYhj7HVuK--_XLgTd0JZL_+HNg@mail.gmail.com>
Message-ID: <CAMj1kXHuvz7wBPaTQFN9T03xZYhj7HVuK--_XLgTd0JZL_+HNg@mail.gmail.com>
Subject: Re: [edk2-devel] [PATCH] OvmfPkg: Harden #VC instruction emulation somewhat (CVE-2024-25742)
To: devel@edk2.groups.io, acdunlap@google.com, 
	Jiewen Yao <jiewen.yao@intel.com>
Cc: Borislav Petkov <bp@alien8.de>, Peter Gonda <pgonda@google.com>, 
	Tom Lendacky <thomas.lendacky@amd.com>
Precedence: Bulk
List-Subscribe: <mailto:devel+subscribe@edk2.groups.io>
List-Help: <mailto:devel+help@edk2.groups.io>
Sender: devel@edk2.groups.io
List-Id: <devel.edk2.groups.io>
Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io
Resent-Date: Wed, 17 Apr 2024 10:08:35 -0700
Resent-From: ardb@kernel.org
Reply-To: devel@edk2.groups.io,ardb@kernel.org
List-Unsubscribe-Post: List-Unsubscribe=One-Click
List-Unsubscribe: <https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/plugh>
Content-Type: text/plain; charset="UTF-8"
X-GND-Status: LEGIT
Authentication-Results: spool.mail.gandi.net;
	dkim=pass header.d=groups.io header.s=20240206 header.b=fvZoHh0O;
	spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 45.79.224.9 as permitted sender) smtp.mailfrom=bounce@groups.io;
	dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=kernel.org (policy=none)

(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]
-=-=-=-=-=-=-=-=-=-=-=-