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

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

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&amp;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;
>>  
>>
>