CVM register validation#3960
Open
jennagoddard wants to merge 6 commits into
Open
Conversation
Contributor
|
Whats the reason for needing this validation? Are we seeing callers pass bad values in and we can't just fix the callers? Should we not respect their decision to crash themselves? |
smalis-msft
reviewed
Jul 17, 2026
smalis-msft
reviewed
Jul 17, 2026
smalis-msft
reviewed
Jul 17, 2026
smalis-msft
reviewed
Jul 17, 2026
smalis-msft
reviewed
Jul 17, 2026
Contributor
Author
The issue is that if the guest writes invalid values to its registers, the next time the hypervisor tries to run the VMSA VMRUN fails with an invalid VMSA error. This is particularly bad as the host cannot read the VMSA to see what is invalid, and the VP cannot run. |
Contributor
|
We should probably trace a warn on any of these invalid writes then |
Contributor
Author
I thought about it but thought it might be too much given how many occur though we can use rate limiting. The error will cause the guest to see a #GP, which is the right place for the error to surface to a misbehaving guest |
sluck-msft
reviewed
Jul 17, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
openhcl/virt_mshv_vtl/src/processor/hardware_cvm/mod.rs:705
- CR0 normalization here clears some reserved low bits, but it still allows architecturally-invalid state to slip through:
- CR0 bits 63:32 are reserved and should be forced to 0 (or rejected).
- CR0.ET (bit 4) is architecturally fixed to 1 on x86_64 (TDX path already forces this via
value | X64_CR0_ET).
Leaving either case unhandled can still result in bad guest state being committed viaset_registers()and later failing at entry/run time.
HvX64RegisterName::Cr0 | HvX64RegisterName::IntermediateCr0 => {
let mut cr0 = reg.value.as_u64();
cr0 &= !X64_CR0_RESERVED_BITS_MASK_LOW32;
// Bit 6 must be zero; PG requires PE; NW requires CD.
if cr0 & X64_CR0_RSVDZ1_MASK != 0
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
openhcl/virt_mshv_vtl/src/processor/hardware_cvm/mod.rs:708
- CR0 validation/normalization only clears a subset of reserved bits in the low 32 bits, but it leaves any bits set in CR0[63:32] untouched. Those upper bits are architecturally reserved and can propagate into the backing state via
registers.cr0 = reg.value.as_u64(), potentially reintroducing the invalid state this validation is trying to prevent. Consider clearing the upper 32 bits (or rejecting nonzero upper bits) during normalization.
HvX64RegisterName::Cr0 | HvX64RegisterName::IntermediateCr0 => {
let mut cr0 = reg.value.as_u64();
cr0 &= !X64_CR0_RESERVED_BITS_MASK_LOW32;
// Bit 6 must be zero; PG requires PE; NW requires CD.
if cr0 & X64_CR0_RSVDZ1_MASK != 0
|| (cr0 & X64_CR0_PG != 0 && cr0 & X64_CR0_PE == 0)
|| (cr0 & X64_CR0_NW != 0 && cr0 & X64_CR0_CD == 0)
{
return Err(HvError::InvalidRegisterValue);
}
reg.value = HvRegisterValue::from(cr0);
}
sluck-msft
reviewed
Jul 21, 2026
smalis-msft
reviewed
Jul 21, 2026
smalis-msft
reviewed
Jul 21, 2026
smalis-msft
reviewed
Jul 21, 2026
Comment on lines
+143
to
+146
| const CVM_GUEST_VA_BITS_48: u32 = 48; | ||
| const CVM_GUEST_VA_BITS_57: u32 = 57; | ||
| const X64_CR0_RESERVED_BITS_MASK_LOW32: u64 = 0x1FFA_FF80; | ||
| const X64_CR0_RSVDZ1_MASK: u64 = 1 << 6; |
Contributor
There was a problem hiding this comment.
Should these be in x86defs?
smalis-msft
reviewed
Jul 21, 2026
smalis-msft
reviewed
Jul 21, 2026
smalis-msft
reviewed
Jul 21, 2026
smalis-msft
approved these changes
Jul 21, 2026
Comment on lines
+232
to
+240
| x86defs::X64_MSR_KERNEL_GS_BASE | ||
| | x86defs::X86X_MSR_LSTAR | ||
| | x86defs::X86X_MSR_CSTAR | ||
| | x86defs::X86X_MSR_SYSENTER_EIP | ||
| | x86defs::X86X_MSR_SYSENTER_ESP | ||
| | x86defs::X86X_MSR_INTERRUPT_SSP_TABLE_ADDR => { | ||
| if !is_canonical_address(value, CVM_GUEST_VA_BITS_57) { | ||
| return Err(MsrError::InvalidAccess); | ||
| } |
Comment on lines
+661
to
+665
| | HvX64RegisterName::InterruptSspTableAddr => { | ||
| if !is_canonical_address(reg.value.as_u64(), CVM_GUEST_VA_BITS_57) { | ||
| return Err(HvError::InvalidRegisterValue); | ||
| } | ||
| } |
sluck-msft
approved these changes
Jul 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add per-register / per-MSR validation on the SetVpRegisters and WRMSR paths to help prevent invalid VMSA state which cause failures at VMRUN