Re: [PATCH 1/3] OvfmPkg/VmgExitLib: Properly decode MMIO MOVZX and MOVSX opcodes


Lendacky, Thomas
 

On 4/22/21 12:28 AM, Laszlo Ersek wrote:
On 04/21/21 00:54, Lendacky, Thomas wrote:
From: Tom Lendacky <thomas.lendacky@amd.com>

BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3345&;data=04%7C01%7Cthomas.lendacky%40amd.com%7C22bf3a3ae9cb4421e93208d9054f79c8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546661229697941%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=1EmUDf%2FfuCuu%2BkXPZijzatfliplMhKEQH8kiZ9Z8ZF0%3D&amp;reserved=0

The MOVZX and MOVSX instructions use the ModRM byte in the instruction,
but the instruction decoding support was not decoding it. This resulted
in invalid decoding and failing of the MMIO operation. Also, when
performing the zero-extend or sign-extend operation, the memory operation
should be using the size, and not the size enumeration value.

Add the ModRM byte decoding for the MOVZX and MOVSX opcodes and use the
true data size to perform the extend operations. Additionally, add a
DEBUG statement identifying the MMIO address being flagged as encrypted
during the MMIO address validation.

Fixes: c45f678a1ea2080344e125dc55b14e4b9f98483d
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
index 24259060fd65..273f36499988 100644
--- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
+++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
@@ -643,6 +643,7 @@ ValidateMmioMemory (
//
// Any state other than unencrypted is an error, issue a #GP.
//
+ DEBUG ((DEBUG_INFO, "MMIO using encrypted memory: %lx\n", MemoryAddress));
GpEvent.Uint64 = 0;
GpEvent.Elements.Vector = GP_EXCEPTION;
GpEvent.Elements.Type = GHCB_EVENT_INJECTION_TYPE_EXCEPTION;
(1) This can potentially generate a large number of debug messages;
please use the DEBUG_VERBOSE log mask.
Actually, you will see this only once since the code will propagate a GP
and the guest will terminate in this situation.


(2) "MemoryAddress" has type UINTN, but %lx takes UINT64. Given that
this is X64-only code, functionally there is no bug, but it's still
cleaner to pass "(UINT64)MemoryAddress" to %lx.
Will do.

Thanks,
Tom


With that:

Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo


@@ -817,6 +818,7 @@ MmioExit (
// fall through
//
case 0xB7:
+ DecodeModRm (Regs, InstructionData);
Bytes = (Bytes != 0) ? Bytes : 2;

Status = ValidateMmioMemory (Ghcb, InstructionData->Ext.RmData, Bytes);
@@ -835,7 +837,7 @@ MmioExit (
}

Register = GetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg);
- SetMem (Register, InstructionData->DataSize, 0);
+ SetMem (Register, (UINTN) (1 << InstructionData->DataSize), 0);
CopyMem (Register, Ghcb->SharedBuffer, Bytes);
break;

@@ -848,6 +850,7 @@ MmioExit (
// fall through
//
case 0xBF:
+ DecodeModRm (Regs, InstructionData);
Bytes = (Bytes != 0) ? Bytes : 2;

Status = ValidateMmioMemory (Ghcb, InstructionData->Ext.RmData, Bytes);
@@ -878,7 +881,7 @@ MmioExit (
}

Register = GetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg);
- SetMem (Register, InstructionData->DataSize, SignByte);
+ SetMem (Register, (UINTN) (1 << InstructionData->DataSize), SignByte);
CopyMem (Register, Ghcb->SharedBuffer, Bytes);
break;

Join devel@edk2.groups.io to automatically receive all group messages.