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 ADC57D8042C for ; 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 ; 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 ; Wed, 17 Apr 2024 17:08:35 +0000 (UTC) X-Received: by smtp.kernel.org (Postfix) with ESMTPSA id B82A5C32783 for ; Wed, 17 Apr 2024 17:08:34 +0000 (UTC) X-Received: by mail-lj1-f177.google.com with SMTP id 38308e7fff4ca-2d895e2c6efso75806861fa.0 for ; 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" Date: Wed, 17 Apr 2024 19:08:21 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH] OvmfPkg: Harden #VC instruction emulation somewhat (CVE-2024-25742) To: devel@edk2.groups.io, acdunlap@google.com, Jiewen Yao Cc: Borislav Petkov , Peter Gonda , Tom Lendacky 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: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: 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 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/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] -=-=-=-=-=-=-=-=-=-=-=-