From a84fa74bd0a56d4ef61c09e46a7bbc4b0f260902 Mon Sep 17 00:00:00 2001 From: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com> Date: Mon, 16 Mar 2026 16:03:57 -0700 Subject: [PATCH] Disallow MSR read/write on KVM Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com> --- Cargo.lock | 6 +- src/hyperlight_host/Cargo.toml | 4 +- src/hyperlight_host/src/error.rs | 19 ++++- .../src/hypervisor/hyperlight_vm/mod.rs | 24 ++++++ .../src/hypervisor/hyperlight_vm/x86_64.rs | 9 ++- .../hypervisor/virtual_machine/kvm/x86_64.rs | 80 ++++++++++++++++++- .../src/hypervisor/virtual_machine/mod.rs | 11 +++ src/hyperlight_host/src/sandbox/config.rs | 26 ++++++ .../src/sandbox/initialized_multi_use.rs | 72 +++++++++++++++++ src/tests/rust_guests/simpleguest/src/main.rs | 30 +++++++ 10 files changed, 270 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 09bb85aa0..6f231efa2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1857,8 +1857,7 @@ dependencies = [ [[package]] name = "kvm-bindings" version = "0.14.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4b3c06ff73c7ce03e780887ec2389d62d2a2a9ddf471ab05c2ff69207cd3f3b4" +source = "git+https://github.com/rust-vmm/kvm?rev=3ffc9b62af5978553f73cc0ec79fad13fdd47146#3ffc9b62af5978553f73cc0ec79fad13fdd47146" dependencies = [ "vmm-sys-util", ] @@ -1866,8 +1865,7 @@ dependencies = [ [[package]] name = "kvm-ioctls" version = "0.24.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "333f77a20344a448f3f70664918135fddeb804e938f28a99d685bd92926e0b19" +source = "git+https://github.com/rust-vmm/kvm?rev=3ffc9b62af5978553f73cc0ec79fad13fdd47146#3ffc9b62af5978553f73cc0ec79fad13fdd47146" dependencies = [ "bitflags 2.11.0", "kvm-bindings", diff --git a/src/hyperlight_host/Cargo.toml b/src/hyperlight_host/Cargo.toml index 9352aef1b..8dc76cda2 100644 --- a/src/hyperlight_host/Cargo.toml +++ b/src/hyperlight_host/Cargo.toml @@ -75,8 +75,8 @@ windows-version = "0.1" lazy_static = "1.4.0" [target.'cfg(unix)'.dependencies] -kvm-bindings = { version = "0.14", features = ["fam-wrappers"], optional = true } -kvm-ioctls = { version = "0.24", optional = true } +kvm-bindings = { git = "https://github.com/rust-vmm/kvm", rev = "3ffc9b62af5978553f73cc0ec79fad13fdd47146", features = ["fam-wrappers"], optional = true } +kvm-ioctls = { git = "https://github.com/rust-vmm/kvm", rev = "3ffc9b62af5978553f73cc0ec79fad13fdd47146", optional = true } mshv-bindings = { version = "0.6", optional = true } mshv-ioctls = { version = "0.6", optional = true} diff --git a/src/hyperlight_host/src/error.rs b/src/hyperlight_host/src/error.rs index 831792cc3..ef55d93e7 100644 --- a/src/hyperlight_host/src/error.rs +++ b/src/hyperlight_host/src/error.rs @@ -158,6 +158,16 @@ pub enum HyperlightError { #[error("Memory Access Violation at address {0:#x} of type {1}, but memory is marked as {2}")] MemoryAccessViolation(u64, MemoryRegionFlags, MemoryRegionFlags), + /// MSR Read Violation. Guest attempted to read from a Model-Specific Register + #[cfg(all(kvm, target_arch = "x86_64"))] + #[error("Guest attempted to read from MSR {0:#x}")] + MsrReadViolation(u32), + + /// MSR Write Violation. Guest attempted to write to a Model-Specific Register + #[cfg(all(kvm, target_arch = "x86_64"))] + #[error("Guest attempted to write {1:#x} to MSR {0:#x}")] + MsrWriteViolation(u32, u64), + /// Memory Allocation Failed. #[error("Memory Allocation Failed with OS Error {0:?}.")] MemoryAllocationFailed(Option), @@ -338,8 +348,13 @@ impl HyperlightError { | HyperlightError::ExecutionCanceledByHost() | HyperlightError::PoisonedSandbox | HyperlightError::ExecutionAccessViolation(_) - | HyperlightError::MemoryAccessViolation(_, _, _) - | HyperlightError::SnapshotSizeMismatch(_, _) + | HyperlightError::MemoryAccessViolation(_, _, _) => true, + + #[cfg(all(kvm, target_arch = "x86_64"))] + HyperlightError::MsrReadViolation(_) + | HyperlightError::MsrWriteViolation(_, _) => true, + + HyperlightError::SnapshotSizeMismatch(_, _) | HyperlightError::MemoryRegionSizeMismatch(_, _, _) // HyperlightVmError::Restore is already handled manually in restore(), but we mark it // as poisoning here too for defense in depth. diff --git a/src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs b/src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs index 4fa9fba36..6c9430190 100644 --- a/src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs +++ b/src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs @@ -165,6 +165,16 @@ impl DispatchGuestCallError { region_flags, }) => HyperlightError::MemoryAccessViolation(addr, access_type, region_flags), + #[cfg(all(kvm, target_arch = "x86_64"))] + DispatchGuestCallError::Run(RunVmError::MsrReadViolation(msr_index)) => { + HyperlightError::MsrReadViolation(msr_index) + } + + #[cfg(all(kvm, target_arch = "x86_64"))] + DispatchGuestCallError::Run(RunVmError::MsrWriteViolation { msr_index, value }) => { + HyperlightError::MsrWriteViolation(msr_index, value) + } + // Leave others as is other => HyperlightVmError::DispatchGuestCall(other).into(), }; @@ -215,6 +225,12 @@ pub enum RunVmError { MmioReadUnmapped(u64), #[error("MMIO WRITE access to unmapped address {0:#x}")] MmioWriteUnmapped(u64), + #[cfg(all(kvm, target_arch = "x86_64"))] + #[error("Guest attempted to read from MSR {0:#x}")] + MsrReadViolation(u32), + #[cfg(all(kvm, target_arch = "x86_64"))] + #[error("Guest attempted to write {value:#x} to MSR {msr_index:#x}")] + MsrWriteViolation { msr_index: u32, value: u64 }, #[error("vCPU run failed: {0}")] RunVcpu(#[from] RunVcpuError), #[error("Unexpected VM exit: {0}")] @@ -658,6 +674,14 @@ impl HyperlightVm { } } } + #[cfg(all(kvm, target_arch = "x86_64"))] + Ok(VmExit::MsrRead(msr_index)) => { + break Err(RunVmError::MsrReadViolation(msr_index)); + } + #[cfg(all(kvm, target_arch = "x86_64"))] + Ok(VmExit::MsrWrite { msr_index, value }) => { + break Err(RunVmError::MsrWriteViolation { msr_index, value }); + } Ok(VmExit::Cancelled()) => { // If cancellation was not requested for this specific guest function call, // the vcpu was interrupted by a stale cancellation. This can occur when: diff --git a/src/hyperlight_host/src/hypervisor/hyperlight_vm/x86_64.rs b/src/hyperlight_host/src/hypervisor/hyperlight_vm/x86_64.rs index 57b09fc20..74bf9b562 100644 --- a/src/hyperlight_host/src/hypervisor/hyperlight_vm/x86_64.rs +++ b/src/hyperlight_host/src/hypervisor/hyperlight_vm/x86_64.rs @@ -91,7 +91,14 @@ impl HyperlightVm { let vm: VmType = match get_available_hypervisor() { #[cfg(kvm)] - Some(HypervisorType::Kvm) => Box::new(KvmVm::new().map_err(VmError::CreateVm)?), + Some(HypervisorType::Kvm) => { + let kvm_vm = KvmVm::new().map_err(VmError::CreateVm)?; + #[cfg(target_arch = "x86_64")] + if !config.get_allow_msr() { + kvm_vm.enable_msr_filter().map_err(VmError::CreateVm)?; + } + Box::new(kvm_vm) + } #[cfg(mshv3)] Some(HypervisorType::Mshv) => Box::new(MshvVm::new().map_err(VmError::CreateVm)?), #[cfg(target_os = "windows")] diff --git a/src/hyperlight_host/src/hypervisor/virtual_machine/kvm/x86_64.rs b/src/hyperlight_host/src/hypervisor/virtual_machine/kvm/x86_64.rs index db7b235e1..18763beb7 100644 --- a/src/hyperlight_host/src/hypervisor/virtual_machine/kvm/x86_64.rs +++ b/src/hyperlight_host/src/hypervisor/virtual_machine/kvm/x86_64.rs @@ -19,10 +19,14 @@ use std::sync::LazyLock; #[cfg(gdb)] use kvm_bindings::kvm_guest_debug; use kvm_bindings::{ - kvm_debugregs, kvm_fpu, kvm_regs, kvm_sregs, kvm_userspace_memory_region, kvm_xsave, + kvm_debugregs, kvm_enable_cap, kvm_fpu, kvm_regs, kvm_sregs, kvm_userspace_memory_region, + kvm_xsave, }; use kvm_ioctls::Cap::UserMemory; -use kvm_ioctls::{Kvm, VcpuExit, VcpuFd, VmFd}; +use kvm_ioctls::{ + Cap, Kvm, MsrExitReason, MsrFilterDefaultAction, MsrFilterRange, MsrFilterRangeFlags, VcpuExit, + VcpuFd, VmFd, +}; use tracing::{Span, instrument}; #[cfg(feature = "trace_guest")] use tracing_opentelemetry::OpenTelemetrySpanExt; @@ -136,6 +140,44 @@ impl KvmVm { debug_regs: kvm_guest_debug::default(), }) } + + /// Enable MSR filtering: tell KVM to exit to userspace on filtered MSR + /// access, then install a deny-all filter so every RDMSR/WRMSR traps. + /// + /// Requires KVM_CAP_X86_USER_SPACE_MSR and KVM_CAP_X86_MSR_FILTER + pub(crate) fn enable_msr_filter(&self) -> std::result::Result<(), CreateVmError> { + let hv = KVM.as_ref().map_err(|e| e.clone())?; + if !hv.check_extension(Cap::X86UserSpaceMsr) || !hv.check_extension(Cap::X86MsrFilter) { + tracing::error!( + "KVM does not support KVM_CAP_X86_USER_SPACE_MSR or KVM_CAP_X86_MSR_FILTER." + ); + return Err(CreateVmError::MsrFilterNotSupported); + } + + let cap = kvm_enable_cap { + cap: Cap::X86UserSpaceMsr as u32, + args: [MsrExitReason::Filter.bits() as u64, 0, 0, 0], + ..Default::default() + }; + self.vm_fd + .enable_cap(&cap) + .map_err(|e| CreateVmError::InitializeVm(e.into()))?; + + // At least one range is required when using KVM_MSR_FILTER_DEFAULT_DENY. + let bitmap = [0u8; 1]; // 1 byte covers 8 MSRs, all bits 0 (deny) + self.vm_fd + .set_msr_filter( + MsrFilterDefaultAction::DENY, + &[MsrFilterRange { + flags: MsrFilterRangeFlags::READ | MsrFilterRangeFlags::WRITE, + base: 0, + msr_count: 1, + bitmap: &bitmap, + }], + ) + .map_err(|e| CreateVmError::InitializeVm(e.into()))?; + Ok(()) + } } impl VirtualMachine for KvmVm { @@ -176,6 +218,40 @@ impl VirtualMachine for KvmVm { Ok(VcpuExit::IoOut(port, data)) => Ok(VmExit::IoOut(port, data.to_vec())), Ok(VcpuExit::MmioRead(addr, _)) => Ok(VmExit::MmioRead(addr)), Ok(VcpuExit::MmioWrite(addr, _)) => Ok(VmExit::MmioWrite(addr)), + // KVM_EXIT_X86_RDMSR / KVM_EXIT_X86_WRMSR (KVM API ยง5, kvm_run structure): + // + // The "index" field tells userspace which MSR the guest wants to + // read/write. If the request was unsuccessful, userspace indicates + // that with a "1" in the "error" field. "This will inject a #GP + // into the guest when the VCPU is executed again." + // + // "for KVM_EXIT_IO, KVM_EXIT_MMIO, [...] KVM_EXIT_X86_RDMSR and + // KVM_EXIT_X86_WRMSR the corresponding operations are complete + // (and guest state is consistent) only after userspace has + // re-entered the kernel with KVM_RUN." + // + // We set error=1 and then re-run with `immediate_exit` to let KVM + // inject the #GP without executing further guest code. From the + // kvm_run docs: "[immediate_exit] is polled once when KVM_RUN + // starts; if non-zero, KVM_RUN exits immediately, returning + // -EINTR." + Ok(VcpuExit::X86Rdmsr(msr_exit)) => { + let msr_index = msr_exit.index; + *msr_exit.error = 1; + self.vcpu_fd.set_kvm_immediate_exit(1); + let _ = self.vcpu_fd.run(); + self.vcpu_fd.set_kvm_immediate_exit(0); + Ok(VmExit::MsrRead(msr_index)) + } + Ok(VcpuExit::X86Wrmsr(msr_exit)) => { + let msr_index = msr_exit.index; + let value = msr_exit.data; + *msr_exit.error = 1; + self.vcpu_fd.set_kvm_immediate_exit(1); + let _ = self.vcpu_fd.run(); + self.vcpu_fd.set_kvm_immediate_exit(0); + Ok(VmExit::MsrWrite { msr_index, value }) + } #[cfg(gdb)] Ok(VcpuExit::Debug(debug_exit)) => Ok(VmExit::Debug { dr6: debug_exit.dr6, diff --git a/src/hyperlight_host/src/hypervisor/virtual_machine/mod.rs b/src/hyperlight_host/src/hypervisor/virtual_machine/mod.rs index b3bed769e..46d5b79d1 100644 --- a/src/hyperlight_host/src/hypervisor/virtual_machine/mod.rs +++ b/src/hyperlight_host/src/hypervisor/virtual_machine/mod.rs @@ -135,6 +135,12 @@ pub(crate) enum VmExit { MmioRead(u64), /// The vCPU tried to write to the given (unmapped) addr MmioWrite(u64), + /// The vCPU tried to read from the given MSR + #[cfg(all(kvm, target_arch = "x86_64"))] + MsrRead(u32), + /// The vCPU tried to write to the given MSR with the given value + #[cfg(all(kvm, target_arch = "x86_64"))] + MsrWrite { msr_index: u32, value: u64 }, /// The vCPU execution has been cancelled Cancelled(), /// The vCPU has exited for a reason that is not handled by Hyperlight @@ -179,6 +185,11 @@ pub enum CreateVmError { HypervisorNotAvailable(HypervisorError), #[error("Initialize VM failed: {0}")] InitializeVm(HypervisorError), + #[cfg(all(kvm, target_arch = "x86_64"))] + #[error( + "KVM MSR filtering not supported (requires KVM_CAP_X86_USER_SPACE_MSR and KVM_CAP_X86_MSR_FILTER)" + )] + MsrFilterNotSupported, #[error("Set Partition Property failed: {0}")] SetPartitionProperty(HypervisorError), #[cfg(target_os = "windows")] diff --git a/src/hyperlight_host/src/sandbox/config.rs b/src/hyperlight_host/src/sandbox/config.rs index f12387a0b..2361d4f5c 100644 --- a/src/hyperlight_host/src/sandbox/config.rs +++ b/src/hyperlight_host/src/sandbox/config.rs @@ -74,6 +74,9 @@ pub struct SandboxConfiguration { interrupt_vcpu_sigrtmin_offset: u8, /// How much writable memory to offer the guest scratch_size: usize, + /// Allow MSR (Model Specific Register) access. This is disabled by default for security reasons. + #[cfg(all(kvm, target_arch = "x86_64"))] + allow_msr: bool, } impl SandboxConfiguration { @@ -118,6 +121,8 @@ impl SandboxConfiguration { guest_debug_info, #[cfg(crashdump)] guest_core_dump, + #[cfg(all(kvm, target_arch = "x86_64"))] + allow_msr: false, } } @@ -159,6 +164,27 @@ impl SandboxConfiguration { self.interrupt_vcpu_sigrtmin_offset } + /// Set whether MSR access is allowed. By default, MSR access is disabled + /// for security reasons. This setting only applies when using KVM. It is a no-op on MSHV and WHP. + /// + /// # Safety + /// + /// If enabled, MSR intercepts are not installed, which means guest-modified + /// MSRs are not reset across snapshot restores. This can leak state + /// between guest executions within the same sandbox. + #[cfg(all(kvm, target_arch = "x86_64"))] + #[instrument(skip_all, parent = Span::current(), level= "Trace")] + pub unsafe fn set_allow_msr(&mut self, allow_msr: bool) { + self.allow_msr = allow_msr; + } + + /// Get whether MSR access is allowed + #[cfg(all(kvm, target_arch = "x86_64"))] + #[instrument(skip_all, parent = Span::current(), level= "Trace")] + pub(crate) fn get_allow_msr(&self) -> bool { + self.allow_msr + } + /// Sets the offset from `SIGRTMIN` to determine the real-time signal used for /// interrupting the VCPU thread. /// diff --git a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs index 97044c521..e9c2c672b 100644 --- a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs +++ b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs @@ -2011,4 +2011,76 @@ mod tests { let _ = std::fs::remove_file(&path); } + + #[test] + #[cfg(all(kvm, target_arch = "x86_64"))] + fn test_msr_read_write_denied() { + use crate::hypervisor::virtual_machine::{HypervisorType, get_available_hypervisor}; + + match get_available_hypervisor() { + Some(HypervisorType::Kvm) => {} + _ => { + return; + } + } + + let mut sbox = UninitializedSandbox::new( + GuestBinary::FilePath(simple_guest_as_string().expect("Guest Binary Missing")), + None, + ) + .unwrap() + .evolve() + .unwrap(); + + let snapshot = sbox.snapshot().unwrap(); + let msr_index: u32 = 0xC000_0102; // IA32_KERNEL_GS_BASE + + // RDMSR should be intercepted + let result = sbox.call::("ReadMSR", msr_index); + assert!( + matches!( + &result, + Err(HyperlightError::MsrReadViolation(idx)) if *idx == msr_index + ), + "RDMSR 0x{:X}: expected MsrReadViolation, got: {:?}", + msr_index, + result + ); + assert!(sbox.poisoned()); + + // Restore before next call + sbox.restore(snapshot.clone()).unwrap(); + + // WRMSR should be intercepted + let result = sbox.call::<()>("WriteMSR", (msr_index, 0x5u64)); + assert!( + matches!( + &result, + Err(HyperlightError::MsrWriteViolation(idx, _)) if *idx == msr_index + ), + "WRMSR 0x{:X}: expected MsrWriteViolation, got: {:?}", + msr_index, + result + ); + assert!(sbox.poisoned()); + + // Also verify that MSR access works when explicitly allowed + let mut cfg = SandboxConfiguration::default(); + unsafe { cfg.set_allow_msr(true) }; + + let mut sbox = UninitializedSandbox::new( + GuestBinary::FilePath(simple_guest_as_string().expect("Guest Binary Missing")), + Some(cfg), + ) + .unwrap() + .evolve() + .unwrap(); + + let msr_index: u32 = 0xC000_0102; // IA32_KERNEL_GS_BASE + let value: u64 = 0x5; + + sbox.call::<()>("WriteMSR", (msr_index, value)).unwrap(); + let read_value: u64 = sbox.call("ReadMSR", msr_index).unwrap(); + assert_eq!(read_value, value); + } } diff --git a/src/tests/rust_guests/simpleguest/src/main.rs b/src/tests/rust_guests/simpleguest/src/main.rs index 6d53c94c6..6fd38885b 100644 --- a/src/tests/rust_guests/simpleguest/src/main.rs +++ b/src/tests/rust_guests/simpleguest/src/main.rs @@ -690,6 +690,36 @@ fn call_host_expect_error(hostfuncname: String) -> Result<()> { Ok(()) } +#[guest_function("ReadMSR")] +fn read_msr(msr: u32) -> u64 { + let (read_eax, read_edx): (u32, u32); + unsafe { + core::arch::asm!( + "rdmsr", + in("ecx") msr, + out("eax") read_eax, + out("edx") read_edx, + options(nostack, nomem) + ); + } + ((read_edx as u64) << 32) | (read_eax as u64) +} + +#[guest_function("WriteMSR")] +fn write_msr(msr: u32, value: u64) { + let eax = (value & 0xFFFFFFFF) as u32; + let edx = ((value >> 32) & 0xFFFFFFFF) as u32; + unsafe { + core::arch::asm!( + "wrmsr", + in("ecx") msr, + in("eax") eax, + in("edx") edx, + options(nostack, nomem) + ); + } +} + #[no_mangle] #[instrument(skip_all, parent = Span::current(), level= "Trace")] pub extern "C" fn hyperlight_main() {