Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stage2: Use IGVM memory map to resize SVSM kernel #590

Merged
merged 1 commit into from
Jan 21, 2025

Conversation

AdamCDunlap
Copy link
Contributor

If the IGVM memory map contains a PARAVISOR_RESERVED entry, then place the SVSM kernel at that region instead of the region specified in the IgvmParamBlock. If no such entry exists in the memory map, then continue using the IgvmParamBlock.

This allows the hypervisor to dynamically resize the SVSM based on the machine shape without changing the IgvmParamBlock and thus the launch measurement. This is necessary because the SVSM uses more memory when there are more vCPUs (and, in the future, guest memory).

Note this relies on an unpublished IGVM spec change to add the PARAVISOR_RESERVED MemoryMapEntryType.

kernel/src/igvm_params.rs Outdated Show resolved Hide resolved
kernel/src/igvm_params.rs Outdated Show resolved Hide resolved
@AdamCDunlap
Copy link
Contributor Author

Thanks for taking a look. It seems like the fundamental question here is if a maliciously-chosen kernel region could cause anything worse than DoS. If it can, then we should add checks for the specific conditions necessary (e.g. check alignment, check it doesn't overlap anything important, check a size minimum, or in the worst case check against a few known-good values).

I was under the impression that an incorrect region wouldn't cause anything worse than a DoS, however, which is why I didn't add any checks (besides 4k alignment which is implicit in the multiplication). I'd certainly be open to adding generic checks for things that are weird even if not currently exploitable.

@AdamCDunlap AdamCDunlap force-pushed the paravisor_reserved_mem branch 3 times, most recently from 6244f89 to 5842fd9 Compare January 11, 2025 01:03
@joergroedel joergroedel added the in-review PR is under active review and not yet approved label Jan 13, 2025
@msft-jlange
Copy link
Collaborator

@AdamCDunlap Version 0.3.4 of the igvm crate will be released within the next couple of hours. It defines a new memory type, HIDDEN, which indicates that memory is hidden from the guest outside of the enumeration in the IGVM memory map. You should change your PR to use that name and to update the IGVM version in Cargo.toml. Once that's done, I can approve this PR.

If the IGVM memory map has a HIDDEN entry that matches where the SVSM
kernel should go according to the IgvmParamBlock, then use the size of
that block for the size of the kernel (checked against a minimum and
maximum size in the ParamBlock)

This allows the hypervisor to dynamically resize the SVSM based on the
machine shape without changing the IgvmParamBlock and thus the launch
measurement. This is necessary because the SVSM uses more memory when
there are more vCPUs (and, in the future, guest memory). The location
and minium and maximum size can still be measured in the IgvmParamBlock
to ensure that the kernel region will not overlap anything important or
cause undersize problems.

Signed-off-by: Adam Dunlap <[email protected]>
@AdamCDunlap AdamCDunlap force-pushed the paravisor_reserved_mem branch from 5842fd9 to 7ce600f Compare January 16, 2025 19:07
@AdamCDunlap AdamCDunlap changed the title RFC: stage2: Prefer using IGVM memory map to place SVSM kernel stage2: Use IGVM memory map to resize SVSM kernel Jan 16, 2025
@AdamCDunlap AdamCDunlap marked this pull request as ready for review January 16, 2025 19:08
Copy link
Collaborator

@msft-jlange msft-jlange left a comment

Choose a reason for hiding this comment

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

Thanks for getting this done!

@msft-jlange msft-jlange added ready-to-merge PR is ready for merging into main branch and removed in-review PR is under active review and not yet approved labels Jan 16, 2025
@AdamCDunlap
Copy link
Contributor Author

Thanks for your help!

@joergroedel joergroedel merged commit 286ac39 into coconut-svsm:main Jan 21, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge PR is ready for merging into main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants