Prevent MSRs from leaking across calls to MultiuseSandbox::restore - #991
Conversation
aeca04d to
968f2f4
Compare
e859ff9 to
e793129
Compare
8f55249 to
3bb2294
Compare
032168d to
c0383dd
Compare
2e98073 to
089227c
Compare
There was a problem hiding this comment.
Pull request overview
This PR prevents x86_64 model-specific register (MSR) state from persisting across snapshot restores by adding explicit MSR capture/reset semantics to snapshots, enforcing an MSR allow-list contract, and (on KVM) denying guest MSR access by default via an MSR filter.
Changes:
- Capture MSR reset state into
Snapshot(and snapshot-on-disk format), and restore it duringMultiUseSandbox::from_snapshot/MultiUseSandbox::restore. - Add
SandboxConfiguration::allow_msrs(bounded allow list) and enforce allow-list compatibility on restore (destination must be a superset). - Implement backend-specific MSR handling: KVM deny-by-default filtering + violation reporting; MSHV/WHP MSR read/write support for reset.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/rust_guests/simpleguest/src/main.rs | Adds guest-side RDMSR/WRMSR and SWAPGS helpers for host MSR tests. |
| src/hyperlight_host/src/sandbox/snapshot/mod.rs | Extends Snapshot with optional captured MSRs and snapshot allow list. |
| src/hyperlight_host/src/sandbox/snapshot/file/mod.rs | Persists/loads MSR fields and validates msrs/allowed_msrs consistency. |
| src/hyperlight_host/src/sandbox/snapshot/file/config.rs | Adds MSR fields to OCI snapshot config and serde tests/schema pin updates. |
| src/hyperlight_host/src/sandbox/snapshot/file_tests.rs | Adds end-to-end disk snapshot tests for MSR capture/compat/allow-list rules. |
| src/hyperlight_host/src/sandbox/initialized_multi_use.rs | Captures MSRs on snapshot and restores them on from_snapshot/restore + extensive MSR tests. |
| src/hyperlight_host/src/sandbox/config.rs | Introduces MSR allow-list storage and capacity-checked allow_msrs. |
| src/hyperlight_host/src/mem/mgr.rs | Threads MSR state through snapshot construction. |
| src/hyperlight_host/src/hypervisor/virtual_machine/whp.rs | Adds WHP MSR register mapping + get/set MSRs for reset. |
| src/hyperlight_host/src/hypervisor/virtual_machine/mshv/x86_64.rs | Adds MSHV MSR mapping + get/set MSRs for reset. |
| src/hyperlight_host/src/hypervisor/virtual_machine/mod.rs | Extends VirtualMachine with MSR read/write APIs + validation helpers/errors. |
| src/hyperlight_host/src/hypervisor/virtual_machine/kvm/x86_64.rs | Implements KVM MSR filter configuration, filtered-exit handling, and KVM MSR get/set. |
| src/hyperlight_host/src/hypervisor/regs/x86_64/special_regs.rs | Adds test asserting x2APIC stays disabled by default. |
| src/hyperlight_host/src/hypervisor/regs/x86_64/msrs.rs | New MSR reset table + snapshot validation logic and related constants/tests. |
| src/hyperlight_host/src/hypervisor/regs/x86_64/mod.rs | Exposes the new MSR module via pub(crate) use. |
| src/hyperlight_host/src/hypervisor/hyperlight_vm/x86_64.rs | Captures baseline MSR reset state at VM creation and implements restore logic. |
| src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs | Wires KVM MSR violations into RunVmError and stores msr_reset in HyperlightVm. |
| src/hyperlight_host/src/error.rs | Adds HyperlightError::{MsrReadViolation, MsrWriteViolation} and marks them poisoning on KVM. |
| Justfile | Adds ignored host-dependent MSR audit test recipes to test-isolated. |
| docs/msr.md | Documents MSR reset set, backend differences, validation rules, and limitations. |
| CHANGELOG.md | Adds breaking-change entry for MSR allow-list + restore behavior. |
| .github/workflows/ValidatePullRequest.yml | Disables matrix fail-fast for PR validation workflow. |
c079263 to
50b829f
Compare
syntactically
left a comment
There was a problem hiding this comment.
This looks functionally quite good! I left a number of review comments inline, which are mostly about improving clarity / things whose function was not obvious to me.
Beyond what I've explicitly called out in the review comments, I think the high level structure of both the code and the documentation could be a little bit clearer. For the code, I felt like it was not always clear what was HV-specific and what wasn't, and there was just generally a lot of jumping around to follow e.g. the initialisation sequences and compare kvm vs mshv for things like allowlist filtering. For the documentation, there are currently a lot of sections that are all level 2 headings, some of which are related to each other and some of which are not, and it feels like it jumps between different things a few times, making it harder to follow; probably there is some way to use heading structure to organise it a bit more nicely) could be a bit clearer.
87d696b to
3e609eb
Compare
ludfjig
left a comment
There was a problem hiding this comment.
I think I addressed your comments. I moved the hyperv specific code to be called from respective backend (whp/mshv) but since it's duplicate code the code is still located outside the backends. Would you prefer the duplicate code appraoch inside each backend isntead?
KVM denies guest MSR access by default. SandboxConfiguration::allow_msrs permits selected MSRs. MSHV and WHP have no per-MSR filter. Hyperlight captures exposed retained MSR state at VM creation and resets it on restore. Captured MSR state persists in OCI snapshots. KVM denials report the MSR index. Unsupported accesses on MSHV and WHP raise a guest general protection fault. Both failures poison the sandbox. Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
Active SSP is guest-writable state exposed through the Hyper-V VP register API, not an architectural MSR. Add it to the Hyper-V reset candidates and the WHP register map so restore clears it, while keeping it out of the allow-list surface. Guest tests read SSP and TSC to confirm neither leaks across restore. Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
KVM cannot reset active SSP across restore. Removing CET (CPUID leaf 7 SHSTK/IBT) from the guest stops it enabling shadow stacks, so active SSP never changes and the gap is unreachable. IA32_S_CET is then unreadable host-side and rejected from allow_msrs at creation. MSHV and WHP expose CET and reset active SSP instead. Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
|
I didn't have a chance to actually read this yet. A couple of immediate reactions from the comment replies you wrote to me:
Duplicating the code does indeed not seem great. I might go for a mshv_shared module at the same level as kvm/mshv/hvf/whp that has any shared logic for the mshv/whp stuff? This is getting into the weeds of taste now, so feel free to ignore. |
Save only the declared guest_msrs plus a fixed core into a snapshot and scrub the rest to the destination baseline on restore. Renames allow_msrs to guest_msrs and stores snapshot MSRs as a flat Vec<MsrEntry>. Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
The goal is to prevent guest state to persist across snapshot-restores.
restorewill reset set msrs using a hardcoded list of MSRs that are known to be writeable. The baseline values of these MSRs are captured on vm-creation. A limitation is that any MSR that is guest-writable and not in this list, has the potential to leak.Future work:
Will mark ready for review once KVM releases new version, which should include newly addedKVM_X86_SET_MSR_FILTERvm ioctl that this PR depends on, see rust-vmm/kvm#359