Snp fixes #6285

tlendacky · 2024-10-03T21:26:09Z

Description

This pull request fixes problems related to SEV and the EFI variable store.

Patch 1:
Current SEV and SEV-ES guests run with flash devices, Qemu -drive if=pflash,... option, as the source for EFI code and variables. However, SEV-SNP does not use flash drives, instead running with the Qemu -bios option. As a result, the Qemu flash driver support should not be loaded under SEV-SNP. Fix this by not attempting to detect flash when running under SEV-SNP.

Patch 2:
When OVMF is built with SECURE_BOOT_ENABLE=TRUE, OVMF attempts to reserve and initialize memory for the EFI NV variable store during the PEI phase. However, this is done before SEV-SNP has had a chance to accept memory. Fix this by moving the call to ReserveEmuVariableNvStore() to after SEV-SNP initialization. Additionally, this is moved to after TDX initialization, too, but I am unable to test if that is an issue.

Patch 3:
When OVMF is build with SECURE_BOOT_ENABLE=TRUE, OVMF attempts to reserved and initialize memory for the EFI NV variable store during the PEI phase. For an SEV or SEV-ES, a flash device holds the EFI variable store. If this is the case, then accesses to the device need to be done unencrypted. PlatformValidateNvVarStore() validates the EFI variable store using an encrypted mapping. If this validation fails (because the flash device holds the variable store) then an ASSERT() is issued. Fix this issue by attempting a re-validation of the EFI variable store after changing the mapping to unencrypted. This fixes bugzilla 4379.

Patch 4:
Issue a message when the emulated variable store is initialized using the template structure.

Breaking change?
- Breaking change - Does this PR cause a break in build or boot behavior?
- Examples: Does it add a new library class or move a module to a different repo.
Impacts security?
- Security - Does this PR have a direct security impact?
- Examples: Crypto algorithm change or buffer overflow fix.
Includes tests?
- Tests - Does this PR include any explicit test code?
- Examples: Unit tests or integration tests.

How This Was Tested

Booted legacy, SEV, SEV-ES and SEV-SNP guests in multiple flash (or non-flash for SEV-SNP) configurations.

Integration Instructions

N/A

tlendacky · 2024-10-04T16:56:22Z

@mxu9, I'm looking at moving the call to ReserveEmuVariableNvStore() to after the SEV (where SNP memory acceptance/validation is done) and TDX initialization. Do you have any issues with this? Is there a scenario where this has to be done before TDX initialization?

r1chard-lyu

Hi all,

I have tested this patch set, and it works with SEV-SNP on my SEV machine without issues. Additionally, it resolves Bugzilla 4379.

Please feel free to add the tag 'Reviewed-by: Richard Lyu <[email protected]>'. Thank you!

mxu9 · 2024-10-16T03:20:32Z

We're testing the PR in Intel TDX platform and will update the test result soon.
[Update] This PR works in Intel TDX Platform.

mxu9

LGTM.

tlendacky · 2024-10-21T13:06:56Z

@kraxel , @jyao1 , any comments?

tlendacky · 2024-10-29T17:03:35Z

@kraxel , @jyao1 , just a reminder...

kraxel

Two small remarks, overall looks good to me.

OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c

OvmfPkg/Library/PlatformInitLib/Platform.c

cowbon · 2024-11-01T19:05:25Z

Hi @tlendacky ,

May I ask how exactly you build this patch and test it? I passed -bios OVMF.fd to QEMU in a normal guest and try to enroll necessary certs for SB validation using mokutil --import /path/to/certs, but shim did not show any pending MOK enrollment request after reboot. Btw, is OVMF.fd a combined version of OVMF_CODE/VARS?

tlendacky · 2024-11-04T17:47:08Z

Hi @tlendacky ,

May I ask how exactly you build this patch and test it? I passed -bios OVMF.fd to QEMU in a normal guest and try to enroll necessary certs for SB validation using mokutil --import /path/to/certs, but shim did not show any pending MOK enrollment request after reboot. Btw, is OVMF.fd a combined version of OVMF_CODE/VARS?

Not sure what you mean by build this patch and test it? Which patch of the four are you referring to?

And, yes, OVMF.fd is a combined OVMF_VARS.fd/OVMF_CODE.fd file.

