From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by mx.groups.io with SMTP id smtpd.web11.90720.1673308590013112462 for ; Mon, 09 Jan 2023 15:56:34 -0800 Authentication-Results: mx.groups.io; dkim=fail reason="unable to parse pub key" header.i=@intel.com header.s=intel header.b=a/DXlzDm; spf=pass (domain: intel.com, ip: 192.55.52.120, mailfrom: min.m.xu@intel.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1673308594; x=1704844594; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=z2x602rZ+axq+JqLfd2Cq0ektKb114MjUtQ084KMPs0=; b=a/DXlzDmzxEHgUEnyMdoIr+P9qWCaG3flcQ9TEdm2c+jbV+VigQ3I+lx tISx+kix5dSR9P6/QMxFyZQtYOcW77P3nVxys1zYLZ6db2ipuXhmQCXQy rxPuEDvsJGHlhl9b+eaLSMc/Z5bkmHwFFjhYCGEBTNzck7Ko4lHzpGVBE GSsv2BtQ+3cmuOmpMuGgBJH3LlKV5HF3VB4Eo4VgkZcFJ6wP4IOazp5JL axcW/Mv41hbXYjwkHfa74j0uQOYPjzuk8ypg9GLysQL2o7A0gGjgu5opQ 7W1eYuv7D7V491WQKV4ifnsEXYfD08DJsVWW2bALCOQxwE62ZaKHUkXvz Q==; X-IronPort-AV: E=McAfee;i="6500,9779,10585"; a="321715307" X-IronPort-AV: E=Sophos;i="5.96,313,1665471600"; d="scan'208";a="321715307" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Jan 2023 15:56:33 -0800 X-IronPort-AV: E=McAfee;i="6500,9779,10585"; a="689221925" X-IronPort-AV: E=Sophos;i="5.96,313,1665471600"; d="scan'208";a="689221925" Received: from caij-mobl.ccr.corp.intel.com (HELO mxu9-mobl1.ccr.corp.intel.com) ([10.255.29.89]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Jan 2023 15:56:31 -0800 From: "Min Xu" To: devel@edk2.groups.io Cc: Min M Xu , Erdem Aktas , James Bottomley , Jiewen Yao , Gerd Hoffmann , Tom Lendacky , Ryan Afranji Subject: [PATCH V2 2/2] OvmfPkg/CcExitLib: Refactor TDX MmioExit Date: Tue, 10 Jan 2023 07:56:12 +0800 Message-Id: <20230109235612.439-3-min.m.xu@intel.com> X-Mailer: git-send-email 2.29.2.windows.2 In-Reply-To: <20230109235612.439-1-min.m.xu@intel.com> References: <20230109235612.439-1-min.m.xu@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit From: Min M Xu BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4169 The previous TDX MmioExit doesn't handle the Mmio instructions correctly in some scenarios. This patch refactors the implementation to fix the issues. Cc: Erdem Aktas Cc: James Bottomley Cc: Jiewen Yao Cc: Gerd Hoffmann Cc: Tom Lendacky Cc: Ryan Afranji Reported-by: Ryan Afranji Signed-off-by: Min Xu --- OvmfPkg/Library/CcExitLib/CcExitVeHandler.c | 525 ++++++++++++++------ 1 file changed, 363 insertions(+), 162 deletions(-) diff --git a/OvmfPkg/Library/CcExitLib/CcExitVeHandler.c b/OvmfPkg/Library/CcExitLib/CcExitVeHandler.c index 30d547d5fe55..9a803cc38c84 100644 --- a/OvmfPkg/Library/CcExitLib/CcExitVeHandler.c +++ b/OvmfPkg/Library/CcExitLib/CcExitVeHandler.c @@ -13,6 +13,10 @@ #include #include #include +#include "CcInstruction.h" + +#define TDX_MMIO_READ 0 +#define TDX_MMIO_WRITE 1 typedef union { struct { @@ -216,14 +220,15 @@ STATIC VOID EFIAPI TdxDecodeInstruction ( - IN UINT8 *Rip + IN UINT8 *Rip, + IN UINT32 Length ) { UINTN i; DEBUG ((DEBUG_INFO, "TDX: #TD[EPT] instruction (%p):", Rip)); - for (i = 0; i < 15; i++) { - DEBUG ((DEBUG_INFO, "%02x:", Rip[i])); + for (i = 0; i < MIN (15, Length); i++) { + DEBUG ((DEBUG_INFO, "%02x ", Rip[i])); } DEBUG ((DEBUG_INFO, "\n")); @@ -233,52 +238,326 @@ TdxDecodeInstruction ( if ((x)) { \ TdxDecodeInstruction(Rip); \ TdVmCall(TDVMCALL_HALT, 0, 0, 0, 0, 0); \ + CpuDeadLoop (); \ } +/** + * Tdx MMIO access via TdVmcall. + * + * @param MmioSize Size of the MMIO access + * @param ReadOrWrite Read or write operation + * @param GuestPA Guest physical address + * @param Val Pointer to the value which is read or written + + * @retval EFI_SUCCESS Successfully access the mmio + * @retval Others Other errors as indicated + */ STATIC -UINT64 * -EFIAPI -GetRegFromContext ( - IN EFI_SYSTEM_CONTEXT_X64 *Regs, - IN UINTN RegIndex +EFI_STATUS +TdxMmioReadWrite ( + IN UINT32 MmioSize, + IN UINT32 ReadOrWrite, + IN UINT64 GuestPA, + IN UINT64 *Val ) { - switch (RegIndex) { - case 0: return &Regs->Rax; - break; - case 1: return &Regs->Rcx; - break; - case 2: return &Regs->Rdx; - break; - case 3: return &Regs->Rbx; - break; - case 4: return &Regs->Rsp; - break; - case 5: return &Regs->Rbp; - break; - case 6: return &Regs->Rsi; - break; - case 7: return &Regs->Rdi; - break; - case 8: return &Regs->R8; - break; - case 9: return &Regs->R9; + UINT64 TdStatus; + + if ((MmioSize != 1) && (MmioSize != 2) && (MmioSize != 4) && (MmioSize != 8)) { + return EFI_INVALID_PARAMETER; + } + + if (Val == NULL) { + return EFI_INVALID_PARAMETER; + } + + TdStatus = 0; + if (ReadOrWrite == TDX_MMIO_READ) { + TdStatus = TdVmCall (TDVMCALL_MMIO, MmioSize, TDX_MMIO_READ, GuestPA, 0, Val); + } else if (ReadOrWrite == TDX_MMIO_WRITE) { + TdStatus = TdVmCall (TDVMCALL_MMIO, MmioSize, TDX_MMIO_WRITE, GuestPA, *Val, 0); + } else { + return EFI_INVALID_PARAMETER; + } + + if (TdStatus != 0) { + DEBUG ((DEBUG_ERROR, "%a: TdVmcall failed with %llx\n", __FUNCTION__, TdStatus)); + return EFI_ABORTED; + } + + return EFI_SUCCESS; +} + +typedef struct { + UINT8 OpCode; + UINT32 Bytes; + EFI_PHYSICAL_ADDRESS Address; + UINT64 Val; + UINT64 *Register; + UINT32 ReadOrWrite; +} MMIO_EXIT_PARSED_INSTRUCTION; + +/** + * Parse the MMIO instructions. + * + * @param Regs Pointer to the EFI_SYSTEM_CONTEXT_X64 which includes the instructions + * @param InstructionData Pointer to the CC_INSTRUCTION_DATA + * @param ParsedInstruction Pointer to the parsed instruction data + * + * @retval EFI_SUCCESS Successfully parsed the instructions + * @retval Others Other error as indicated + */ +STATIC +EFI_STATUS +ParseMmioExitInstructions ( + IN OUT EFI_SYSTEM_CONTEXT_X64 *Regs, + IN OUT CC_INSTRUCTION_DATA *InstructionData, + OUT MMIO_EXIT_PARSED_INSTRUCTION *ParsedInstruction + ) +{ + EFI_STATUS Status; + UINT8 OpCode; + UINT8 SignByte; + UINT32 Bytes; + EFI_PHYSICAL_ADDRESS Address; + UINT64 Val; + UINT64 *Register; + UINT32 ReadOrWrite; + + Address = 0; + Bytes = 0; + Register = NULL; + Status = EFI_SUCCESS; + Val = 0; + + Status = CcInitInstructionData (InstructionData, NULL, Regs); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "%a: Initialize InstructionData failed! (%r)\n", __FUNCTION__, Status)); + return Status; + } + + OpCode = *(InstructionData->OpCodes); + if (OpCode == TWO_BYTE_OPCODE_ESCAPE) { + OpCode = *(InstructionData->OpCodes + 1); + } + + switch (OpCode) { + // + // MMIO write (MOV reg/memX, regX) + // + case 0x88: + Bytes = 1; + // + // fall through + // + case 0x89: + CcDecodeModRm (Regs, InstructionData); + Bytes = ((Bytes != 0) ? Bytes : + (InstructionData->DataSize == Size16Bits) ? 2 : + (InstructionData->DataSize == Size32Bits) ? 4 : + (InstructionData->DataSize == Size64Bits) ? 8 : + 0); + + if (InstructionData->Ext.ModRm.Mod == 3) { + DEBUG ((DEBUG_ERROR, "%a: Parse Ext.ModRm.Mod error! (OpCode: 0x%x)\n", __FUNCTION__, OpCode)); + return EFI_UNSUPPORTED; + } + + Address = InstructionData->Ext.RmData; + Val = InstructionData->Ext.RegData; + ReadOrWrite = TDX_MMIO_WRITE; + break; - case 10: return &Regs->R10; + + // + // MMIO write (MOV moffsetX, aX) + // + case 0xA2: + Bytes = 1; + // + // fall through + // + case 0xA3: + Bytes = ((Bytes != 0) ? Bytes : + (InstructionData->DataSize == Size16Bits) ? 2 : + (InstructionData->DataSize == Size32Bits) ? 4 : + (InstructionData->DataSize == Size64Bits) ? 8 : + 0); + + InstructionData->ImmediateSize = (UINTN)(1 << InstructionData->AddrSize); + InstructionData->End += InstructionData->ImmediateSize; + CopyMem (&Address, InstructionData->Immediate, InstructionData->ImmediateSize); + + Val = Regs->Rax; + ReadOrWrite = TDX_MMIO_WRITE; break; - case 11: return &Regs->R11; + + // + // MMIO write (MOV reg/memX, immX) + // + case 0xC6: + Bytes = 1; + // + // fall through + // + case 0xC7: + CcDecodeModRm (Regs, InstructionData); + Bytes = ((Bytes != 0) ? Bytes : + (InstructionData->DataSize == Size16Bits) ? 2 : + (InstructionData->DataSize == Size32Bits) ? 4 : + (InstructionData->DataSize == Size64Bits) ? 8 : + 0); + + InstructionData->ImmediateSize = Bytes; + InstructionData->End += Bytes; + + Val = 0; + CopyMem (&Val, InstructionData->Immediate, InstructionData->ImmediateSize); + + Address = InstructionData->Ext.RmData; + ReadOrWrite = TDX_MMIO_WRITE; + break; - case 12: return &Regs->R12; + + // + // MMIO read (MOV regX, reg/memX) + // + case 0x8A: + Bytes = 1; + // + // fall through + // + case 0x8B: + CcDecodeModRm (Regs, InstructionData); + Bytes = ((Bytes != 0) ? Bytes : + (InstructionData->DataSize == Size16Bits) ? 2 : + (InstructionData->DataSize == Size32Bits) ? 4 : + (InstructionData->DataSize == Size64Bits) ? 8 : + 0); + if (InstructionData->Ext.ModRm.Mod == 3) { + // + // NPF on two register operands??? + // + DEBUG ((DEBUG_ERROR, "%a: Parse Ext.ModRm.Mod error! (OpCode: 0x%x)\n", __FUNCTION__, OpCode)); + return EFI_UNSUPPORTED; + } + + Address = InstructionData->Ext.RmData; + ReadOrWrite = TDX_MMIO_READ; + + Register = CcGetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg); + if (Bytes == 4) { + // + // Zero-extend for 32-bit operation + // + *Register = 0; + } + break; - case 13: return &Regs->R13; + + // + // MMIO read (MOV aX, moffsetX) + // + case 0xA0: + Bytes = 1; + // + // fall through + // + case 0xA1: + Bytes = ((Bytes != 0) ? Bytes : + (InstructionData->DataSize == Size16Bits) ? 2 : + (InstructionData->DataSize == Size32Bits) ? 4 : + (InstructionData->DataSize == Size64Bits) ? 8 : + 0); + + InstructionData->ImmediateSize = (UINTN)(1 << InstructionData->AddrSize); + InstructionData->End += InstructionData->ImmediateSize; + + Address = 0; + CopyMem ( + &Address, + InstructionData->Immediate, + InstructionData->ImmediateSize + ); + + if (Bytes == 4) { + // + // Zero-extend for 32-bit operation + // + Regs->Rax = 0; + } + + Register = &Regs->Rax; + ReadOrWrite = TDX_MMIO_READ; + break; - case 14: return &Regs->R14; + + // + // MMIO read w/ zero-extension ((MOVZX regX, reg/memX) + // + case 0xB6: + Bytes = 1; + // + // fall through + // + case 0xB7: + CcDecodeModRm (Regs, InstructionData); + Bytes = (Bytes != 0) ? Bytes : 2; + Address = InstructionData->Ext.RmData; + + Register = CcGetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg); + SetMem (Register, (UINTN)(1 << InstructionData->DataSize), 0); + + ReadOrWrite = TDX_MMIO_READ; + break; - case 15: return &Regs->R15; + + // + // MMIO read w/ sign-extension (MOVSX regX, reg/memX) + // + case 0xBE: + Bytes = 1; + // + // fall through + // + case 0xBF: + CcDecodeModRm (Regs, InstructionData); + Bytes = (Bytes != 0) ? Bytes : 2; + + Address = InstructionData->Ext.RmData; + + if (Bytes == 1) { + UINT8 *Data; + Data = (UINT8 *)&Val; + SignByte = ((*Data & BIT7) != 0) ? 0xFF : 0x00; + } else { + UINT16 *Data; + Data = (UINT16 *)&Val; + SignByte = ((*Data & BIT15) != 0) ? 0xFF : 0x00; + } + + Register = CcGetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg); + SetMem (Register, (UINTN)(1 << InstructionData->DataSize), SignByte); + + ReadOrWrite = TDX_MMIO_READ; + break; + + default: + DEBUG ((DEBUG_ERROR, "%a: Invalid MMIO opcode (%x)\n", __FUNCTION__, OpCode)); + Status = EFI_UNSUPPORTED; + } + + if (!EFI_ERROR (Status)) { + ParsedInstruction->OpCode = OpCode; + ParsedInstruction->Address = Address; + ParsedInstruction->Bytes = Bytes; + ParsedInstruction->Register = Register; + ParsedInstruction->Val = Val; + ParsedInstruction->ReadOrWrite = ReadOrWrite; } - return NULL; + return Status; } /** @@ -290,160 +569,80 @@ GetRegFromContext ( @param[in] Veinfo VE Info @retval 0 Event handled successfully - @return New exception value to propagate **/ STATIC -INTN +UINT64 EFIAPI MmioExit ( IN OUT EFI_SYSTEM_CONTEXT_X64 *Regs, IN TDCALL_VEINFO_RETURN_DATA *Veinfo ) { - UINT64 Status; - UINT32 MmioSize; - UINT32 RegSize; - UINT8 OpCode; - BOOLEAN SeenRex; - UINT64 *Reg; - UINT8 *Rip; - UINT64 Val; - UINT32 OpSize; - MODRM ModRm; - REX Rex; - TD_RETURN_DATA TdReturnData; - UINT8 Gpaw; - UINT64 TdSharedPageMask; + UINT64 TdStatus; + EFI_STATUS Status; + TD_RETURN_DATA TdReturnData; + UINT8 Gpaw; + UINT64 Val; + UINT64 TdSharedPageMask; + CC_INSTRUCTION_DATA InstructionData; + MMIO_EXIT_PARSED_INSTRUCTION ParsedInstruction; - Rip = (UINT8 *)Regs->Rip; - Val = 0; - Rex.Val = 0; - SeenRex = FALSE; - - Status = TdCall (TDCALL_TDINFO, 0, 0, 0, &TdReturnData); - if (Status == TDX_EXIT_REASON_SUCCESS) { + TdStatus = TdCall (TDCALL_TDINFO, 0, 0, 0, &TdReturnData); + if (TdStatus == TDX_EXIT_REASON_SUCCESS) { Gpaw = (UINT8)(TdReturnData.TdInfo.Gpaw & 0x3f); TdSharedPageMask = 1ULL << (Gpaw - 1); } else { - DEBUG ((DEBUG_ERROR, "TDCALL failed with status=%llx\n", Status)); - return Status; + DEBUG ((DEBUG_ERROR, "%a: TDCALL failed with status=%llx\n", __FUNCTION__, TdStatus)); + goto FatalError; } if ((Veinfo->GuestPA & TdSharedPageMask) == 0) { - DEBUG ((DEBUG_ERROR, "EPT-violation #VE on private memory is not allowed!")); - TdVmCall (TDVMCALL_HALT, 0, 0, 0, 0, 0); - CpuDeadLoop (); + DEBUG ((DEBUG_ERROR, "%a: EPT-violation #VE on private memory is not allowed!", __FUNCTION__)); + goto FatalError; } - // - // Default to 32bit transfer - // - OpSize = 4; + Status = ParseMmioExitInstructions (Regs, &InstructionData, &ParsedInstruction); + if (EFI_ERROR (Status)) { + goto FatalError; + } + + if (Veinfo->GuestPA != (ParsedInstruction.Address | TdSharedPageMask)) { + DEBUG (( + DEBUG_ERROR, + "%a: Address is not correct! (%d: 0x%llx != 0x%llx)\n", + __FUNCTION__, + ParsedInstruction.OpCode, + Veinfo->GuestPA, + ParsedInstruction.Address + )); + goto FatalError; + } - do { - OpCode = *Rip++; - if (OpCode == 0x66) { - OpSize = 2; - } else if ((OpCode == 0x64) || (OpCode == 0x65) || (OpCode == 0x67)) { - continue; - } else if ((OpCode >= 0x40) && (OpCode <= 0x4f)) { - SeenRex = TRUE; - Rex.Val = OpCode; - } else { - break; + if (ParsedInstruction.ReadOrWrite == TDX_MMIO_WRITE ) { + Status = TdxMmioReadWrite (ParsedInstruction.Bytes, TDX_MMIO_WRITE, Veinfo->GuestPA, &ParsedInstruction.Val); + } else { + Val = 0; + Status = TdxMmioReadWrite (ParsedInstruction.Bytes, TDX_MMIO_READ, Veinfo->GuestPA, &Val); + if (!EFI_ERROR (Status)) { + CopyMem (ParsedInstruction.Register, &Val, ParsedInstruction.Bytes); } - } while (TRUE); - - // - // We need to have at least 2 more bytes for this instruction - // - TDX_DECODER_BUG_ON (((UINT64)Rip - Regs->Rip) > 13); - - OpCode = *Rip++; - // - // Two-byte opecode, get next byte - // - if (OpCode == 0x0F) { - OpCode = *Rip++; - } - - switch (OpCode) { - case 0x88: - case 0x8A: - case 0xB6: - MmioSize = 1; - break; - case 0xB7: - MmioSize = 2; - break; - default: - MmioSize = Rex.Bits.W ? 8 : OpSize; - break; - } - - /* Punt on AH/BH/CH/DH unless it shows up. */ - ModRm.Val = *Rip++; - TDX_DECODER_BUG_ON (MmioSize == 1 && ModRm.Bits.Reg > 4 && !SeenRex && OpCode != 0xB6); - Reg = GetRegFromContext (Regs, ModRm.Bits.Reg | ((int)Rex.Bits.R << 3)); - TDX_DECODER_BUG_ON (!Reg); - - if (ModRm.Bits.Rm == 4) { - ++Rip; /* SIB byte */ } - if ((ModRm.Bits.Mod == 2) || ((ModRm.Bits.Mod == 0) && (ModRm.Bits.Rm == 5))) { - Rip += 4; /* DISP32 */ - } else if (ModRm.Bits.Mod == 1) { - ++Rip; /* DISP8 */ - } - - switch (OpCode) { - case 0x88: - case 0x89: - CopyMem ((void *)&Val, Reg, MmioSize); - Status = TdVmCall (TDVMCALL_MMIO, MmioSize, 1, Veinfo->GuestPA, Val, 0); - break; - case 0xC7: - CopyMem ((void *)&Val, Rip, OpSize); - Status = TdVmCall (TDVMCALL_MMIO, MmioSize, 1, Veinfo->GuestPA, Val, 0); - Rip += OpSize; - default: - // - // 32-bit write registers are zero extended to the full register - // Hence 'MOVZX r[32/64], r/m16' is - // hardcoded to reg size 8, and the straight MOV case has a reg - // size of 8 in the 32-bit read case. - // - switch (OpCode) { - case 0xB6: - RegSize = Rex.Bits.W ? 8 : OpSize; - break; - case 0xB7: - RegSize = 8; - break; - default: - RegSize = MmioSize == 4 ? 8 : MmioSize; - break; - } - - Status = TdVmCall (TDVMCALL_MMIO, MmioSize, 0, Veinfo->GuestPA, 0, &Val); - if (Status == 0) { - ZeroMem (Reg, RegSize); - CopyMem (Reg, (void *)&Val, MmioSize); - } - } - - if (Status == 0) { - TDX_DECODER_BUG_ON (((UINT64)Rip - Regs->Rip) > 15); - + if (!EFI_ERROR (Status)) { // // We change instruction length to reflect true size so handler can // bump rip // - Veinfo->ExitInstructionLength = (UINT32)((UINT64)Rip - Regs->Rip); + Veinfo->ExitInstructionLength = (UINT32)(CcInstructionLength (&InstructionData)); + TdxDecodeInstruction ((UINT8 *)Regs->Rip, Veinfo->ExitInstructionLength); } - return Status; + return 0; + +FatalError: + TdVmCall (TDVMCALL_HALT, 0, 0, 0, 0, 0); + CpuDeadLoop (); + return 0; } /** @@ -479,6 +678,7 @@ CcExitHandleVe ( if (Status != 0) { DEBUG ((DEBUG_ERROR, "#VE happened. TDGETVEINFO failed with Status = 0x%llx\n", Status)); TdVmCall (TDVMCALL_HALT, 0, 0, 0, 0, 0); + CpuDeadLoop (); } switch (ReturnData.VeInfo.ExitReason) { @@ -571,6 +771,7 @@ CcExitHandleVe ( )); TdVmCall (TDVMCALL_HALT, 0, 0, 0, 0, 0); + CpuDeadLoop (); } SystemContext.SystemContextX64->Rip += ReturnData.VeInfo.ExitInstructionLength; -- 2.29.2.windows.2