Re: [edk2-devel] [PATCH 1/3] OvmfPkg/PlatformInitLib: Detect FlashNvVarStore before validate it

["Lendacky, Thomas via groups.io" <thomas.lendacky=amd.com@groups.io>]

On 7/14/24 07:24, wojiaohanliyang@163.com wrote:
> From: hanliyang <wojiaohanliyang@163.com>
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4807
> 
> The commit 4f173db8b45b ("OvmfPkg/PlatformInitLib: Add functions for
> EmuVariableNvStore") rename the function from TdxValidateCfv to
> PlatformValidateNvVarStore.
> 
> PlatformValidateNvVarStore is placed in the PlatformInitLib and is used
> in the case that OVMF is launched with -bios parameter and to validate
> the integrity of FlashNvVarStore. But if we launch a VM without
> FlashNvVarStore, the PlatformValidateNvVarStore will fail to validate
> the integrity and will trigger ASSERT (FALSE) in
> PlatformInitEmuVariableNvStore.
> 
> In order to prevent calls to PlatformValidateNvVarStore in the case lack
> of FlashNvVarStore, we should detect FlashNvVarStore before calls to
> PlatformValidateNvVarStore. If fail to detect FlashNvVarStore, we should
> return don't initialize the EmuVariableNvStore, otherwise calls to
> PlatformValidateNvVarStore and initialize the EmuVariableNvStore when
> succeed to validate the integrity of NvVarStore.
> 

While Secure Boot isn't supported at the moment for SEV-ES / SEV-SNP,
this will cause a boot failure for those types of VMs should it be
enabled.

SEV-ES results in:

Invalid MMIO opcode (AF)
ASSERT [SecMain] /root/kernels/ovmf-build-X64/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c(498): ((BOOLEAN)(0==1))

while SEV-SNP just terminates with an error in Qemu.

I haven't looked into what the cause is at this time.

Thanks,
Tom

> Fixes: 4f173db8b45b ("OvmfPkg/PlatformInitLib: Add functions for EmuVariableNvStore")
> Signed-off-by: hanliyang <wojiaohanliyang@163.com>
> ---
>  OvmfPkg/Library/PlatformInitLib/Platform.c    | 47 +++++++++++++++++++
>  .../PlatformInitLib/PlatformInitLib.inf       |  1 +
>  2 files changed, 48 insertions(+)
> 
> diff --git a/OvmfPkg/Library/PlatformInitLib/Platform.c b/OvmfPkg/Library/PlatformInitLib/Platform.c
> index f48bf16ae3..0a720a4c2c 100644
> --- a/OvmfPkg/Library/PlatformInitLib/Platform.c
> +++ b/OvmfPkg/Library/PlatformInitLib/Platform.c
> @@ -895,6 +895,16 @@ PlatformReserveEmuVariableNvStore (
>    return VariableStore;
>  }
>  
> +#define WRITE_BYTE_CMD           0x10
> +#define BLOCK_ERASE_CMD          0x20
> +#define CLEAR_STATUS_CMD         0x50
> +#define READ_STATUS_CMD          0x70
> +#define READ_DEVID_CMD           0x90
> +#define BLOCK_ERASE_CONFIRM_CMD  0xd0
> +#define READ_ARRAY_CMD           0xff
> +
> +#define CLEARED_ARRAY_STATUS  0x00
> +
>  /**
>   When OVMF is lauched with -bios parameter, UEFI variables will be
>   partially emulated, and non-volatile variables may lose their contents
> @@ -928,6 +938,43 @@ PlatformInitEmuVariableNvStore (
>    Size = (UINT32)PcdGet32 (PcdFlashNvStorageVariableSize);
>    ASSERT (Size < EmuVariableNvStoreSize);
>  
> +  //
> +  // If launch a VM without OvmfFlashNvStorage device, then we'll fail
> +  // to check the integrity of NvVarStore and trigger ASSERT (FALSE).
> +  // So, we should detect the OvmfFlashNvStorage before calls to
> +  // PlatformValidateNvVarStore(). If fail to detect OvmfFlashNvStorage,
> +  // we should return and don't initialize the EmuVariableNvStore,
> +  // otherwise calls to PlatformValidateNvVarStore() and initialize the
> +  // EmuVariableNvStore when succeed to check the integrity of
> +  // NvVarStore.
> +  //
> +  // This method to detect the OvmfFlashNvStorage here references
> +  // OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c.
> +  //
> +  volatile UINT8  *Ptr;
> +
> +  UINTN  BlockSize;
> +  UINTN  Offset;
> +  UINT8  ProbeUint8;
> +
> +  BlockSize = PcdGet32 (PcdOvmfFirmwareBlockSize);
> +
> +  for (Offset = 0; Offset < BlockSize; Offset++) {
> +    Ptr        = Base + Offset;
> +    ProbeUint8 = *Ptr;
> +    if ((ProbeUint8 != CLEAR_STATUS_CMD) &&
> +        (ProbeUint8 != READ_STATUS_CMD) &&
> +        (ProbeUint8 != CLEARED_ARRAY_STATUS))
> +    {
> +      break;
> +    }
> +  }
> +
> +  if (Offset >= BlockSize) {
> +    DEBUG ((DEBUG_INFO, "OvmfFlashNvStorage: Failed to find probe location\n"));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
>    if (!PlatformValidateNvVarStore (Base, PcdGet32 (PcdCfvRawDataSize))) {
>      ASSERT (FALSE);
>      return EFI_INVALID_PARAMETER;
> diff --git a/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf b/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf
> index 21e6efa5e0..b7d5e63dcd 100644
> --- a/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf
> +++ b/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf
> @@ -104,6 +104,7 @@
>    gUefiOvmfPkgTokenSpaceGuid.PcdTdxAcceptPageSize
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageVariableBase
>    gUefiOvmfPkgTokenSpaceGuid.PcdCfvRawDataSize
> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareBlockSize
>  
>  [FeaturePcd]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119935): https://edk2.groups.io/g/devel/message/119935
Mute This Topic: https://groups.io/mt/107212942/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-