cowbon · 2024-11-04T22:40:09Z

Hi @tlendacky ,
May I ask how exactly you build this patch and test it? I passed -bios OVMF.fd to QEMU in a normal guest and try to enroll necessary certs for SB validation using mokutil --import /path/to/certs, but shim did not show any pending MOK enrollment request after reboot. Btw, is OVMF.fd a combined version of OVMF_CODE/VARS?

Not sure what you mean by build this patch and test it? Which patch of the four are you referring to?

And, yes, OVMF.fd is a combined OVMF_VARS.fd/OVMF_CODE.fd file.

I was asking how you built and tested this PR (branch snp-fixes), sorry for the poor wording. I was not able to enroll MOK after reboot with -bios OVMF.fd is passed to QEMU, because shim did not show MOK enrollment screen but jumped directly to GRUB menu. I want to ask if I miss any step. The build target was set to OvmfPkg/OvmfPkgX64.dsc and I passed -D SMM_REQUIRED -D SECURE_BOOT_ENABLE when I built your snp-fixes branch

kraxel · 2024-11-05T11:28:57Z

I was asking how you built and tested this PR (branch snp-fixes), sorry for the poor wording. I was not able to enroll MOK after reboot with -bios OVMF.fd is passed to QEMU, because shim did not show MOK enrollment screen but jumped directly to GRUB menu. I want to ask if I miss any step. The build target was set to OvmfPkg/OvmfPkgX64.dsc and I passed -D SMM_REQUIRED -D SECURE_BOOT_ENABLE when I built your snp-fixes branch

With -bios OVMF.fd you simply have no persistent efi variable store, it will be reset on every boot. You'll need the splitted version for that, and use pflash (r/o for CODE and r/w for VARS).

cowbon · 2024-11-05T15:02:42Z

I was asking how you built and tested this PR (branch snp-fixes), sorry for the poor wording. I was not able to enroll MOK after reboot with -bios OVMF.fd is passed to QEMU, because shim did not show MOK enrollment screen but jumped directly to GRUB menu. I want to ask if I miss any step. The build target was set to OvmfPkg/OvmfPkgX64.dsc and I passed -D SMM_REQUIRED -D SECURE_BOOT_ENABLE when I built your snp-fixes branch

With -bios OVMF.fd you simply have no persistent efi variable store, it will be reset on every boot. You'll need the splitted version for that, and use pflash (r/o for CODE and r/w for VARS).

Thanks @kraxel. My goal is to enable secure boot under SEV-SNP. I understand OVMF_VARS.fd provides a persistent efi variable store. But in the case of SEV-SNP, a volatile OVMF.fd is used instead. So how do you enable SB under SEV-SNP? To be more specific, what are the steps to enroll MOK when it's required to use the combined OVMF.fd under SEV-SNP?

kraxel · 2024-11-06T08:58:09Z

Thanks @kraxel. My goal is to enable secure boot under SEV-SNP. I understand OVMF_VARS.fd provides a persistent efi variable store. But in the case of SEV-SNP, a volatile OVMF.fd is used instead. So how do you enable SB under SEV-SNP? To be more specific, what are the steps to enroll MOK when it's required to use the combined OVMF.fd under SEV-SNP?

You can use https://gitlab.com/kraxel/virt-firmware/ to update the variable store (works for both OVMF_VARS.fd and OVMF.fd).

tlendacky · 2024-11-12T14:08:11Z

@jyao1 , ping...

SEV-SNP does not support the use of the Qemu flash device as SEV-SNP guests are started using the Qemu -bios option instead of the Qemu -drive if=pflash option. Perform runtime detection of SEV-SNP and exit early from the Qemu flash device initialization, indicating the Qemu flash device is not present. SEV-SNP guests will use the emulated variable support. Signed-off-by: Tom Lendacky <[email protected]>

…ance When OVMF is built with the SECURE_BOOT_ENABLE set to true, reserving and initializing the emulated variable store happens before memory has been accepted under SEV-SNP. This results in a #VC exception for accessing memory that hasn't been validated (error code 0x404). The #VC handler treats this error code as a fatal error, causing the OVMF boot to fail. Move the call to ReserveEmuVariableNvStore() to after memory has been accepted by AmdSevInitialize(). Signed-off-by: Tom Lendacky <[email protected]>

When OVMF is built with SECURE_BOOT_ENABLE, the variable store will be populated and validated in PlatformValidateNvVarStore(). When an SEV or an SEV-ES guest is running, this may be encrypted or unencrypted depending on how the guest was started. If the guest was started with the combined code and variable contents (OVMF.fd), then the variable store will be encrypted. If the guest was started with the separate code and variables contents (OVMF_CODE.fd and OVMF_VARS.fd), then the variable store will be unencrypted. When PlatformValidateNvVarStore() is first invoked, the variable store area is initially mapped encrypted, which may or may not pass the variable validation step depending how the guest was launched. To accomodate this, retry the validation step on failure after remapping the variable store area as unencrypted. Signed-off-by: Tom Lendacky <[email protected]>

Add a debug message that indicates when the NV variables are being initialized through the template structure. Signed-off-by: Tom Lendacky <[email protected]>

tlendacky force-pushed the snp-fixes branch 2 times, most recently from 8f0ad15 to 112df44 Compare October 9, 2024 17:33

tlendacky requested review from mdroth, kraxel, jyao1 and mxu9 October 9, 2024 17:37

tlendacky marked this pull request as ready for review October 9, 2024 17:37

tlendacky force-pushed the snp-fixes branch 2 times, most recently from 7d0461e to d8dff51 Compare October 14, 2024 16:23

r1chard-lyu approved these changes Oct 16, 2024

View reviewed changes

mxu9 approved these changes Oct 16, 2024

View reviewed changes

tlendacky force-pushed the snp-fixes branch 2 times, most recently from 69d15f7 to 7d7775c Compare October 23, 2024 19:30

kraxel reviewed Nov 1, 2024

View reviewed changes

OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c Outdated Show resolved Hide resolved

OvmfPkg/Library/PlatformInitLib/Platform.c Show resolved Hide resolved

tlendacky force-pushed the snp-fixes branch from 7d7775c to b2f6e50 Compare November 5, 2024 15:59

tianocore-assign-reviewers bot requested a review from ardbiesheuvel November 5, 2024 15:59

kraxel approved these changes Nov 6, 2024

View reviewed changes

tlendacky added 2 commits November 12, 2024 11:56

OvmfPkg/EmuVariableFvbRuntimeDxe: Issue NV vars initializitation message

5d0bc81

Add a debug message that indicates when the NV variables are being initialized through the template structure. Signed-off-by: Tom Lendacky <[email protected]>

tlendacky force-pushed the snp-fixes branch from b2f6e50 to 5d0bc81 Compare November 12, 2024 17:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Snp fixes #6285

Snp fixes #6285

tlendacky commented Oct 3, 2024

tlendacky commented Oct 4, 2024

r1chard-lyu left a comment •

edited

Loading

mxu9 commented Oct 16, 2024 •

edited

Loading

mxu9 left a comment

tlendacky commented Oct 21, 2024

tlendacky commented Oct 29, 2024

kraxel left a comment

cowbon commented Nov 1, 2024

tlendacky commented Nov 4, 2024

cowbon commented Nov 4, 2024

kraxel commented Nov 5, 2024

cowbon commented Nov 5, 2024

kraxel commented Nov 6, 2024

tlendacky commented Nov 12, 2024

Snp fixes #6285

Are you sure you want to change the base?

Snp fixes #6285

Conversation

tlendacky commented Oct 3, 2024

Description

How This Was Tested

Integration Instructions

tlendacky commented Oct 4, 2024

r1chard-lyu left a comment • edited Loading

Choose a reason for hiding this comment

mxu9 commented Oct 16, 2024 • edited Loading

mxu9 left a comment

Choose a reason for hiding this comment

tlendacky commented Oct 21, 2024

tlendacky commented Oct 29, 2024

kraxel left a comment

Choose a reason for hiding this comment

cowbon commented Nov 1, 2024

tlendacky commented Nov 4, 2024

cowbon commented Nov 4, 2024

kraxel commented Nov 5, 2024

cowbon commented Nov 5, 2024

kraxel commented Nov 6, 2024

tlendacky commented Nov 12, 2024

r1chard-lyu left a comment •

edited

Loading

mxu9 commented Oct 16, 2024 •

edited

Loading