From: "Yao, Jiewen" <jiewen.yao@intel.com>
To: Adam Dunlap <acdunlap@google.com>, Ard Biesheuvel <ardb@kernel.org>
Cc: "devel@edk2.groups.io" <devel@edk2.groups.io>,
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)
Date: Thu, 18 Apr 2024 08:03:22 +0000 [thread overview]
Message-ID: <MW4PR11MB587256348CE4A17227073D848C0E2@MW4PR11MB5872.namprd11.prod.outlook.com> (raw)
In-Reply-To: <CAMBK9=ZiiOhVtJdQzO=xvzVwmCihhKhCM=Y=c6Jveg38bBM1CA@mail.gmail.com>
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]
-=-=-=-=-=-=-=-=-=-=-=-
next prev parent reply other threads:[~2024-04-18 8:03 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-17 16:54 [edk2-devel] [PATCH] OvmfPkg: Harden #VC instruction emulation somewhat (CVE-2024-25742) Adam Dunlap via groups.io
2024-04-17 17:08 ` Ard Biesheuvel
2024-04-17 17:45 ` Adam Dunlap via groups.io
2024-04-18 8:03 ` Yao, Jiewen [this message]
2024-04-18 12:15 ` Gerd Hoffmann
2024-04-18 15:39 ` Adam Dunlap via groups.io
2024-04-18 15:43 ` Peter Gonda via groups.io
2024-04-19 11:31 ` Gerd Hoffmann
2024-04-19 14:56 ` Lendacky, Thomas via groups.io
2024-04-19 15:12 ` Lendacky, Thomas via groups.io
2024-04-19 17:39 ` Adam Dunlap via groups.io
2024-04-19 18:21 ` [edk2-devel] [PATCH v2] " Adam Dunlap via groups.io
2024-04-22 14:12 ` Lendacky, Thomas via groups.io
2024-04-23 9:27 ` Gerd Hoffmann
2024-04-24 16:27 ` Ard Biesheuvel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=MW4PR11MB587256348CE4A17227073D848C0E2@MW4PR11MB5872.namprd11.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox