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

["Laszlo Ersek" <lersek@redhat.com>]

On 04/22/21 15:35, Tom Lendacky wrote:
> 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.

Ugh, sorry, I must have completely lost track of the context here. I
apologize.

In that case however, it should be DEBUG_ERROR.

Thanks,
Laszlo

> 
>>
>> (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;
>>>  
>>>
>>
>