Skip to content

OvmfPkg: Use the OvmfPkg version of CcProbeLib#10597

Merged
mergify[bot] merged 2 commits intotianocore:masterfrom
tlendacky:sev-amdsev-pkg-fix
Jan 13, 2025
Merged

OvmfPkg: Use the OvmfPkg version of CcProbeLib#10597
mergify[bot] merged 2 commits intotianocore:masterfrom
tlendacky:sev-amdsev-pkg-fix

Conversation

@tlendacky
Copy link
Copy Markdown
Contributor

Description

Currently, multiple dsc files within the OvmfPkg directory use the NULL version of the CcProbeLib library. However, these packages have support for confidential guests (usage of CcExitLib, MemEncrypt{Sev,Tdx}Lib, etc.) and should be using the OvmfPkg version of the CcProbeLib.

The use of the NULL library causes the PCI option ROM to be enabled, which can't be trusted as it originates from the hypervisor. The use of the NULL library also causes a KVM hypervisor error when attempting to map/back the option ROM region when running an SEV-SNP guest.

Update the various dsc files to reference the OvmfPkg version of the CcProbeLib library and prevent usage of PCI option ROMs.

  • 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

This was tested by launching SEV, SEV-ES and SEV-SNP guest using the AmdSev package. I have not tested the changes to the CloudHv or the Microvm packages.

Integration Instructions

N/A

Currently, multiple dsc files within the OvmfPkg directory use the NULL
version of the CcProbeLib library. However, these packages have support
for confidential guests (usage of CcExitLib, MemEncrypt{Sev,Tdx}Lib, etc.)
and should be using the OvmfPkg version of the CcProbeLib.

The use of the NULL library causes the PCI option ROM to be enabled, which
can't be trusted as it originates from the hypervisor. The use of the NULL
library also causes a KVM hypervisor error when attempting to map/back the
option ROM region when running an SEV-SNP guest.

Update the various dsc files to reference the OvmfPkg version of the
CcProbeLib library and prevent usage of PCI option ROMs.

Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
@github-actions github-actions Bot added the impact:security This change has a direct security impact such as changing a crypto algorithm. label Jan 8, 2025
@tlendacky tlendacky marked this pull request as ready for review January 8, 2025 21:15
@tlendacky tlendacky requested a review from kraxel January 8, 2025 21:15
@tlendacky
Copy link
Copy Markdown
Contributor Author

@fitzthum , I can't add you as a reviewer (should you get added for the AmdSev package?), but wanted you to take a look at the change.

@kraxel, do you know who could test the CloudHv and Microvm changes?

Copy link
Copy Markdown
Contributor

@mxu9 mxu9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me.

@kraxel
Copy link
Copy Markdown
Member

kraxel commented Jan 9, 2025

I can test microvm tomorrow. Not sure about cloudhw, I'd suggest to check the commit log.

@tlendacky
Copy link
Copy Markdown
Contributor Author

I can test microvm tomorrow. Not sure about cloudhw, I'd suggest to check the commit log.

Ok, I was able to boot an image using Cloud Hypervisor.

@tlendacky tlendacky requested review from jongwu and weltling January 9, 2025 17:13
Copy link
Copy Markdown
Contributor

@fitzthum fitzthum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Surprising that we didn't notice this regression earlier.

@kraxel
Copy link
Copy Markdown
Member

kraxel commented Jan 10, 2025

microvm is broken in master, looking ...
I don't expect this causing additional problems, so merging this without microvm testing is fine with me.

@ardbiesheuvel ardbiesheuvel added the push Auto push patch series in PR if all checks pass label Jan 13, 2025
@mergify mergify Bot merged commit 8b87eb9 into tianocore:master Jan 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

impact:security This change has a direct security impact such as changing a crypto algorithm. push Auto push patch series in PR if all checks pass

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants