From e05132aaa091701b457ebcd725484ee5751759da Mon Sep 17 00:00:00 2001 From: Min M Xu Date: Tue, 17 Jan 2023 15:43:30 +0800 Subject: [PATCH] OvmfPkg/CcExitLib: Refactor TDX MmioExit 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 Reviewed-by: Jiewen Yao Acked-by: Gerd Hoffmann --- OvmfPkg/Library/CcExitLib/CcExitVeHandler.c | 544 ++++++++++++++------ 1 file changed, 381 insertions(+), 163 deletions(-) diff --git a/OvmfPkg/Library/CcExitLib/CcExitVeHandler.c b/OvmfPkg/Library/CcExitLib/CcExitVeHandler.c index 30d547d5fe..b8979ec2c0 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,339 @@ 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)) { + DEBUG ((DEBUG_ERROR, "%a: Invalid MmioSize - %d\n", __FUNCTION__, MmioSize)); + 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 (Register == NULL) { + return EFI_ABORTED; + } + + 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); + if (Register == NULL) { + return EFI_ABORTED; + } + + 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); + if (Register == NULL) { + return EFI_ABORTED; + } + + 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; } - return NULL; + if (!EFI_ERROR (Status)) { + ParsedInstruction->OpCode = OpCode; + ParsedInstruction->Address = Address; + ParsedInstruction->Bytes = Bytes; + ParsedInstruction->Register = Register; + ParsedInstruction->Val = Val; + ParsedInstruction->ReadOrWrite = ReadOrWrite; + } + + return Status; } /** @@ -290,160 +582,84 @@ 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; - - 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) { + UINT64 TdStatus; + EFI_STATUS Status; + TD_RETURN_DATA TdReturnData; + UINT8 Gpaw; + UINT64 Val; + UINT64 TdSharedPageMask; + CC_INSTRUCTION_DATA InstructionData; + MMIO_EXIT_PARSED_INSTRUCTION ParsedInstruction; + + 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; - - 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; - } - } 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; + Status = ParseMmioExitInstructions (Regs, &InstructionData, &ParsedInstruction); + if (EFI_ERROR (Status)) { + goto FatalError; } - /* 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 (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; } - if ((ModRm.Bits.Mod == 2) || ((ModRm.Bits.Mod == 0) && (ModRm.Bits.Rm == 5))) { - Rip += 4; /* DISP32 */ - } else if (ModRm.Bits.Mod == 1) { - ++Rip; /* DISP8 */ + if (ParsedInstruction.ReadOrWrite == TDX_MMIO_WRITE ) { + Status = TdxMmioReadWrite (ParsedInstruction.Bytes, TDX_MMIO_WRITE, Veinfo->GuestPA, &ParsedInstruction.Val); + } else if (ParsedInstruction.ReadOrWrite == TDX_MMIO_READ) { + Val = 0; + Status = TdxMmioReadWrite (ParsedInstruction.Bytes, TDX_MMIO_READ, Veinfo->GuestPA, &Val); + if (!EFI_ERROR (Status)) { + CopyMem (ParsedInstruction.Register, &Val, ParsedInstruction.Bytes); + } + } else { + goto FatalError; } - 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 (EFI_ERROR (Status)) { + goto FatalError; } - if (Status == 0) { - TDX_DECODER_BUG_ON (((UINT64)Rip - Regs->Rip) > 15); + // + // We change instruction length to reflect true size so handler can + // bump rip + // + Veinfo->ExitInstructionLength = (UINT32)(CcInstructionLength (&InstructionData)); + TdxDecodeInstruction ((UINT8 *)Regs->Rip, Veinfo->ExitInstructionLength); - // - // We change instruction length to reflect true size so handler can - // bump rip - // - Veinfo->ExitInstructionLength = (UINT32)((UINT64)Rip - Regs->Rip); - } + return 0; - return Status; +FatalError: + TdVmCall (TDVMCALL_HALT, 0, 0, 0, 0, 0); + CpuDeadLoop (); + return 0; } /** @@ -479,6 +695,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 +788,7 @@ CcExitHandleVe ( )); TdVmCall (TDVMCALL_HALT, 0, 0, 0, 0, 0); + CpuDeadLoop (); } SystemContext.SystemContextX64->Rip += ReturnData.VeInfo.ExitInstructionLength; -- 2.39.2