From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail04.groups.io (mail04.groups.io [45.79.224.9]) by spool.mail.gandi.net (Postfix) with ESMTPS id 27711941D97 for ; Wed, 17 Apr 2024 17:45:22 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=tLyYccE4jfbmutO5S/Vr4WxUfTf/Y0VZPSbzpBqbcXM=; 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:Content-Transfer-Encoding; s=20240206; t=1713375921; v=1; b=qo6A8qq6oRZfJs185wEAFddVUmRzGhfBTk5Sh5Qb7tIqwzk1DwEy//GDJ1hszCZTyp2X90iC eEgWgrJ3xGzE3DliZ4bxp4tJ+c/UNHRSqnXhefgDh1yKDAN6BnSMCQhmKzX5WlAGqiG32y6LP1I hJv0Ab+KweizDEhfk73ZzjU+AGyiwgMXw4zLGKp/AlT7iOKGex0AruNv9gO3Y/KdoIrQYfVGjrs egLk3/7tY2fjlSVlljv19m/UfW2VbtPtWbE8MxJvmkOTubaK6ockq1SZcHjaRvYvgatYuiOHE/b 57IvoMQKo7DYX8KmRs4dlsUuwiAnxFsW/jiBySx6qLVhQ== X-Received: by 127.0.0.2 with SMTP id XKJdYY7687511xdlB5K33qag; Wed, 17 Apr 2024 10:45:21 -0700 X-Received: from mail-pj1-f52.google.com (mail-pj1-f52.google.com [209.85.216.52]) by mx.groups.io with SMTP id smtpd.web11.20656.1713375921113378607 for ; Wed, 17 Apr 2024 10:45:21 -0700 X-Received: by mail-pj1-f52.google.com with SMTP id 98e67ed59e1d1-2a4bdef3d8eso45327a91.1 for ; Wed, 17 Apr 2024 10:45:21 -0700 (PDT) X-Gm-Message-State: au6ZKa8bKVpmlmgogZkdrNvYx7686176AA= X-Google-Smtp-Source: AGHT+IHF7/rGaUNXXHQs6AYH9rxpo4CCai8R6YGTXDjLmZRkyprhCwVTCgp12kE34mpLONrbZf7Q5DSymBdYZwuoaNM= X-Received: by 2002:a17:90a:9206:b0:2a3:10d3:239d with SMTP id m6-20020a17090a920600b002a310d3239dmr66742pjo.32.1713375920183; Wed, 17 Apr 2024 10:45:20 -0700 (PDT) MIME-Version: 1.0 References: <20240417165400.3615824-1-acdunlap@google.com> In-Reply-To: From: "Adam Dunlap via groups.io" Date: Wed, 17 Apr 2024 10:45:08 -0700 Message-ID: Subject: Re: [edk2-devel] [PATCH] OvmfPkg: Harden #VC instruction emulation somewhat (CVE-2024-25742) To: Ard Biesheuvel Cc: devel@edk2.groups.io, Jiewen Yao , Borislav Petkov , Peter Gonda , Tom Lendacky , Erdem Aktas , Gerd Hoffmann , Michael Roth , Min Xu Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Resent-Date: Wed, 17 Apr 2024 10:45:21 -0700 Resent-From: acdunlap@google.com Reply-To: devel@edk2.groups.io,acdunlap@google.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240206 header.b=qo6A8qq6; dmarc=pass (policy=none) header.from=groups.io; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 45.79.224.9 as permitted sender) smtp.mailfrom=bounce@groups.io On Wed, Apr 17, 2024 at 10:08=E2=80=AFAM Ard Biesheuvel w= rote: > > (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 > 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) > > Cc: Tom Lendacky > > Signed-off-by: Adam Dunlap > > --- > > OvmfPkg/Library/CcExitLib/CcExitVcHandler.c | 171 ++++++++++++++++++-- > > 1 file changed, 160 insertions(+), 11 deletions(-) > > > > diff --git a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c b/OvmfPkg/Libr= ary/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 =3D Regs->Rax; > > CcExitVmgSetOffsetValid (Ghcb, GhcbRax); > > Ghcb->SaveArea.Rcx =3D Regs->Rcx; > > @@ -564,8 +562,6 @@ MonitorExit ( > > IN CC_INSTRUCTION_DATA *InstructionData > > ) > > { > > - CcDecodeModRm (Regs, InstructionData); > > - > > Ghcb->SaveArea.Rax =3D Regs->Rax; // Identity mapped, so VA =3D PA > > CcExitVmgSetOffsetValid (Ghcb, GhcbRax); > > Ghcb->SaveArea.Rcx =3D Regs->Rcx; > > @@ -670,8 +666,6 @@ VmmCallExit ( > > { > > UINT64 Status; > > > > - CcDecodeModRm (Regs, InstructionData); > > - > > Ghcb->SaveArea.Rax =3D Regs->Rax; > > CcExitVmgSetOffsetValid (Ghcb, GhcbRax); > > Ghcb->SaveArea.Cpl =3D (UINT8)(Regs->Cs & 0x3); > > @@ -1603,8 +1597,6 @@ Dr7WriteExit ( > > Ext =3D &InstructionData->Ext; > > SevEsData =3D (SEV_ES_PER_CPU_DATA *)(Ghcb + 1); > > > > - CcDecodeModRm (Regs, InstructionData); > > - > > // > > // MOV DRn always treats MOD =3D=3D 3 no matter how encoded > > // > > @@ -1655,8 +1647,6 @@ Dr7ReadExit ( > > Ext =3D &InstructionData->Ext; > > SevEsData =3D (SEV_ES_PER_CPU_DATA *)(Ghcb + 1); > > > > - CcDecodeModRm (Regs, InstructionData); > > - > > // > > // MOV DRn always treats MOD =3D=3D 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 instruc= tions. > > + Verify that rIP points to a correct instruction based on the exit co= de 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 Com= munication > > + 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, th= ey always > > + // start with TWO_BYTE_OPCODE_ESCAPE (0x0f), so skip over that. > > + // > > + OpCode =3D *(InstructionData->OpCodes); > > + if (OpCode =3D=3D TWO_BYTE_OPCODE_ESCAPE) { > > + OpCode =3D *(InstructionData->OpCodes + 1); > > + } > > + > > + switch (ExitCode) { > > + case SVM_EXIT_IOIO_PROT: > > + case SVM_EXIT_NPF: > > + /* handled separately */ > > + return 0; > > + > > + case SVM_EXIT_CPUID: > > + if (OpCode =3D=3D 0xa2) { > > + return 0; > > + } > > + > > + break; > > + > > + case SVM_EXIT_INVD: > > + break; > > + > > + case SVM_EXIT_MONITOR: > > + CcDecodeModRm (Regs, InstructionData); > > + > > + if ((OpCode =3D=3D 0x01) && (InstructionData->ModRm.Uint8 =3D=3D= 0xc8)) { > > + return 0; > > + } > > + > > + break; > > + > > + case SVM_EXIT_MWAIT: > > + CcDecodeModRm (Regs, InstructionData); > > + > > + if ((OpCode =3D=3D 0x01) && (InstructionData->ModRm.Uint8 =3D=3D= 0xc9)) { > > + return 0; > > + } > > + > > + break; > > + > > + case SVM_EXIT_MSR: > > + /* RDMSR */ > > + if ((OpCode =3D=3D 0x32) || > > + /* WRMSR */ > > + (OpCode =3D=3D 0x30)) > > + { > > + return 0; > > + } > > + > > + break; > > + > > + case SVM_EXIT_RDPMC: > > + if (OpCode =3D=3D 0x33) { > > + return 0; > > + } > > + > > + break; > > + > > + case SVM_EXIT_RDTSC: > > + if (OpCode =3D=3D 0x31) { > > + return 0; > > + } > > + > > + break; > > + > > + case SVM_EXIT_RDTSCP: > > + CcDecodeModRm (Regs, InstructionData); > > + > > + if ((OpCode =3D=3D 0x01) && (InstructionData->ModRm.Uint8 =3D=3D= 0xf9)) { > > + return 0; > > + } > > + > > + break; > > + > > + case SVM_EXIT_DR7_READ: > > + CcDecodeModRm (Regs, InstructionData); > > + > > + if ((OpCode =3D=3D 0x21) && > > + (InstructionData->Ext.ModRm.Reg =3D=3D 7)) > > + { > > + return 0; > > + } > > + > > + break; > > + > > + case SVM_EXIT_VMMCALL: > > + CcDecodeModRm (Regs, InstructionData); > > + > > + if ((OpCode =3D=3D 0x01) && (InstructionData->ModRm.Uint8 =3D=3D= 0xd9)) { > > + return 0; > > + } > > + > > + break; > > + > > + case SVM_EXIT_DR7_WRITE: > > + CcDecodeModRm (Regs, InstructionData); > > + > > + if ((OpCode =3D=3D 0x23) && > > + (InstructionData->Ext.ModRm.Reg =3D=3D 7)) > > + { > > + return 0; > > + } > > + > > + break; > > + > > + case SVM_EXIT_WBINVD: > > + if (OpCode =3D=3D 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 =3D NaeExit (Ghcb, Regs, &InstructionData); > > + Status =3D VcCheckOpcodeBytes (Ghcb, Regs, &InstructionData, ExitCod= e); > > + > > + if (Status =3D=3D 0) { > > + Status =3D 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 it= s 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 retur= ned) then the code could be a bit simpler since it wouldn't need to handle error cases. > > if (Status =3D=3D 0) { > > Regs->Rip +=3D CcInstructionLength (&InstructionData); > > } else { > > -- > > 2.44.0.683.g7961c838ac-goog > > > > > > > >=20 > > > > -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-