* [edk2-devel] [PATCH 1/3] OvmfPkg/PlatformInitLib: Detect FlashNvVarStore before validate it
2024-07-14 12:24 [edk2-devel] [PATCH 0/3] Fix boot failure when use secure boot supported (-D SECURE_BOOT_ENABLE=TRUE) OVMF packages wojiaohanliyang
@ 2024-07-14 12:24 ` wojiaohanliyang
2024-07-15 14:15 ` Lendacky, Thomas via groups.io
2024-07-14 12:24 ` [edk2-devel] [PATCH 2/3] OvmfPkg/PlatformInitLib: Init the EmuVariableNvStore before copy data wojiaohanliyang
2024-07-14 12:24 ` [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Update mapping of FlashNvVarStore before validate it wojiaohanliyang
2 siblings, 1 reply; 11+ messages in thread
From: wojiaohanliyang @ 2024-07-14 12:24 UTC (permalink / raw)
To: devel
Cc: erdemaktas, jejb, jiewen.yao, min.m.xu, thomas.lendacky, kraxel,
hanliyang
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.
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
--
2.25.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119927): https://edk2.groups.io/g/devel/message/119927
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [edk2-devel] [PATCH 1/3] OvmfPkg/PlatformInitLib: Detect FlashNvVarStore before validate it
2024-07-14 12:24 ` [edk2-devel] [PATCH 1/3] OvmfPkg/PlatformInitLib: Detect FlashNvVarStore before validate it wojiaohanliyang
@ 2024-07-15 14:15 ` Lendacky, Thomas via groups.io
2024-07-15 14:32 ` Lendacky, Thomas via groups.io
0 siblings, 1 reply; 11+ messages in thread
From: Lendacky, Thomas via groups.io @ 2024-07-15 14:15 UTC (permalink / raw)
To: wojiaohanliyang, devel; +Cc: erdemaktas, jejb, jiewen.yao, min.m.xu, kraxel
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [edk2-devel] [PATCH 1/3] OvmfPkg/PlatformInitLib: Detect FlashNvVarStore before validate it
2024-07-15 14:15 ` Lendacky, Thomas via groups.io
@ 2024-07-15 14:32 ` Lendacky, Thomas via groups.io
2024-07-17 2:30 ` 韩里洋
0 siblings, 1 reply; 11+ messages in thread
From: Lendacky, Thomas via groups.io @ 2024-07-15 14:32 UTC (permalink / raw)
To: wojiaohanliyang, devel; +Cc: erdemaktas, jejb, jiewen.yao, min.m.xu, kraxel
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 (#119936): https://edk2.groups.io/g/devel/message/119936
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [edk2-devel] [PATCH 1/3] OvmfPkg/PlatformInitLib: Detect FlashNvVarStore before validate it
2024-07-15 14:32 ` Lendacky, Thomas via groups.io
@ 2024-07-17 2:30 ` 韩里洋
2024-07-19 0:57 ` Lendacky, Thomas via groups.io
0 siblings, 1 reply; 11+ messages in thread
From: 韩里洋 @ 2024-07-17 2:30 UTC (permalink / raw)
To: devel, thomas.lendacky; +Cc: erdemaktas, jejb, jiewen.yao, min.m.xu, kraxel
[-- 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 --]
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [edk2-devel] [PATCH 1/3] OvmfPkg/PlatformInitLib: Detect FlashNvVarStore before validate it
2024-07-17 2:30 ` 韩里洋
@ 2024-07-19 0:57 ` Lendacky, Thomas via groups.io
2024-07-19 7:35 ` Gerd Hoffmann
0 siblings, 1 reply; 11+ messages in thread
From: Lendacky, Thomas via groups.io @ 2024-07-19 0:57 UTC (permalink / raw)
To: 韩里洋, devel
Cc: erdemaktas, jejb, jiewen.yao, min.m.xu, kraxel
On 7/16/24 21:30, 韩里洋 wrote:
> 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 )
Secure Boot is not supported under SEV-ES and SEV-SNP because SMM is
required in order for Secure Boot to be secure. And SMM is not supported
under SEV-ES and SEV-SNP because the hypervisor is not allowed to alter
the vCPU register state that is needed to use SMM.
Thanks,
Tom
>
> Thank you very much for your time and consideration.
>
> Best regards,
>
> hanliyang
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119977): https://edk2.groups.io/g/devel/message/119977
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH 1/3] OvmfPkg/PlatformInitLib: Detect FlashNvVarStore before validate it
2024-07-19 0:57 ` Lendacky, Thomas via groups.io
@ 2024-07-19 7:35 ` Gerd Hoffmann
2024-08-01 23:47 ` Andrew Fish via groups.io
0 siblings, 1 reply; 11+ messages in thread
From: Gerd Hoffmann @ 2024-07-19 7:35 UTC (permalink / raw)
To: Tom Lendacky
Cc: 韩里洋, devel, erdemaktas, jejb, jiewen.yao,
min.m.xu
On Thu, Jul 18, 2024 at 07:57:27PM GMT, Tom Lendacky wrote:
> On 7/16/24 21:30, 韩里洋 wrote:
> > 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 )
>
> Secure Boot is not supported under SEV-ES and SEV-SNP because SMM is
> required in order for Secure Boot to be secure.
The other option is initializing the variable store from ROM on each
boot. Which implies there are no persistent EFI variables, which has
its own set of drawbacks. But this is what the IntelTdx build is doing
and AmdSev should be able to do this too.
take care,
Gerd
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119980): https://edk2.groups.io/g/devel/message/119980
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH 1/3] OvmfPkg/PlatformInitLib: Detect FlashNvVarStore before validate it
2024-07-19 7:35 ` Gerd Hoffmann
@ 2024-08-01 23:47 ` Andrew Fish via groups.io
0 siblings, 0 replies; 11+ messages in thread
From: Andrew Fish via groups.io @ 2024-08-01 23:47 UTC (permalink / raw)
To: edk2-devel-groups-io, Gerd Hoffmann
Cc: Tom Lendacky, 韩里洋, Erdem Aktas, jejb,
Yao, Jiewen, min.m.xu
[-- Attachment #1: Type: text/plain, Size: 1705 bytes --]
> On Jul 19, 2024, at 12:35 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Thu, Jul 18, 2024 at 07:57:27PM GMT, Tom Lendacky wrote:
>> On 7/16/24 21:30, 韩里洋 wrote:
>>> 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 )
>>
>> Secure Boot is not supported under SEV-ES and SEV-SNP because SMM is
>> required in order for Secure Boot to be secure.
>
> The other option is initializing the variable store from ROM on each
> boot. Which implies there are no persistent EFI variables, which has
> its own set of drawbacks. But this is what the IntelTdx build is doing
> and AmdSev should be able to do this too.
>
Seems like you might be able to just overwrite the secure boot related variables on every boot to a hard coded value. You could have PCDs for the default values of the variables.
Thanks,
Andrew Fish
> take care,
> Gerd
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#120200): https://edk2.groups.io/g/devel/message/120200
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: 9801 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* [edk2-devel] [PATCH 2/3] OvmfPkg/PlatformInitLib: Init the EmuVariableNvStore before copy data
2024-07-14 12:24 [edk2-devel] [PATCH 0/3] Fix boot failure when use secure boot supported (-D SECURE_BOOT_ENABLE=TRUE) OVMF packages wojiaohanliyang
2024-07-14 12:24 ` [edk2-devel] [PATCH 1/3] OvmfPkg/PlatformInitLib: Detect FlashNvVarStore before validate it wojiaohanliyang
@ 2024-07-14 12:24 ` wojiaohanliyang
2024-07-14 12:24 ` [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Update mapping of FlashNvVarStore before validate it wojiaohanliyang
2 siblings, 0 replies; 11+ messages in thread
From: wojiaohanliyang @ 2024-07-14 12:24 UTC (permalink / raw)
To: devel
Cc: erdemaktas, jejb, jiewen.yao, min.m.xu, thomas.lendacky, kraxel,
hanliyang
From: hanliyang <wojiaohanliyang@163.com>
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4807
In the case launch with just OVMF.fd, if we just init part of the
EmuVariableNvStore, then EmuVariableFvbRuntimeDxe will skip the
initialize process of the EmuVariableNvStore and the Ftw (Fault
Tolerant Write) part of the EmuVariableNvStore will not be
initialized before the Ftw part is accessed. When we launch a SEV
guest, the FaultTolerantWriteDxe will get scrambled data when read
Ftw part of the EmuVariableNvStore, the FaultToleranteWriteDxe
access address specified by the scrambled data will cause invalid
address access and crash. The crash message is shown as below.
Loading driver at 0x000BDB92000 EntryPoint=0x000BDB95EF4 FaultTolerantWriteDxe.efi
InstallProtocolInterface: BC62157E-3E33-4FEC-9920-2D3B36D750DF BDE01D98
ProtectUefiImageCommon - 0xBDE01040
- 0x00000000BDB92000 - 0x0000000000005B00
Ftw: FtwWorkSpaceLba - 0x40, WorkBlockSize - 0x1000, FtwWorkSpaceBase - 0x0
Ftw: FtwSpareLba - 0x42, SpareBlockSize - 0x1000
Ftw: NumberOfWorkBlock - 0x1, FtwWorkBlockLba - 0x40
Ftw: WorkSpaceLbaInSpare - 0x0, WorkSpaceBaseInSpare - 0x0
Ftw: Remaining work space size - FE0
!!!! X64 Exception Type - 0D(#GP - General Protection) CPU Apic ID - 00000000 !!!!
ExceptionData - 0000000000000000
RIP - 00000000BDB92459, CS - 0000000000000038, RFLAGS - 0000000000010286
RAX - 587E3201A019FB0C, RCX - 587E3200E238F994, RDX - 0000000000000001
RBX - 00000000BDE10018, RSP - 00000000BFB79AD8, RBP - 0000000000000FE0
RSI - 00000000BDE100A8, RDI - 00000000BDE10128
R8 - D4642A9DFB7C79BE, R9 - 00000000000003F8, R10 - 00000000BDB96602
R11 - 0000000000000002, R12 - 00000000BDE100A0, R13 - 0000000000000000
R14 - 0000000000000001, R15 - 00000000BFBA76C0
DS - 0000000000000030, ES - 0000000000000030, FS - 0000000000000030
GS - 0000000000000030, SS - 0000000000000030
CR0 - 0000000080010033, CR2 - 0000000000000000, CR3 - 00000000BF801000
CR4 - 0000000000000668, CR8 - 0000000000000000
DR0 - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000
DR3 - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400
GDTR - 00000000BF5DC000 0000000000000047, LDTR - 0000000000000000
IDTR - 00000000BEF0C018 0000000000000FFF, TR - 0000000000000000
FXSAVE_STATE - 00000000BFB79730
!!!! Find image based on IP(0xBDB92459) /dev/shm/edk2/Build/OvmfX64/DEBUG_GCC5/X64/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe/DEBUG/FaultTolerantWriteDxe.dll (ImageBase=00000000BDB92000, EntryPoint=00000000BDB95EF4) !!!!
Fixes: 4f173db8b45b ("OvmfPkg/PlatformInitLib: Add functions for EmuVariableNvStore")
Signed-off-by: hanliyang <wojiaohanliyang@163.com>
---
OvmfPkg/Library/PlatformInitLib/Platform.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/OvmfPkg/Library/PlatformInitLib/Platform.c b/OvmfPkg/Library/PlatformInitLib/Platform.c
index 0a720a4c2c..5dbc5506f4 100644
--- a/OvmfPkg/Library/PlatformInitLib/Platform.c
+++ b/OvmfPkg/Library/PlatformInitLib/Platform.c
@@ -905,6 +905,8 @@ PlatformReserveEmuVariableNvStore (
#define CLEARED_ARRAY_STATUS 0x00
+#define ERASED_UINT8 0xff
+
/**
When OVMF is lauched with -bios parameter, UEFI variables will be
partially emulated, and non-volatile variables may lose their contents
@@ -982,6 +984,24 @@ PlatformInitEmuVariableNvStore (
DEBUG ((DEBUG_INFO, "Init EmuVariableNvStore with the content in FlashNvStorage\n"));
+ //
+ // Init the whole EmuVariableNvStore before copy the content from
+ // FlashNvStorage to the EmuVariableNvStore.
+ //
+ // In the case launch with just OVMF.fd, if we just init part of the
+ // EmuVariableNvStore, then EmuVariableFvbRuntimeDxe will skip the
+ // initialize process of the EmuVariableNvStore and the Ftw (Fault
+ // Tolerant Write) part of the EmuVariableNvStore will not be
+ // initialized before the Ftw part is accessed. When we launch a SEV
+ // guest, the FaultTolerantWriteDxe will get scrambled data when read
+ // Ftw part of the EmuVariableNvStore, the FaultToleranteWriteDxe
+ // access address specified by the scrambled data will cause invalid
+ // address access and crash.
+ //
+ // The method to init EmuVariableNvStore here references
+ // OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c.
+ //
+ SetMem (EmuVariableNvStore, EmuVariableNvStoreSize, ERASED_UINT8);
CopyMem (EmuVariableNvStore, Base, Size);
return EFI_SUCCESS;
--
2.25.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119928): https://edk2.groups.io/g/devel/message/119928
Mute This Topic: https://groups.io/mt/107212943/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 11+ messages in thread* [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Update mapping of FlashNvVarStore before validate it
2024-07-14 12:24 [edk2-devel] [PATCH 0/3] Fix boot failure when use secure boot supported (-D SECURE_BOOT_ENABLE=TRUE) OVMF packages wojiaohanliyang
2024-07-14 12:24 ` [edk2-devel] [PATCH 1/3] OvmfPkg/PlatformInitLib: Detect FlashNvVarStore before validate it wojiaohanliyang
2024-07-14 12:24 ` [edk2-devel] [PATCH 2/3] OvmfPkg/PlatformInitLib: Init the EmuVariableNvStore before copy data wojiaohanliyang
@ 2024-07-14 12:24 ` wojiaohanliyang
2 siblings, 0 replies; 11+ messages in thread
From: wojiaohanliyang @ 2024-07-14 12:24 UTC (permalink / raw)
To: devel
Cc: erdemaktas, jejb, jiewen.yao, min.m.xu, thomas.lendacky, kraxel,
hanliyang
From: hanliyang <wojiaohanliyang@163.com>
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4807
In the case of launch a SEV-ES VM with just OVMF_CODE.fd, the validation
process in PlatformValidateNvVarStore will trigger MMIO NPF, and the #VC
handler will detect that mmio access is invalid because the mmio address
range of FlashNvVarStore is mapped as encrypted.
In the case of launch a SEV VM with both OVMF_CODE.fd and OVMF_VARS.fd,
PlatformValidateNvVarStore will fail to validate FlashNvVarStore because
the mapping of FlashNvVarStore address range is encrypted in the guest
but the corresponding data in system physical memory was not encrypted
by guest key.
We should map FlashNvVarStore address range as unencrypted for the above
cases.
Fixes: 4f173db8b45b ("OvmfPkg/PlatformInitLib: Add functions for EmuVariableNvStore")
Signed-off-by: hanliyang <wojiaohanliyang@163.com>
---
OvmfPkg/PlatformPei/AmdSev.c | 105 ++++++++++++++++++++++++++++
OvmfPkg/PlatformPei/Platform.c | 6 ++
OvmfPkg/PlatformPei/Platform.h | 6 ++
OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
4 files changed, 118 insertions(+)
diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
index 8562787035..89f4c02b6a 100644
--- a/OvmfPkg/PlatformPei/AmdSev.c
+++ b/OvmfPkg/PlatformPei/AmdSev.c
@@ -575,3 +575,108 @@ SevInitializeRam (
);
}
}
+
+/**
+ Prepared for FlashNvVarStore access.
+
+ **/
+VOID
+SevFlashNvVarStoreUpdateMapping (
+ IN UINTN NvVarStoreBase,
+ IN UINTN NvVarStoreSize
+ )
+{
+ volatile UINT8 *Ptr;
+
+ RETURN_STATUS DecEncStatus;
+ UINT8 Offset;
+
+ DEBUG ((DEBUG_INFO, "%a\n", __func__));
+
+ if (!MemEncryptSevIsEnabled ()) {
+ return;
+ }
+
+ //
+ // In the case of launch a SEV-ES VM with just OVMF_CODE.fd, the
+ // validation process in PlatformValidateNvVarStore will trigger MMIO
+ // NPF, and the #VC handler will detect that mmio access is invalid
+ // because the mmio address range of FlashNvVarStore is mapped as
+ // encrypted. So, first we should update the mapping of address range
+ // of FlashNvVarStore as decrypted.
+ //
+ DEBUG (
+ (
+ DEBUG_INFO,
+ "%a: mapping FlashNvVarStore address range unencrypted [0x%p - 0x%p]\n",
+ __func__,
+ (UINT8 *)NvVarStoreBase,
+ (UINT8 *)NvVarStoreBase + NvVarStoreSize - 1
+ )
+ );
+
+ DecEncStatus = MemEncryptSevClearMmioPageEncMask (
+ 0,
+ NvVarStoreBase,
+ EFI_SIZE_TO_PAGES (NvVarStoreSize)
+ );
+ if (RETURN_ERROR (DecEncStatus)) {
+ DEBUG (
+ (
+ DEBUG_ERROR,
+ "%a: failed to map FlashNvStorage address range unencrypted\n",
+ __func__
+ )
+ );
+ ASSERT_RETURN_ERROR (DecEncStatus);
+ }
+
+ //
+ // Here, the first 16 bytes of FlashNvVarStore will be all zeros in
+ // the following cases:
+ // a. Launch VM with just OVMF_CODE.fd
+ // b. Launch VM with OVMF_CODE.fd and OVMF_VARS.fd
+ // In these cases, the access of FlashNvVarStore will be as expected.
+ //
+ // But if launch VM with just OVMF.fd, the first 16 bytes of
+ // FlashNvVarStore will be scrambled data because the data of
+ // FlashNvVarStore are encrypted by SEV API. In this case, we need
+ // mapping FlashNvVarStore address range as encrypted again, otherwise
+ // the validation of FlashNvVarStore will fail and trigger
+ // ASSERT (FALSE).
+ //
+ for (Offset = 0; Offset < 16; Offset++) {
+ Ptr = (UINT8 *)NvVarStoreBase + Offset;
+ if (*Ptr) {
+ break;
+ }
+ }
+
+ if (Offset == 16) {
+ return;
+ }
+
+ DEBUG (
+ (
+ DEBUG_INFO,
+ "%a: mapping FlashNvStorage address range encrypted\n",
+ __func__
+ )
+ );
+
+ DecEncStatus = MemEncryptSevSetPageEncMask (
+ 0,
+ NvVarStoreBase,
+ EFI_SIZE_TO_PAGES (NvVarStoreSize)
+ );
+ if (RETURN_ERROR (DecEncStatus)) {
+ DEBUG (
+ (
+ DEBUG_ERROR,
+ "%a: failed to map FlashNvStorage address range encrypted\n",
+ __func__
+ )
+ );
+ ASSERT_RETURN_ERROR (DecEncStatus);
+ }
+}
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 0114529778..bcb18dacac 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -224,6 +224,12 @@ ReserveEmuVariableNvStore (
PcdStatus = PcdSet64S (PcdEmuVariableNvStoreReserved, VariableStore);
if (FeaturePcdGet (PcdSecureBootSupported)) {
+ // update mapping of FlashNvVarStore address range
+ SevFlashNvVarStoreUpdateMapping (
+ PcdGet32 (PcdOvmfFlashNvStorageVariableBase),
+ 2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize)
+ );
+
// restore emulated VarStore from pristine ROM copy
PlatformInitEmuVariableNvStore ((VOID *)(UINTN)VariableStore);
}
diff --git a/OvmfPkg/PlatformPei/Platform.h b/OvmfPkg/PlatformPei/Platform.h
index 0a59547cfc..b8ad8df1dc 100644
--- a/OvmfPkg/PlatformPei/Platform.h
+++ b/OvmfPkg/PlatformPei/Platform.h
@@ -111,4 +111,10 @@ SevInitializeRam (
VOID
);
+VOID
+SevFlashNvVarStoreUpdateMapping (
+ IN UINTN NvVarStoreBase,
+ IN UINTN NvVarStoreSize
+ );
+
#endif // _PLATFORM_PEI_H_INCLUDED_
diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
index 0bb1a46291..a3dd3db72d 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -139,6 +139,7 @@
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaSize
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsBase
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsSize
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageVariableBase
[FeaturePcd]
gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
--
2.25.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119929): https://edk2.groups.io/g/devel/message/119929
Mute This Topic: https://groups.io/mt/107212944/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Update mapping of FlashNvVarStore before validate it
2024-07-14 12:22 [edk2-devel] [PATCH 0/3] Fix boot failure when use secure boot supported (-D SECURE_BOOT_ENABLE=TRUE) OVMF packages wojiaohanliyang
@ 2024-07-14 12:22 ` wojiaohanliyang
0 siblings, 0 replies; 11+ messages in thread
From: wojiaohanliyang @ 2024-07-14 12:22 UTC (permalink / raw)
To: devel; +Cc: hanliyang
From: hanliyang <wojiaohanliyang@163.com>
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4807
In the case of launch a SEV-ES VM with just OVMF_CODE.fd, the validation
process in PlatformValidateNvVarStore will trigger MMIO NPF, and the #VC
handler will detect that mmio access is invalid because the mmio address
range of FlashNvVarStore is mapped as encrypted.
In the case of launch a SEV VM with both OVMF_CODE.fd and OVMF_VARS.fd,
PlatformValidateNvVarStore will fail to validate FlashNvVarStore because
the mapping of FlashNvVarStore address range is encrypted in the guest
but the corresponding data in system physical memory was not encrypted
by guest key.
We should map FlashNvVarStore address range as unencrypted for the above
cases.
Fixes: 4f173db8b45b ("OvmfPkg/PlatformInitLib: Add functions for EmuVariableNvStore")
Signed-off-by: hanliyang <wojiaohanliyang@163.com>
---
OvmfPkg/PlatformPei/AmdSev.c | 105 ++++++++++++++++++++++++++++
OvmfPkg/PlatformPei/Platform.c | 6 ++
OvmfPkg/PlatformPei/Platform.h | 6 ++
OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
4 files changed, 118 insertions(+)
diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
index 8562787035..89f4c02b6a 100644
--- a/OvmfPkg/PlatformPei/AmdSev.c
+++ b/OvmfPkg/PlatformPei/AmdSev.c
@@ -575,3 +575,108 @@ SevInitializeRam (
);
}
}
+
+/**
+ Prepared for FlashNvVarStore access.
+
+ **/
+VOID
+SevFlashNvVarStoreUpdateMapping (
+ IN UINTN NvVarStoreBase,
+ IN UINTN NvVarStoreSize
+ )
+{
+ volatile UINT8 *Ptr;
+
+ RETURN_STATUS DecEncStatus;
+ UINT8 Offset;
+
+ DEBUG ((DEBUG_INFO, "%a\n", __func__));
+
+ if (!MemEncryptSevIsEnabled ()) {
+ return;
+ }
+
+ //
+ // In the case of launch a SEV-ES VM with just OVMF_CODE.fd, the
+ // validation process in PlatformValidateNvVarStore will trigger MMIO
+ // NPF, and the #VC handler will detect that mmio access is invalid
+ // because the mmio address range of FlashNvVarStore is mapped as
+ // encrypted. So, first we should update the mapping of address range
+ // of FlashNvVarStore as decrypted.
+ //
+ DEBUG (
+ (
+ DEBUG_INFO,
+ "%a: mapping FlashNvVarStore address range unencrypted [0x%p - 0x%p]\n",
+ __func__,
+ (UINT8 *)NvVarStoreBase,
+ (UINT8 *)NvVarStoreBase + NvVarStoreSize - 1
+ )
+ );
+
+ DecEncStatus = MemEncryptSevClearMmioPageEncMask (
+ 0,
+ NvVarStoreBase,
+ EFI_SIZE_TO_PAGES (NvVarStoreSize)
+ );
+ if (RETURN_ERROR (DecEncStatus)) {
+ DEBUG (
+ (
+ DEBUG_ERROR,
+ "%a: failed to map FlashNvStorage address range unencrypted\n",
+ __func__
+ )
+ );
+ ASSERT_RETURN_ERROR (DecEncStatus);
+ }
+
+ //
+ // Here, the first 16 bytes of FlashNvVarStore will be all zeros in
+ // the following cases:
+ // a. Launch VM with just OVMF_CODE.fd
+ // b. Launch VM with OVMF_CODE.fd and OVMF_VARS.fd
+ // In these cases, the access of FlashNvVarStore will be as expected.
+ //
+ // But if launch VM with just OVMF.fd, the first 16 bytes of
+ // FlashNvVarStore will be scrambled data because the data of
+ // FlashNvVarStore are encrypted by SEV API. In this case, we need
+ // mapping FlashNvVarStore address range as encrypted again, otherwise
+ // the validation of FlashNvVarStore will fail and trigger
+ // ASSERT (FALSE).
+ //
+ for (Offset = 0; Offset < 16; Offset++) {
+ Ptr = (UINT8 *)NvVarStoreBase + Offset;
+ if (*Ptr) {
+ break;
+ }
+ }
+
+ if (Offset == 16) {
+ return;
+ }
+
+ DEBUG (
+ (
+ DEBUG_INFO,
+ "%a: mapping FlashNvStorage address range encrypted\n",
+ __func__
+ )
+ );
+
+ DecEncStatus = MemEncryptSevSetPageEncMask (
+ 0,
+ NvVarStoreBase,
+ EFI_SIZE_TO_PAGES (NvVarStoreSize)
+ );
+ if (RETURN_ERROR (DecEncStatus)) {
+ DEBUG (
+ (
+ DEBUG_ERROR,
+ "%a: failed to map FlashNvStorage address range encrypted\n",
+ __func__
+ )
+ );
+ ASSERT_RETURN_ERROR (DecEncStatus);
+ }
+}
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 0114529778..bcb18dacac 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -224,6 +224,12 @@ ReserveEmuVariableNvStore (
PcdStatus = PcdSet64S (PcdEmuVariableNvStoreReserved, VariableStore);
if (FeaturePcdGet (PcdSecureBootSupported)) {
+ // update mapping of FlashNvVarStore address range
+ SevFlashNvVarStoreUpdateMapping (
+ PcdGet32 (PcdOvmfFlashNvStorageVariableBase),
+ 2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize)
+ );
+
// restore emulated VarStore from pristine ROM copy
PlatformInitEmuVariableNvStore ((VOID *)(UINTN)VariableStore);
}
diff --git a/OvmfPkg/PlatformPei/Platform.h b/OvmfPkg/PlatformPei/Platform.h
index 0a59547cfc..b8ad8df1dc 100644
--- a/OvmfPkg/PlatformPei/Platform.h
+++ b/OvmfPkg/PlatformPei/Platform.h
@@ -111,4 +111,10 @@ SevInitializeRam (
VOID
);
+VOID
+SevFlashNvVarStoreUpdateMapping (
+ IN UINTN NvVarStoreBase,
+ IN UINTN NvVarStoreSize
+ );
+
#endif // _PLATFORM_PEI_H_INCLUDED_
diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
index 0bb1a46291..a3dd3db72d 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -139,6 +139,7 @@
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaSize
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsBase
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsSize
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageVariableBase
[FeaturePcd]
gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
--
2.25.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119925): https://edk2.groups.io/g/devel/message/119925
Mute This Topic: https://groups.io/mt/107212922/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 11+ messages in thread