[Bug]: AMD SEV-SNP: SVSM-populated secrets fields not respected

[deeglaze]

Is there an existing issue for this?

• I have searched existing issues

Bug Type

• Firmware
• Tool
• Unit Test

Code first?

• Yes

What packages are impacted?

OvmfPkg

Which targets are impacted by this bug?

DEBUG, NO-TARGET, NOOPT, RELEASE

Current Behavior

The PcdsFixedAtBuild-classified PcdOvmfSecSvsmCaaBase and PcdOvmfSecSvsmCaaSize as located at

edk2/OvmfPkg/OvmfPkgX64.fdf

Line 97 in 87f1432

0x00F000|0x001000

ought to instead be populated as

• the 64-bit value at offset 0x150 of the AMD SEV secrets page
• 0x1000

respectively [1]. Relatedly, the base/size values at offsets 0x140, 0x148 ought to define a reserved memory HOB. We can restrict these allocations to be within few pre-determined areas to ensure we know exactly which HOBs to break up.

[1] https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/58019.pdf table 1

Expected Behavior

The CAA_BASE can be set by the SVSM itself in the Secrets page, subject to some restrictions

Steps To Reproduce

Populate the CAA_BASE field in SVSM be somewhere else and try to communicate with SVSM at that address. If you're lucky it crashes. If you're unlucky, you corrupt memory.

Build Environment

- OS(s): Any
- Tool Chain(s): Any

Version Information

Current

Urgency

Medium

Are you going to fix this?

Someone else needs to fix it

Do you need maintainer feedback?

Maintainer feedback requested

Anything else?

Request feedback from Thomas Lendacky.

[lgao4]

@tlendacky , please help check this issue.

[tlendacky]

ought to instead be populated as

• the 64-bit value at offset 0x150 of the AMD SEV secrets page
• 0x1000

The purpose of PcdOvmfSecSvsmCaaBase and PcdOvmfSecSvsmCaaSize is to provide an area that the SVSM can use to populate the SVSM_CAA field of the Secrets Page, which is supplied by the SVSM to OVMF. The SVSM would locate this area by parsing the SNP MetaData in the firmware image. However, the SVSM could supply another address if it wants. This is correct as it is.

Relatedly, the base/size values at offsets 0x140, 0x148 ought to define a reserved memory HOB.

The SVSM_BASE and SVSM_SIZE are the address and size that the SVSM lives at. Are you saying that a reserved memory HOB should be created for this range?

Populate the CAA_BASE field in SVSM be somewhere else and try to communicate with SVSM at that address. If you're lucky it crashes. If you're unlucky, you corrupt memory.

Are you trying to protect OVMF from a malicious SVSM? Are you saying that OVMF should somehow validate the address before using it?

[deeglaze]

Less of a malicious svsm and more of an unfortunate integration problem when assembling an IGVM.

The SVSM_BASE and SVSM_SIZE are the address and size that the SVSM lives at. Are you saying that a reserved memory HOB should be created for this range?

Yes I believe so.

[tlendacky]

Less of a malicious svsm and more of an unfortunate integration problem when assembling an IGVM.

The SVSM_BASE and SVSM_SIZE are the address and size that the SVSM lives at. Are you saying that a reserved memory HOB should be created for this range?

Yes I believe so.

Ok, so just the creation of the reserved memory HOB (assuming it falls within the ranges of the known OVMF memory). I'll work on that.

[tlendacky]

Creating the reserved memory HOB is straight forward. However, when providing an address space that encompasses the SVSM (SVSM at 512GB and guest with over 512GB of memory), the SVSM ends up in unaccepted memory. That reservation does not get propagated to the OS and eventually the OS crashes trying to accept SVSM memory.

@deeglaze, since you worked on the Unaccepted Memory support, any idea how to resolve this? I would expect that we should see a single large unaccepted memory range, broken into three ranges now - an unaccepted memory range, a reserved range, and then the remaining unaccepted memory range. Once this is fixed, I can add the reserved memory HOB for the SVSM.

[deeglaze]

Yes, the memdetect code will need to be updated to split the HOBs to account for the svsm placement.

[tlendacky]

Yes, the memdetect code will need to be updated to split the HOBs to account for the svsm placement.

Not just for SVSM placement, but for any reserved area that falls in the unaccepted range, right?

[deeglaze]

It’s only “the unaccepted range” if the HOB includes it, and I’m saying to split the HOB to exclude the svsm and then declare the svsm as reserved.

[tlendacky]

The problem will occur with any memory that is marked reserved that falls above 4GB. Marking a range above 4GB as unaccepted doesn't take any reserved areas into account and will report the full area as unaccepted to the OS. Splitting the system memory HOB around the SVSM is fine for now, but if there is ever something else that ends up reserved above 4GB, it is broken again.

I'm not familiar with the memory management system in EDK2, but it would appear that whatever code that deals with existing memory allocation HOBs when establishing the memory map, needs to also support unaccepted memory.

[tlendacky]

It could also be argued that the E820 map presented by the VMM (or the SVSM) to OVMF should exclude the SVSM range or have it marked reserved, without requiring any OVMF changes.