From fcdd15537b1466992344daf175b6975c31efb5c0 Mon Sep 17 00:00:00 2001 From: Thomas Leroy Date: Fri, 20 Dec 2024 14:37:00 +0100 Subject: [PATCH 1/3] kernel/cpu: safety comments for SMAP instructions stac and clac instructions don't break memory safety. Signed-off-by: Thomas Leroy --- kernel/src/cpu/x86/smap.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/kernel/src/cpu/x86/smap.rs b/kernel/src/cpu/x86/smap.rs index 48bfabb0d..9ec282e2f 100644 --- a/kernel/src/cpu/x86/smap.rs +++ b/kernel/src/cpu/x86/smap.rs @@ -12,6 +12,7 @@ use core::arch::asm; #[inline(always)] pub fn clac() { if !cfg!(feature = "nosmap") { + // SAFETY: `clac` instruction doesn't break memory safety. unsafe { asm!("clac", options(att_syntax, nomem, nostack, preserves_flags)) } } } @@ -22,6 +23,7 @@ pub fn clac() { #[inline(always)] pub fn stac() { if !cfg!(feature = "nosmap") { + // SAFETY: `stac` instruction doesn't break memory safety. unsafe { asm!("stac", options(att_syntax, nomem, nostack, preserves_flags)) } } } From d75651ce96e16f39b504933873ad44be709700d6 Mon Sep 17 00:00:00 2001 From: Thomas Leroy Date: Fri, 20 Dec 2024 14:38:20 +0100 Subject: [PATCH 2/3] kernel/cpu: set write_msr() unsafe Writing to some MSRs can break memory safety. Therefore, write_msr() should be unsafe. Signed-off-by: Thomas Leroy --- kernel/src/cpu/efer.rs | 11 +++++++++-- kernel/src/cpu/msr.rs | 7 ++++++- kernel/src/cpu/percpu.rs | 3 ++- kernel/src/cpu/vc.rs | 4 +++- kernel/src/hyperv/hv.rs | 7 +++++-- kernel/src/platform/native.rs | 6 ++++-- kernel/src/sev/ghcb.rs | 5 ++++- kernel/src/sev/msr_protocol.rs | 13 ++++++++----- kernel/src/task/schedule.rs | 6 +++++- 9 files changed, 46 insertions(+), 16 deletions(-) diff --git a/kernel/src/cpu/efer.rs b/kernel/src/cpu/efer.rs index 681db39a1..7d877e2dc 100644 --- a/kernel/src/cpu/efer.rs +++ b/kernel/src/cpu/efer.rs @@ -28,7 +28,14 @@ pub fn read_efer() -> EFERFlags { EFERFlags::from_bits_truncate(read_msr(EFER)) } -pub fn write_efer(efer: EFERFlags) { +/// # Safety +/// +/// The caller should ensure that the new value written to EFER MSR doesn't +/// break memory safety. +pub unsafe fn write_efer(efer: EFERFlags) { let val = efer.bits(); - write_msr(EFER, val); + // SAFETY: requirements should be verified by the caller. + unsafe { + write_msr(EFER, val); + } } diff --git a/kernel/src/cpu/msr.rs b/kernel/src/cpu/msr.rs index c328ccf76..82bb41ed4 100644 --- a/kernel/src/cpu/msr.rs +++ b/kernel/src/cpu/msr.rs @@ -27,10 +27,15 @@ pub fn read_msr(msr: u32) -> u64 { (eax as u64) | (edx as u64) << 32 } -pub fn write_msr(msr: u32, val: u64) { +/// # Safety +/// +/// The caller should ensure that the new value in the target MSR doesn't break +/// memory safety. +pub unsafe fn write_msr(msr: u32, val: u64) { let eax = (val & 0x0000_0000_ffff_ffff) as u32; let edx = (val >> 32) as u32; + // SAFETY: requirements have to be checked by the caller. unsafe { asm!("wrmsr", in("ecx") msr, diff --git a/kernel/src/cpu/percpu.rs b/kernel/src/cpu/percpu.rs index e4d75c1ec..8409f7e76 100644 --- a/kernel/src/cpu/percpu.rs +++ b/kernel/src/cpu/percpu.rs @@ -800,7 +800,8 @@ impl PerCpu { pub fn load_isst(&self) { let isst = self.isst.as_ptr(); - write_msr(ISST_ADDR, isst as u64); + // SAFETY: ISST is already setup when this is called. + unsafe { write_msr(ISST_ADDR, isst as u64) }; } pub fn load(&self) { diff --git a/kernel/src/cpu/vc.rs b/kernel/src/cpu/vc.rs index 09fb79453..3a8076956 100644 --- a/kernel/src/cpu/vc.rs +++ b/kernel/src/cpu/vc.rs @@ -577,7 +577,9 @@ mod tests { fn test_wrmsr_tsc_aux() { if is_qemu_test_env() && is_test_platform_type(SvsmPlatformType::Snp) { let test_val = 0x1234; - verify_ghcb_gets_altered(|| write_msr(MSR_TSC_AUX, test_val)); + verify_ghcb_gets_altered(|| + // SAFETY: writing to TSC MSR doesn't break memory safety. + unsafe {write_msr(MSR_TSC_AUX, test_val) }); let readback = verify_ghcb_gets_altered(|| read_msr(MSR_TSC_AUX)); assert_eq!(test_val, readback); } diff --git a/kernel/src/hyperv/hv.rs b/kernel/src/hyperv/hv.rs index 095b390ca..9c46cdb9f 100644 --- a/kernel/src/hyperv/hv.rs +++ b/kernel/src/hyperv/hv.rs @@ -190,12 +190,15 @@ pub fn hyperv_setup_hypercalls() -> Result<(), SvsmError> { .expect("Hypercall code page already allocated"); // Set the guest OS ID. The value is arbitrary. - write_msr(HyperVMsr::GuestOSID.into(), 0xC0C0C0C0); + // SAFETY: writing to HV Guest OS ID doesn't break safety. + unsafe { write_msr(HyperVMsr::GuestOSID.into(), 0xC0C0C0C0) }; // Set the hypercall code page address to the physical address of the // allocated page, and mark it enabled. let pa = virt_to_phys(page); - write_msr(HyperVMsr::Hypercall.into(), u64::from(pa) | 1); + // SAFETY: we trust the page allocator to allocate a valid page to which pa + // points. + unsafe { write_msr(HyperVMsr::Hypercall.into(), u64::from(pa) | 1) }; // Obtain the current VTL for use in future hypercalls. let vsm_status_value = get_vp_register(hyperv::HvRegisterName::VsmVpStatus)?; diff --git a/kernel/src/platform/native.rs b/kernel/src/platform/native.rs index cefcc3ec7..5585ac2d3 100644 --- a/kernel/src/platform/native.rs +++ b/kernel/src/platform/native.rs @@ -156,12 +156,14 @@ impl SvsmPlatform for NativePlatform { } fn post_irq(&self, icr: u64) -> Result<(), SvsmError> { - write_msr(APIC_MSR_ICR, icr); + // SAFETY: writing to ICR MSR doesn't break memory safety. + unsafe { write_msr(APIC_MSR_ICR, icr) }; Ok(()) } fn eoi(&self) { - write_msr(APIC_MSR_EOI, 0); + // SAFETY: writing to EOI MSR doesn't break memory safety. + unsafe { write_msr(APIC_MSR_EOI, 0) }; } fn is_external_interrupt(&self, _vector: usize) -> bool { diff --git a/kernel/src/sev/ghcb.rs b/kernel/src/sev/ghcb.rs index 5e1dc3e01..aa2ce508c 100644 --- a/kernel/src/sev/ghcb.rs +++ b/kernel/src/sev/ghcb.rs @@ -377,7 +377,10 @@ impl GHCB { // Disable interrupts between writing the MSR and making the GHCB call // to prevent reentrant use of the GHCB MSR. let guard = IrqGuard::new(); - write_msr(SEV_GHCB, ghcb_pa); + // SAFETY: GHCB is already allocated and setup so this is safe. + unsafe { + write_msr(SEV_GHCB, ghcb_pa); + } raw_vmgexit(); drop(guard); diff --git a/kernel/src/sev/msr_protocol.rs b/kernel/src/sev/msr_protocol.rs index daa8fbc2f..3dbe937ae 100644 --- a/kernel/src/sev/msr_protocol.rs +++ b/kernel/src/sev/msr_protocol.rs @@ -78,7 +78,8 @@ pub fn verify_ghcb_version() { // used. assert!(!irqs_enabled()); // Request SEV information. - write_msr(SEV_GHCB, GHCBMsr::SEV_INFO_REQ); + // SAFETY: Requesting info through the GHCB MSR protocol is safe. + unsafe { write_msr(SEV_GHCB, GHCBMsr::SEV_INFO_REQ) }; raw_vmgexit(); let sev_info = read_msr(SEV_GHCB); @@ -107,7 +108,8 @@ pub fn hypervisor_ghcb_features() -> GHCBHvFeatures { pub fn init_hypervisor_ghcb_features() -> Result<(), GhcbMsrError> { let guard = IrqGuard::new(); - write_msr(SEV_GHCB, GHCBMsr::SNP_HV_FEATURES_REQ); + // SAFETY: Requesting HV features through the GHCB MSR protocol is safe. + unsafe { write_msr(SEV_GHCB, GHCBMsr::SNP_HV_FEATURES_REQ) }; raw_vmgexit(); let result = read_msr(SEV_GHCB); drop(guard); @@ -143,7 +145,7 @@ pub fn register_ghcb_gpa_msr(addr: PhysAddr) -> Result<(), GhcbMsrError> { info |= GHCBMsr::SNP_REG_GHCB_GPA_REQ; let guard = IrqGuard::new(); - write_msr(SEV_GHCB, info); + unsafe { write_msr(SEV_GHCB, info) }; raw_vmgexit(); info = read_msr(SEV_GHCB); drop(guard); @@ -170,7 +172,7 @@ fn set_page_valid_status_msr(addr: PhysAddr, valid: bool) -> Result<(), GhcbMsrE info |= GHCBMsr::SNP_STATE_CHANGE_REQ; let guard = IrqGuard::new(); - write_msr(SEV_GHCB, info); + unsafe { write_msr(SEV_GHCB, info) }; raw_vmgexit(); let response = read_msr(SEV_GHCB); drop(guard); @@ -201,7 +203,8 @@ pub fn request_termination_msr() -> ! { // no reason to preserve interrupt state. Interrupts can be disabled // outright prior to shutdown. raw_irqs_disable(); - write_msr(SEV_GHCB, info); + // SAFETY: Requesting termination doesn't break memory safety. + unsafe { write_msr(SEV_GHCB, info) }; raw_vmgexit(); loop { halt(); diff --git a/kernel/src/task/schedule.rs b/kernel/src/task/schedule.rs index da6b6ee61..1180834db 100644 --- a/kernel/src/task/schedule.rs +++ b/kernel/src/task/schedule.rs @@ -396,7 +396,11 @@ pub fn schedule() { this_cpu().set_tss_rsp0(next.stack_bounds.end()); } if is_cet_ss_supported() { - write_msr(PL0_SSP, next.exception_shadow_stack.bits() as u64); + // SAFETY: Task::exception_shadow_stack is always initialized when + // creating a new Task. + unsafe { + write_msr(PL0_SSP, next.exception_shadow_stack.bits() as u64); + } } // Get task-pointers, consuming the Arcs and release their reference From f11a86f1ecbc33550ccaf443fe74dea15dceb5ac Mon Sep 17 00:00:00 2001 From: Thomas Leroy Date: Fri, 20 Dec 2024 14:00:33 +0100 Subject: [PATCH 3/3] kernel/cpu/gdt: add safety comments Missing safety comments were missing in GDT code. Adding a couple of checks to validate the safety requirements. Signed-off-by: Thomas Leroy --- kernel/src/cpu/gdt.rs | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/kernel/src/cpu/gdt.rs b/kernel/src/cpu/gdt.rs index 1ff46be78..0b5f29a4d 100644 --- a/kernel/src/cpu/gdt.rs +++ b/kernel/src/cpu/gdt.rs @@ -80,7 +80,18 @@ impl GDT { let tss_entries = &self.entries[idx..idx + 1].as_mut_ptr(); + // SAFETY: + // For add(): + // - idx and idx + size_of::() don't overflow isize. + // - the borrow checker guarantees that self is still allocated + // - self.entries[6:8] fits in self's allocation. + // For write_volatile(): + // - the borrow checker guarantees that self.entries is allocated + // - alignment is checked inside unsafe { + assert_eq!(align_of_val(&tss_entries.add(0)), align_of::()); + assert_eq!(align_of_val(&tss_entries.add(1)), align_of::()); + tss_entries.add(0).write_volatile(desc0); tss_entries.add(1).write_volatile(desc1); } @@ -93,11 +104,12 @@ impl GDT { pub fn load_tss(&mut self, tss: &X86Tss) { let (desc0, desc1) = tss.to_gdt_entry(); - unsafe { - self.set_tss_entry(desc0, desc1); - asm!("ltr %ax", in("ax") SVSM_TSS, options(att_syntax)); - self.clear_tss_entry() - } + self.set_tss_entry(desc0, desc1); + // SAFETY: loading task register must me done in assembly. + // It's safe to do so as long as a global GDT is in use and still + // allocated, which is always our case. + unsafe { asm!("ltr %ax", in("ax") SVSM_TSS, options(att_syntax)) }; + self.clear_tss_entry() } pub fn kernel_cs(&self) -> GDTEntry {