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

[韩里洋 <wojiaohanliyang@163.com>]

[-- Attachment #1: Type: text/plain, Size: 6763 bytes --]

Hi Tom,




Thank you for your response.

In fact, I'm unable to proceed with the development of the fix patch locally as I don't have a SEV-SNP hardware for experimentation. However, it has proven to be crucial for effectively testing and completing the patch.

Given your expertise and potentially available hardware, would your team be able to take over the fixing of this issue? (bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=4807 )

Thank you very much for your time and consideration.

Best regards,

hanliyang

















At 2024-07-15 22:32:03, "Lendacky, Thomas via groups.io" <thomas.lendacky=amd.com@groups.io> wrote:
>On 7/15/24 09:15, Tom Lendacky wrote:
>> 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.
>
>My bad, that was before this patch was applied. After applying this
>patch, I receive:
>
>SEV-ES:
>
>MMIO using encrypted memory: FFC00000
>!!!! X64 Exception Type - 0D(#GP - General Protection)  CPU Apic ID - 00000000 !!!!
>
>SEV-SNP just terminates as before.
>
>So maybe you need patch #3 to be the first patch in order to
>maintain git bisection.
>
>But after applying all 3 patches, SEV-SNP self terminates for an
>unsupported #VC error code - 0x404. 0x404 is from accessing a page
>as encrypted, but it has not been validated.
>
>Thanks,
>Tom
>
>> 
>> 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 (#119945): https://edk2.groups.io/g/devel/message/119945
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]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 12420 bytes --]