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

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

On 04/21/21 00:54, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345
> 
> 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.

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

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