|
| 1 | +# Phase 1 handoff: MSR reset on KVM |
| 2 | + |
| 3 | +This brief is for an agent working on a Linux/KVM machine. It implements Phase 1 |
| 4 | +of `plan.md` (KVM only). Read `plan.md` first, then this. This brief pins the |
| 5 | +starting point, the exact files, the task order, and the done criteria. Every |
| 6 | +factual claim here was verified against the source or the pinned crates. |
| 7 | + |
| 8 | +## Scope |
| 9 | + |
| 10 | +Phase 1 only. KVM backend. Split reset from access control: |
| 11 | +* Add MSR reset (capture in snapshot, write back in `reset_vcpu`). |
| 12 | +* Replace the `allow_msr` bool with a validated per-MSR allow list. |
| 13 | +* Keep the KVM default-deny filter, add allow ranges so allowed MSRs pass through. |
| 14 | +* Tests. |
| 15 | + |
| 16 | +Out of scope for Phase 1: mshv, WHP, the `0a` CPUID masking work, LBR/MTRR |
| 17 | +decisions beyond CPUID gating, persistence schema changes. Do not start those. |
| 18 | + |
| 19 | +## Why this change (one paragraph) |
| 20 | + |
| 21 | +`reset2` denies MSR access but never resets MSR state, and its `allow_msr` bool is |
| 22 | +an all-or-nothing `unsafe` switch that disables the filter entirely and leaks |
| 23 | +state across `restore()`. Reset (save in snapshot, restore on `restore()`) is the |
| 24 | +isolation guarantee. Access control (deny-by-default plus a per-MSR allow list) |
| 25 | +bounds what the guest can write so the reset set is complete by construction. See |
| 26 | +`plan.md` §2 for the full rationale. |
| 27 | + |
| 28 | +## Starting point |
| 29 | + |
| 30 | +* Branch: detached at `origin/reset2` @ `68ad5265` "Disallow guest from |
| 31 | + read/write MSRs by default". Create a working branch from it. |
| 32 | +* Environment check before anything else. Confirm `/dev/kvm` exists and is |
| 33 | + read/write for the current user. If a sandbox fails with "No Hypervisor was |
| 34 | + found", print `ls -l /dev/kvm` and group membership and stop. |
| 35 | + |
| 36 | +## What `reset2` already ships (verified anchors) |
| 37 | + |
| 38 | +All KVM-only, under `src/hyperlight_host/src/`. |
| 39 | + |
| 40 | +* KVM deny-all filter: `enable_msr_filter()` in |
| 41 | + `hypervisor/virtual_machine/kvm/x86_64.rs` (~line 357). Sets |
| 42 | + `KVM_MSR_FILTER_DEFAULT_DENY` with one dummy all-zero-bit range plus |
| 43 | + `enable_cap(X86UserSpaceMsr, Filter)`. |
| 44 | +* Filter wired at VM creation: `hypervisor/hyperlight_vm/x86_64.rs` (~line 99), |
| 45 | + calls `enable_msr_filter()` unless `config.get_allow_msr()`. |
| 46 | +* Filter exits: `kvm/x86_64.rs` handles `X86Rdmsr`/`X86Wrmsr` (~lines 236-252 and |
| 47 | + 313-333) with the `error=1` plus `immediate_exit` handshake, injecting `#GP`, |
| 48 | + returning `VmExit::MsrRead`/`MsrWrite`. |
| 49 | +* `VmExit` variants: `hypervisor/virtual_machine/mod.rs` (~lines 144, 147). |
| 50 | +* Violation mapping: `hypervisor/hyperlight_vm/mod.rs` maps `MsrRead`/`MsrWrite` |
| 51 | + (~lines 752-757) to `RunVmError::MsrReadViolation`/`MsrWriteViolation` |
| 52 | + (~lines 228, 231), then to the matching `HyperlightError` (~lines 167-173). |
| 53 | +* Config: `sandbox/config.rs` has `allow_msr: bool` (~lines 79, 125) and |
| 54 | + `unsafe fn set_allow_msr(bool)` / `get_allow_msr()` (~lines 177, 184). |
| 55 | +* Config use: `sandbox/initialized_multi_use.rs` calls `set_allow_msr(true)` |
| 56 | + (~line 2777). Update this call site when the API changes. |
| 57 | +* Reset today: `reset_vcpu` in `hypervisor/hyperlight_vm/x86_64.rs` (~line 348) |
| 58 | + resets only the `sregs` MSRs. `apply_sregs` (~lines 262-269) calls `set_sregs`. |
| 59 | + `get_snapshot_sregs` (~line 274). |
| 60 | +* Snapshot: `sandbox/snapshot/mod.rs` `Snapshot` struct (~line 70). `snapshot()` |
| 61 | + in `sandbox/initialized_multi_use.rs` (~line 356). |
| 62 | +* Registers: `hypervisor/regs/x86_64/special_regs.rs`. `CommonSegmentRegister` |
| 63 | + carries `base` (~line 379), so `fs.base`/`gs.base` restore through `set_sregs`. |
| 64 | + |
| 65 | +## Verified facts that shape the code |
| 66 | + |
| 67 | +* `FS_BASE`/`GS_BASE` reset through `sregs` on every restore. Do NOT put them in |
| 68 | + `CommonMsrs`. Only `KERNEL_GS_BASE` needs MSR reset. |
| 69 | +* Guest CPUID is host pass-through on KVM. `kvm/x86_64.rs` (~lines 155-165) starts |
| 70 | + from `get_supported_cpuid()` and modifies only leaf `0x8000_0008`. So the guest |
| 71 | + sees host MTRR/CET/FRED/LBR/PMU. `KVM_GET_MSR_INDEX_LIST` tracks this, so Phase 1 |
| 72 | + needs no CPUID masking. |
| 73 | +* In-kernel LAPIC exists under the `hw-interrupts` feature (`create_irq_chip`, |
| 74 | + `kvm/x86_64.rs` ~line 142). It is xAPIC, not x2APIC. LAPIC reset (§7) is part of |
| 75 | + Phase 3, but keep the ordering hook in mind. |
| 76 | +* No TSC setup anywhere. Bare TSC is monotonic across VP reuse. Reset `TSC_ADJUST` |
| 77 | + to baseline, never write raw `TSC`. Mark `TSC`/`TSC_AUX` `host_specific`. |
| 78 | +* `kvm-ioctls` `get_msrs`/`set_msrs` are batched. `KVM_SET_MSRS` returns a count |
| 79 | + and stops at the first bad index. A short count means fail closed and poison. |
| 80 | + |
| 81 | +## API decision to make first |
| 82 | + |
| 83 | +Replace `unsafe fn set_allow_msr(bool)` on `SandboxConfiguration` with a safe, |
| 84 | +fallible allow list. Suggested shape, adjust to house style: |
| 85 | + |
| 86 | +```rust |
| 87 | +// Add each MSR the guest may read and write. Validated at init (see plan §10). |
| 88 | +pub fn allow_msr(&mut self, index: u32) -> &mut Self; |
| 89 | +pub fn allow_msrs(&mut self, indices: &[u32]) -> &mut Self; |
| 90 | +``` |
| 91 | + |
| 92 | +Default allow list is empty (`plan.md` §0). Drop the `unsafe` bypass. Allowing an |
| 93 | +MSR must never disable reset. Un-gate the field from `#[cfg(kvm)]` so later phases |
| 94 | +reuse it (the KVM filter wiring stays `#[cfg(kvm)]`). |
| 95 | + |
| 96 | +## Task order (small, focused commits) |
| 97 | + |
| 98 | +1. Data model. Add `CommonMsrs = Vec<MsrEntry>` with `MsrEntry { index, value, |
| 99 | + class, flags }` and the `MsrClass`/`MsrFlags` enums (`plan.md` §5). Put it near |
| 100 | + `regs/x86_64/`. `host_specific` marks `TSC`/`TSC_AUX`. |
| 101 | +2. Static table plus derivation (§3, §4). Author the per-index table (class and |
| 102 | + gating CPUID). Derive the resolved reset set from `KVM_GET_MSR_INDEX_LIST` minus |
| 103 | + the `HostOwned` class, expanded with the table. Add MTRRs from CPUID. An |
| 104 | + enumerated writable index the table cannot classify fails closed (probe get/set, |
| 105 | + add if stateful, else refuse VM creation). Cache per process with `LazyLock`. |
| 106 | +3. Backend get/set. KVM `get_msrs`/`set_msrs` over `CommonMsrs`. Short count |
| 107 | + poisons and fails closed. |
| 108 | +4. Snapshot wiring (§6). Add `Snapshot::msrs: Option<CommonMsrs>`. Capture in |
| 109 | + `snapshot()` after `get_snapshot_sregs()`, `Stateful` entries only. In |
| 110 | + `reset_vcpu`: `apply_sregs` (EFER first), then batched `SET_MSRS` of the |
| 111 | + `Stateful` set (no `host_specific` entry in the batch), then `TSC_ADJUST`. |
| 112 | + Init baseline for `msrs == None`. |
| 113 | +5. Access control (§9). Replace the `allow_msr` bool with the allow list. Build |
| 114 | + `KVM_X86_SET_MSR_FILTER` DEFAULT_DENY plus allow ranges for opted-in MSRs. |
| 115 | + Allowed MSRs pass through, denied MSRs keep the existing `#GP`/poison path. |
| 116 | +6. Validation (§10). At init, for each allowed MSR: confirm host has it (index |
| 117 | + list), confirm it is `Stateful` and settable back to baseline, else hard error. |
| 118 | + Add it to the reset set and the allow ranges. |
| 119 | +7. Tests (below). |
| 120 | + |
| 121 | +## Tests (Phase 1 subset of §12) |
| 122 | + |
| 123 | +* Fixed-list differential. For each reset-set MSR, guest writes a sentinel, |
| 124 | + `restore()`, host reads back, assert baseline. |
| 125 | +* KVM index-list sweep. Sweep `KVM_GET_MSR_INDEX_LIST`, write sentinel, restore, |
| 126 | + assert baseline. |
| 127 | +* Opt-out. Allow `KERNEL_GS_BASE`. Assert the guest reads and writes it, assert |
| 128 | + `restore()` returns it to baseline, assert a non-resettable allow request errors. |
| 129 | +* Fail closed. Default-deny installs. Non-get/settable allow errors. Simulated |
| 130 | + partial `SET_MSRS` poisons. An unclassifiable enumerated writable MSR fails at |
| 131 | + VM creation, not silently dropped. |
| 132 | +* Negative leak. Write a sentinel, `restore()`, assert it is gone. |
| 133 | + |
| 134 | +## Build and test loop |
| 135 | + |
| 136 | +```bash |
| 137 | +just guests # REQUIRED before tests, builds the guest binaries |
| 138 | +just test # debug |
| 139 | +just test release # release |
| 140 | +``` |
| 141 | + |
| 142 | +Before pushing: |
| 143 | + |
| 144 | +```bash |
| 145 | +just fmt-apply |
| 146 | +just clippy debug |
| 147 | +just clippy release |
| 148 | +just test-like-ci |
| 149 | +just test-like-ci release |
| 150 | +``` |
| 151 | + |
| 152 | +Commits must be signed and DCO signed off: `git commit -S --signoff`. Keep commits |
| 153 | +small and logically ordered. Rebase on `main`, no merge commits. Label the PR per |
| 154 | +`docs/github-labels.md`. |
| 155 | + |
| 156 | +## Done criteria |
| 157 | + |
| 158 | +* A guest MSR write to any reset-set MSR does not survive `restore()`. |
| 159 | +* An allowed MSR is readable and writable by the guest and still resets. |
| 160 | +* A non-resettable allow request is a hard init error, no `unsafe` bypass remains. |
| 161 | +* The default-deny filter installs, denied MSRs `#GP` and poison. |
| 162 | +* `just test-like-ci` and `just test-like-ci release` pass on Intel and, if |
| 163 | + available, AMD. |
| 164 | + |
| 165 | +## Pitfalls |
| 166 | + |
| 167 | +* Do not write raw `TSC`. It rewinds guest time. Reset `TSC_ADJUST` only. |
| 168 | +* Do not add `FS_BASE`/`GS_BASE` to `CommonMsrs`. They reset via `sregs`. |
| 169 | +* Do not defer an unclassified writable MSR to a nightly sweep. Fail closed. |
| 170 | +* Do not reintroduce an `unsafe` allow-all. Allowing must never disable reset. |
| 171 | +* `KVM_MSR_FILTER_DEFAULT_DENY` needs at least one range. Keep the dummy all-zero |
| 172 | + range and add real allow ranges alongside it. |
0 commit comments