From 169f778853f4caa8ba78c50c2d88d0d92c84dc85 Mon Sep 17 00:00:00 2001 From: Joerg Roedel Date: Tue, 23 Jan 2024 16:49:24 +0100 Subject: [PATCH 1/5] SVSM/cpu/tss: Move GDT descriptor building to struct X86Tss Make this code a method of struct X86Tss and remove it from the open coded function. Signed-off-by: Joerg Roedel --- src/cpu/gdt.rs | 22 ++-------------------- src/cpu/tss.rs | 24 ++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 20 deletions(-) diff --git a/src/cpu/gdt.rs b/src/cpu/gdt.rs index 0d96c75fd..1eff792c5 100644 --- a/src/cpu/gdt.rs +++ b/src/cpu/gdt.rs @@ -4,7 +4,7 @@ // // Author: Joerg Roedel -use super::tss::{X86Tss, TSS_LIMIT}; +use super::tss::X86Tss; use crate::address::VirtAddr; use crate::types::{SVSM_CS, SVSM_DS, SVSM_TSS}; use core::arch::asm; @@ -31,25 +31,7 @@ static mut GDT: [u64; GDT_SIZE as usize] = [ ]; pub fn load_tss(tss: &X86Tss) { - let addr = (tss as *const X86Tss) as u64; - - let mut desc0: u64 = 0; - let mut desc1: u64 = 0; - - // Limit - desc0 |= TSS_LIMIT & 0xffffu64; - desc0 |= ((TSS_LIMIT >> 16) & 0xfu64) << 48; - - // Address - desc0 |= (addr & 0x00ff_ffffu64) << 16; - desc0 |= (addr & 0xff00_0000u64) << 32; - desc1 |= addr >> 32; - - // Present - desc0 |= 1u64 << 47; - - // Type - desc0 |= 0x9u64 << 40; + let (desc0, desc1) = tss.to_gdt_entry(); unsafe { let idx = (SVSM_TSS / 8) as usize; diff --git a/src/cpu/tss.rs b/src/cpu/tss.rs index 3f247c250..864854c8a 100644 --- a/src/cpu/tss.rs +++ b/src/cpu/tss.rs @@ -34,4 +34,28 @@ impl X86Tss { io_bmp_base: (TSS_LIMIT + 1) as u16, } } + + pub fn to_gdt_entry(&self) -> (u64, u64) { + let addr = (self as *const X86Tss) as u64; + + let mut desc0: u64 = 0; + let mut desc1: u64 = 0; + + // Limit + desc0 |= TSS_LIMIT & 0xffffu64; + desc0 |= ((TSS_LIMIT >> 16) & 0xfu64) << 48; + + // Address + desc0 |= (addr & 0x00ff_ffffu64) << 16; + desc0 |= (addr & 0xff00_0000u64) << 32; + desc1 |= addr >> 32; + + // Present + desc0 |= 1u64 << 47; + + // Type + desc0 |= 0x9u64 << 40; + + (desc0, desc1) + } } From a7aa86fe9ffb28e38778652486f905b0d390e21a Mon Sep 17 00:00:00 2001 From: Joerg Roedel Date: Tue, 23 Jan 2024 17:30:10 +0100 Subject: [PATCH 2/5] SVSM/cpu/gdt: Remove mutable global static accesses Fix the unsafe mutable accesses to the static GDT by wrapping it into a structure and add methods for manipulation. The structure is now protected by a lock which serializes mutable accesses. Signed-off-by: Joerg Roedel --- src/cpu/gdt.rs | 164 +++++++++++++++++++++++++++++++--------------- src/cpu/mod.rs | 1 + src/cpu/percpu.rs | 4 +- src/cpu/tss.rs | 5 +- src/cpu/vmsa.rs | 4 +- src/stage2.rs | 4 +- src/svsm.rs | 4 +- 7 files changed, 124 insertions(+), 62 deletions(-) diff --git a/src/cpu/gdt.rs b/src/cpu/gdt.rs index 1eff792c5..f97fcab83 100644 --- a/src/cpu/gdt.rs +++ b/src/cpu/gdt.rs @@ -6,78 +6,138 @@ use super::tss::X86Tss; use crate::address::VirtAddr; +use crate::locking::{RWLock, ReadLockGuard, WriteLockGuard}; use crate::types::{SVSM_CS, SVSM_DS, SVSM_TSS}; use core::arch::asm; use core::mem; #[repr(C, packed(2))] #[derive(Clone, Copy, Debug)] -pub struct GdtDesc { +struct GDTDesc { size: u16, addr: VirtAddr, } -const GDT_SIZE: u16 = 8; +#[derive(Copy, Clone, Debug)] +pub struct GDTEntry(u64); -static mut GDT: [u64; GDT_SIZE as usize] = [ - 0, - 0x00af9a000000ffff, // 64-bit code segment - 0x00cf92000000ffff, // 64-bit data segment - 0, // Reserved for User code - 0, // Reserver for User data - 0, // Reverved - 0, // TSS - 0, // TSS continued -]; - -pub fn load_tss(tss: &X86Tss) { - let (desc0, desc1) = tss.to_gdt_entry(); - - unsafe { - let idx = (SVSM_TSS / 8) as usize; - GDT[idx] = desc0; - GDT[idx + 1] = desc1; +impl GDTEntry { + pub const fn from_raw(entry: u64) -> Self { + Self(entry) + } + + pub fn to_raw(&self) -> u64 { + self.0 + } + + pub const fn null() -> Self { + Self(0u64) + } + + pub const fn code_64_kernel() -> Self { + Self(0x00af9a000000ffffu64) + } - asm!("ltr %ax", in("ax") SVSM_TSS, options(att_syntax)); + pub const fn data_64_kernel() -> Self { + Self(0x00cf92000000ffffu64) } } -pub fn gdt_base_limit() -> (u64, u32) { - unsafe { +const GDT_SIZE: u16 = 8; + +#[derive(Copy, Clone, Debug)] +pub struct GDT { + entries: [GDTEntry; GDT_SIZE as usize], +} + +impl GDT { + pub const fn new() -> Self { + GDT { + entries: [ + GDTEntry::null(), + GDTEntry::code_64_kernel(), + GDTEntry::data_64_kernel(), + GDTEntry::null(), + GDTEntry::null(), + GDTEntry::null(), + GDTEntry::null(), + GDTEntry::null(), + ], + } + } + + pub fn base_limit(&self) -> (u64, u32) { let gdt_entries = GDT_SIZE as usize; - let base = (&GDT as *const [u64; GDT_SIZE as usize]) as u64; + let base = (self as *const GDT) as u64; let limit = ((mem::size_of::() * gdt_entries) - 1) as u32; (base, limit) } -} -pub fn load_gdt() { - unsafe { - let gdt_desc: GdtDesc = GdtDesc { + fn descriptor(&self) -> GDTDesc { + GDTDesc { size: (GDT_SIZE * 8) - 1, - addr: VirtAddr::from(GDT.as_ptr()), - }; - - asm!(r#" /* Load GDT */ - lgdt (%rax) - - /* Reload data segments */ - movw %cx, %ds - movw %cx, %es - movw %cx, %fs - movw %cx, %gs - movw %cx, %ss - - /* Reload code segment */ - pushq %rdx - leaq 1f(%rip), %rax - pushq %rax - lretq - 1: - "#, - in("rax") &gdt_desc, - in("rdx") SVSM_CS, - in("rcx") SVSM_DS, - options(att_syntax)); + addr: VirtAddr::from(self.entries.as_ptr()), + } + } + + pub fn load(&self) { + let gdt_desc = self.descriptor(); + unsafe { + asm!(r#" /* Load GDT */ + lgdt (%rax) + + /* Reload data segments */ + movw %cx, %ds + movw %cx, %es + movw %cx, %fs + movw %cx, %gs + movw %cx, %ss + + /* Reload code segment */ + pushq %rdx + leaq 1f(%rip), %rax + pushq %rax + lretq + 1: + "#, + in("rax") &gdt_desc, + in("rdx") SVSM_CS, + in("rcx") SVSM_DS, + options(att_syntax)); + } + } + + fn set_tss_entry(&mut self, desc0: GDTEntry, desc1: GDTEntry) { + let idx = (SVSM_TSS / 8) as usize; + + self.entries[idx] = desc0; + self.entries[idx + 1] = desc1; + } + + fn clear_tss_entry(&mut self) { + let idx = (SVSM_TSS / 8) as usize; + + self.entries[idx] = GDTEntry::null(); + self.entries[idx + 1] = GDTEntry::null(); } + + pub fn load_tss(&mut self, tss: &X86Tss) { + let (desc0, desc1) = tss.to_gdt_entry(); + + self.set_tss_entry(desc0, desc1); + unsafe { + asm!("ltr %ax", in("ax") SVSM_TSS, options(att_syntax)); + } + self.clear_tss_entry() + } +} + +static GDT: RWLock = RWLock::new(GDT::new()); + +pub fn gdt() -> ReadLockGuard<'static, GDT> { + GDT.lock_read() +} + +pub fn gdt_mut() -> WriteLockGuard<'static, GDT> { + GDT.lock_write() } diff --git a/src/cpu/mod.rs b/src/cpu/mod.rs index a4db309c1..177d4b99e 100644 --- a/src/cpu/mod.rs +++ b/src/cpu/mod.rs @@ -21,6 +21,7 @@ pub mod tss; pub mod vc; pub mod vmsa; +pub use gdt::{gdt, gdt_mut}; pub use idt::common::X86ExceptionContext; pub use registers::{X86GeneralRegs, X86InterruptFrame, X86SegmentRegs}; pub use tlb::*; diff --git a/src/cpu/percpu.rs b/src/cpu/percpu.rs index 2d3e86e39..d9c526584 100644 --- a/src/cpu/percpu.rs +++ b/src/cpu/percpu.rs @@ -6,7 +6,7 @@ extern crate alloc; -use super::gdt::load_tss; +use super::gdt_mut; use super::tss::{X86Tss, IST_DF}; use crate::address::{Address, PhysAddr, VirtAddr}; use crate::cpu::tss::TSS_LIMIT; @@ -429,7 +429,7 @@ impl PerCpu { } pub fn load_tss(&mut self) { - load_tss(&self.tss); + gdt_mut().load_tss(&self.tss); } pub fn load(&mut self) { diff --git a/src/cpu/tss.rs b/src/cpu/tss.rs index 864854c8a..1fb58f5b7 100644 --- a/src/cpu/tss.rs +++ b/src/cpu/tss.rs @@ -4,6 +4,7 @@ // // Author: Joerg Roedel +use super::gdt::GDTEntry; use crate::address::VirtAddr; // IST offsets @@ -35,7 +36,7 @@ impl X86Tss { } } - pub fn to_gdt_entry(&self) -> (u64, u64) { + pub fn to_gdt_entry(&self) -> (GDTEntry, GDTEntry) { let addr = (self as *const X86Tss) as u64; let mut desc0: u64 = 0; @@ -56,6 +57,6 @@ impl X86Tss { // Type desc0 |= 0x9u64 << 40; - (desc0, desc1) + (GDTEntry::from_raw(desc0), GDTEntry::from_raw(desc1)) } } diff --git a/src/cpu/vmsa.rs b/src/cpu/vmsa.rs index 09d7a4683..5f3f2fa52 100644 --- a/src/cpu/vmsa.rs +++ b/src/cpu/vmsa.rs @@ -10,7 +10,7 @@ use cpuarch::vmsa::{VMSASegment, VMSA}; use super::control_regs::{read_cr0, read_cr3, read_cr4}; use super::efer::read_efer; -use super::gdt::gdt_base_limit; +use super::gdt; use super::idt::common::idt_base_limit; use super::msr::read_msr; @@ -33,7 +33,7 @@ fn svsm_data_segment() -> VMSASegment { } fn svsm_gdt_segment() -> VMSASegment { - let (base, limit) = gdt_base_limit(); + let (base, limit) = gdt().base_limit(); VMSASegment { selector: 0, flags: 0, diff --git a/src/stage2.rs b/src/stage2.rs index a0c32357c..af1d1a770 100755 --- a/src/stage2.rs +++ b/src/stage2.rs @@ -18,7 +18,7 @@ use svsm::address::{Address, PhysAddr, VirtAddr}; use svsm::config::SvsmConfig; use svsm::console::{init_console, install_console_logger, WRITER}; use svsm::cpu::cpuid::{dump_cpuid_table, register_cpuid_table}; -use svsm::cpu::gdt::load_gdt; +use svsm::cpu::gdt; use svsm::cpu::idt::stage2::{early_idt_init, early_idt_init_no_ghcb}; use svsm::cpu::percpu::{this_cpu_mut, PerCpu}; use svsm::elf; @@ -84,7 +84,7 @@ static CONSOLE_IO: SVSMIOPort = SVSMIOPort::new(); static CONSOLE_SERIAL: ImmutAfterInitCell = ImmutAfterInitCell::uninit(); fn setup_env(config: &SvsmConfig) { - load_gdt(); + gdt().load(); early_idt_init_no_ghcb(); install_console_logger("Stage2"); diff --git a/src/svsm.rs b/src/svsm.rs index 40fa2a9d1..562714d29 100755 --- a/src/svsm.rs +++ b/src/svsm.rs @@ -22,7 +22,7 @@ use svsm::console::{init_console, install_console_logger, WRITER}; use svsm::cpu::control_regs::{cr0_init, cr4_init}; use svsm::cpu::cpuid::{dump_cpuid_table, register_cpuid_table}; use svsm::cpu::efer::efer_init; -use svsm::cpu::gdt::load_gdt; +use svsm::cpu::gdt; use svsm::cpu::idt::svsm::{early_idt_init, idt_init}; use svsm::cpu::percpu::PerCpu; use svsm::cpu::percpu::{this_cpu, this_cpu_mut}; @@ -286,7 +286,7 @@ pub extern "C" fn svsm_start(li: &KernelLaunchInfo, vb_addr: usize) { init_valid_bitmap_ptr(new_kernel_region(&launch_info), vb_ptr); - load_gdt(); + gdt().load(); early_idt_init(); // Capture the debug serial port before the launch info disappears from From cf3d97d078553795d4360d1314df0dea30f9a01d Mon Sep 17 00:00:00 2001 From: Joerg Roedel Date: Wed, 24 Jan 2024 11:02:12 +0100 Subject: [PATCH 3/5] SVSM/cpu/idt: Remove GLOBAL_IDT and add safe accessors Replace the GLOBAL_IDT static variable with a non-public global struct which can only be accessed via safe locking. This removes all mutable accesses to a global static for the IDT case. Signed-off-by: Joerg Roedel --- src/cpu/idt/common.rs | 63 ++++++++++++++++++++++++++++++++++--------- src/cpu/idt/mod.rs | 2 ++ src/cpu/idt/stage2.rs | 24 +++++------------ src/cpu/idt/svsm.rs | 29 +++++++++----------- src/cpu/vmsa.rs | 4 +-- 5 files changed, 73 insertions(+), 49 deletions(-) diff --git a/src/cpu/idt/common.rs b/src/cpu/idt/common.rs index a72dda964..f06c7933f 100644 --- a/src/cpu/idt/common.rs +++ b/src/cpu/idt/common.rs @@ -6,6 +6,7 @@ use crate::address::{Address, VirtAddr}; use crate::cpu::registers::{X86GeneralRegs, X86InterruptFrame}; +use crate::locking::{RWLock, ReadLockGuard, WriteLockGuard}; use crate::types::SVSM_CS; use core::arch::{asm, global_asm}; use core::mem; @@ -104,27 +105,63 @@ struct IdtDesc { address: VirtAddr, } -pub type Idt = [IdtEntry; IDT_ENTRIES]; +#[derive(Copy, Clone, Debug)] +pub struct IDT { + entries: [IdtEntry; IDT_ENTRIES], +} -pub static mut GLOBAL_IDT: Idt = [IdtEntry::no_handler(); IDT_ENTRIES]; +impl IDT { + pub const fn new() -> Self { + IDT { + entries: [IdtEntry::no_handler(); IDT_ENTRIES], + } + } -pub fn idt_base_limit() -> (u64, u32) { - unsafe { - let base = (&GLOBAL_IDT as *const Idt) as u64; + pub fn init(&mut self, handler_array: *const u8, size: usize) -> &mut Self { + // Set IDT handlers + let handlers = VirtAddr::from(handler_array); + + for idx in 0..size { + self.set_entry(idx, IdtEntry::entry(handlers + (32 * idx))); + } + + self + } + + pub fn set_entry(&mut self, idx: usize, entry: IdtEntry) -> &mut Self { + self.entries[idx] = entry; + + self + } + + pub fn load(&self) -> &Self { + let desc: IdtDesc = IdtDesc { + size: (IDT_ENTRIES * 16) as u16, + address: VirtAddr::from(self.entries.as_ptr()), + }; + + unsafe { + asm!("lidt (%rax)", in("rax") &desc, options(att_syntax)); + } + + self + } + + pub fn base_limit(&self) -> (u64, u32) { + let base = (self as *const IDT) as u64; let limit = (IDT_ENTRIES * mem::size_of::()) as u32; (base, limit) } } -pub fn load_idt(idt: &Idt) { - let desc: IdtDesc = IdtDesc { - size: (IDT_ENTRIES * 16) as u16, - address: VirtAddr::from(idt.as_ptr()), - }; +static IDT: RWLock = RWLock::new(IDT::new()); - unsafe { - asm!("lidt (%rax)", in("rax") &desc, options(att_syntax)); - } +pub fn idt() -> ReadLockGuard<'static, IDT> { + IDT.lock_read() +} + +pub fn idt_mut() -> WriteLockGuard<'static, IDT> { + IDT.lock_write() } pub fn triple_fault() { diff --git a/src/cpu/idt/mod.rs b/src/cpu/idt/mod.rs index d707cdb7d..c80def1c5 100644 --- a/src/cpu/idt/mod.rs +++ b/src/cpu/idt/mod.rs @@ -7,3 +7,5 @@ pub mod common; pub mod stage2; pub mod svsm; + +pub use common::{idt, idt_mut}; diff --git a/src/cpu/idt/stage2.rs b/src/cpu/idt/stage2.rs index bc2f09ea9..6b00b6421 100644 --- a/src/cpu/idt/stage2.rs +++ b/src/cpu/idt/stage2.rs @@ -4,35 +4,25 @@ // // Author: Joerg Roedel -use super::common::{load_idt, Idt, IdtEntry, DF_VECTOR, GLOBAL_IDT, HV_VECTOR, VC_VECTOR}; -use crate::address::VirtAddr; +use super::common::{idt_mut, DF_VECTOR, HV_VECTOR, VC_VECTOR}; use crate::cpu::control_regs::read_cr2; use crate::cpu::vc::{stage2_handle_vc_exception, stage2_handle_vc_exception_no_ghcb}; use crate::cpu::X86ExceptionContext; use core::arch::global_asm; -fn init_idt(idt: &mut Idt, handler_array: *const u8) { - // Set IDT handlers - let handlers = VirtAddr::from(handler_array); - for (i, entry) in idt.iter_mut().enumerate() { - *entry = IdtEntry::entry(handlers + (32 * i)); - } -} - pub fn early_idt_init_no_ghcb() { unsafe { - init_idt( - &mut GLOBAL_IDT, - &stage2_idt_handler_array_no_ghcb as *const u8, - ); - load_idt(&GLOBAL_IDT); + idt_mut() + .init(&stage2_idt_handler_array_no_ghcb as *const u8, 32) + .load(); } } pub fn early_idt_init() { unsafe { - init_idt(&mut GLOBAL_IDT, &stage2_idt_handler_array as *const u8); - load_idt(&GLOBAL_IDT); + idt_mut() + .init(&stage2_idt_handler_array as *const u8, 32) + .load(); } } diff --git a/src/cpu/idt/svsm.rs b/src/cpu/idt/svsm.rs index 3b4efd6e2..260666dc9 100644 --- a/src/cpu/idt/svsm.rs +++ b/src/cpu/idt/svsm.rs @@ -11,39 +11,34 @@ use super::super::tss::IST_DF; use super::super::vc::handle_vc_exception; use super::common::PF_ERROR_WRITE; use super::common::{ - load_idt, Idt, IdtEntry, BP_VECTOR, DF_VECTOR, GLOBAL_IDT, GP_VECTOR, HV_VECTOR, PF_VECTOR, - VC_VECTOR, + idt_mut, IdtEntry, BP_VECTOR, DF_VECTOR, GP_VECTOR, HV_VECTOR, PF_VECTOR, VC_VECTOR, }; use crate::address::VirtAddr; use crate::cpu::X86ExceptionContext; use crate::debug::gdbstub::svsm_gdbstub::handle_debug_exception; use core::arch::global_asm; -fn init_idt(idt: &mut Idt) { - // Set IDT handlers - let handlers = unsafe { VirtAddr::from(&svsm_idt_handler_array as *const u8) }; - for (i, entry) in idt.iter_mut().enumerate() { - *entry = IdtEntry::entry(handlers + (32 * i)); +fn init_ist_vectors() { + unsafe { + let handler = VirtAddr::from(&svsm_idt_handler_array as *const u8) + (32 * DF_VECTOR); + idt_mut().set_entry( + DF_VECTOR, + IdtEntry::ist_entry(handler, IST_DF.try_into().unwrap()), + ); } } -unsafe fn init_ist_vectors(idt: &mut Idt) { - let handler = VirtAddr::from(&svsm_idt_handler_array as *const u8) + (32 * DF_VECTOR); - idt[DF_VECTOR] = IdtEntry::ist_entry(handler, IST_DF.try_into().unwrap()); -} - pub fn early_idt_init() { unsafe { - init_idt(&mut GLOBAL_IDT); - load_idt(&GLOBAL_IDT); + idt_mut() + .init(&svsm_idt_handler_array as *const u8, 32) + .load(); } } pub fn idt_init() { // Set IST vectors - unsafe { - init_ist_vectors(&mut GLOBAL_IDT); - } + init_ist_vectors(); } #[no_mangle] diff --git a/src/cpu/vmsa.rs b/src/cpu/vmsa.rs index 5f3f2fa52..b83fdbe5d 100644 --- a/src/cpu/vmsa.rs +++ b/src/cpu/vmsa.rs @@ -11,7 +11,7 @@ use cpuarch::vmsa::{VMSASegment, VMSA}; use super::control_regs::{read_cr0, read_cr3, read_cr4}; use super::efer::read_efer; use super::gdt; -use super::idt::common::idt_base_limit; +use super::idt::common::idt; use super::msr::read_msr; fn svsm_code_segment() -> VMSASegment { @@ -43,7 +43,7 @@ fn svsm_gdt_segment() -> VMSASegment { } fn svsm_idt_segment() -> VMSASegment { - let (base, limit) = idt_base_limit(); + let (base, limit) = idt().base_limit(); VMSASegment { selector: 0, flags: 0, From 4a05245cce00bb6ed1e4c6b514df7accd2cffb40 Mon Sep 17 00:00:00 2001 From: Joerg Roedel Date: Wed, 24 Jan 2024 12:05:14 +0100 Subject: [PATCH 4/5] SVSM/sev/secrets_page: Remove mutable global static Remove handling around SECRETS_PAGE and replace it with a decent struct and locking. That gets rid the accesses to the global mutable SECRETE_PAGE variable. Signed-off-by: Joerg Roedel --- src/greq/driver.rs | 23 ++++---- src/sev/mod.rs | 1 + src/sev/secrets_page.rs | 114 ++++++++++++++++++++++++++++++---------- src/svsm.rs | 58 +++++++------------- 4 files changed, 116 insertions(+), 80 deletions(-) diff --git a/src/greq/driver.rs b/src/greq/driver.rs index a8ea47db2..de44a8816 100644 --- a/src/greq/driver.rs +++ b/src/greq/driver.rs @@ -21,10 +21,7 @@ use crate::{ greq::msg::{SnpGuestRequestExtData, SnpGuestRequestMsg, SnpGuestRequestMsgType}, locking::SpinLock, protocols::errors::{SvsmReqError, SvsmResultCode}, - sev::{ - ghcb::GhcbError, - secrets_page::{disable_vmpck0, get_vmpck0, is_vmpck0_clear, VMPCK_SIZE}, - }, + sev::{ghcb::GhcbError, secrets_page, secrets_page_mut, VMPCK_SIZE}, types::PAGE_SHIFT, BIT, }; @@ -189,7 +186,7 @@ impl SnpGuestRequestDriver { command_len: usize, ) -> Result<(), SvsmReqError> { // VMPL0 `SNP_GUEST_REQUEST` commands are encrypted with the VMPCK0 key - let vmpck0: [u8; VMPCK_SIZE] = get_vmpck0(); + let vmpck0: [u8; VMPCK_SIZE] = secrets_page().get_vmpck(0); let inbuf = buffer .get(..command_len) @@ -210,7 +207,7 @@ impl SnpGuestRequestDriver { msg_type: SnpGuestRequestMsgType, buffer: &mut [u8], ) -> Result { - let vmpck0: [u8; VMPCK_SIZE] = get_vmpck0(); + let vmpck0: [u8; VMPCK_SIZE] = secrets_page().get_vmpck(0); // For security reasons, decrypt the message in protected memory (staging) *self.staging = *self.response; @@ -223,7 +220,7 @@ impl SnpGuestRequestDriver { // The buffer provided is too small to store the unwrapped response. // There is no need to clear the VMPCK0, just report it as invalid parameter. SvsmReqError::RequestError(SvsmResultCode::INVALID_PARAMETER) => (), - _ => disable_vmpck0(), + _ => secrets_page_mut().clear_vmpck(0), } } @@ -255,7 +252,7 @@ impl SnpGuestRequestDriver { buffer: &mut [u8], command_len: usize, ) -> Result { - if is_vmpck0_clear() { + if secrets_page().is_vmpck_clear(0) { return Err(SvsmReqError::invalid_request()); } @@ -264,7 +261,7 @@ impl SnpGuestRequestDriver { // The sequence number is restored only when the guest is rebooted. let Some(msg_seqno) = self.seqno_last_used().checked_add(1) else { log::error!("SNP_GUEST_REQUEST: sequence number overflow"); - disable_vmpck0(); + secrets_page_mut().clear_vmpck(0); return Err(SvsmReqError::invalid_request()); }; @@ -287,14 +284,14 @@ impl SnpGuestRequestDriver { log::error!( "SNP_GUEST_REQ_INVALID_LEN. Aborting, request resend failed" ); - disable_vmpck0(); + secrets_page_mut().clear_vmpck(0); return Err(e1); } return Err(e); } else { // We sent a regular SNP_GUEST_REQUEST, but the hypervisor returned // an error code that is exclusive for extended SNP_GUEST_REQUEST - disable_vmpck0(); + secrets_page_mut().clear_vmpck(0); return Err(SvsmReqError::invalid_request()); } } @@ -302,7 +299,7 @@ impl SnpGuestRequestDriver { SNP_GUEST_REQ_ERR_BUSY => { if let Err(e2) = self.send(req_class) { log::error!("SNP_GUEST_REQ_ERR_BUSY. Aborting, request resend failed"); - disable_vmpck0(); + secrets_page_mut().clear_vmpck(0); return Err(e2); } // ... request resend worked, continue normally. @@ -311,7 +308,7 @@ impl SnpGuestRequestDriver { // the AMD SEV-SNP spec or in the linux kernel include/uapi/linux/psp-sev.h _ => { log::error!("SNP_GUEST_REQUEST failed, unknown error code={}\n", info2); - disable_vmpck0(); + secrets_page_mut().clear_vmpck(0); return Err(e); } } diff --git a/src/sev/mod.rs b/src/sev/mod.rs index 5ac65cf29..a3ef7bfab 100644 --- a/src/sev/mod.rs +++ b/src/sev/mod.rs @@ -13,6 +13,7 @@ pub mod vmsa; pub mod utils; pub use msr_protocol::init_hypervisor_ghcb_features; +pub use secrets_page::{secrets_page, secrets_page_mut, SecretsPage, VMPCK_SIZE}; pub use status::sev_status_init; pub use status::sev_status_verify; pub use status::{sev_es_enabled, sev_snp_enabled}; diff --git a/src/sev/secrets_page.rs b/src/sev/secrets_page.rs index 9e8198b5b..7ad773ff1 100644 --- a/src/sev/secrets_page.rs +++ b/src/sev/secrets_page.rs @@ -5,52 +5,110 @@ // Author: Joerg Roedel use crate::address::VirtAddr; +use crate::locking::{RWLock, ReadLockGuard, WriteLockGuard}; use crate::sev::vmsa::VMPL_MAX; +use crate::types::GUEST_VMPL; -pub const VMPCK_SIZE: usize = 32; +extern crate alloc; +use alloc::boxed::Box; -extern "C" { - pub static mut SECRETS_PAGE: SecretsPage; -} +pub const VMPCK_SIZE: usize = 32; #[derive(Copy, Clone, Debug)] #[repr(C, packed)] pub struct SecretsPage { - pub version: u32, - pub gctxt: u32, - pub fms: u32, + version: u32, + gctxt: u32, + fms: u32, reserved_00c: u32, - pub gosvw: [u8; 16], - pub vmpck: [[u8; VMPCK_SIZE]; VMPL_MAX], + gosvw: [u8; 16], + vmpck: [[u8; VMPCK_SIZE]; VMPL_MAX], reserved_0a0: [u8; 96], - pub vmsa_tweak_bmp: [u64; 8], - pub svsm_base: u64, - pub svsm_size: u64, - pub svsm_caa: u64, - pub svsm_max_version: u32, - pub svsm_guest_vmpl: u8, + vmsa_tweak_bmp: [u64; 8], + svsm_base: u64, + svsm_size: u64, + svsm_caa: u64, + svsm_max_version: u32, + svsm_guest_vmpl: u8, reserved_15d: [u8; 3], - pub tsc_factor: u32, + tsc_factor: u32, reserved_164: [u8; 3740], } -pub fn copy_secrets_page(target: &mut SecretsPage, source: VirtAddr) { - let table = source.as_ptr::(); +impl SecretsPage { + pub const fn new() -> Self { + SecretsPage { + version: 0, + gctxt: 0, + fms: 0, + reserved_00c: 0, + gosvw: [0; 16], + vmpck: [[0; VMPCK_SIZE]; VMPL_MAX], + reserved_0a0: [0; 96], + vmsa_tweak_bmp: [0; 8], + svsm_base: 0, + svsm_size: 0, + svsm_caa: 0, + svsm_max_version: 0, + svsm_guest_vmpl: 0, + reserved_15d: [0; 3], + tsc_factor: 0, + reserved_164: [0; 3740], + } + } + + pub fn copy_from(&mut self, source: VirtAddr) { + let from = source.as_ptr::(); - unsafe { - *target = *table; + unsafe { + *self = *from; + } } -} -pub fn is_vmpck0_clear() -> bool { - unsafe { SECRETS_PAGE.vmpck[0].iter().all(|e| *e == 0) } + pub fn copy_to(&self, target: VirtAddr) { + let to = target.as_mut_ptr::(); + + unsafe { + *to = *self; + } + } + + pub fn copy_for_vmpl(&self, vmpl: usize) -> Box { + let mut sp = Box::new(*self); + for idx in 0..vmpl { + sp.clear_vmpck(idx); + } + + sp + } + + pub fn set_svsm_data(&mut self, base: u64, size: u64, caa_addr: u64) { + self.svsm_base = base; + self.svsm_size = size; + self.svsm_caa = caa_addr; + self.svsm_max_version = 1; + self.svsm_guest_vmpl = GUEST_VMPL as u8; + } + + pub fn get_vmpck(&self, idx: usize) -> [u8; VMPCK_SIZE] { + self.vmpck[idx] + } + + pub fn is_vmpck_clear(&self, idx: usize) -> bool { + self.vmpck[idx].iter().all(|e| *e == 0) + } + + pub fn clear_vmpck(&mut self, idx: usize) { + self.vmpck[idx].iter_mut().for_each(|e| *e = 0); + } } -pub fn disable_vmpck0() { - unsafe { SECRETS_PAGE.vmpck[0].iter_mut().for_each(|e| *e = 0) }; - log::warn!("VMPCK0 disabled!"); +static SECRETS_PAGE: RWLock = RWLock::new(SecretsPage::new()); + +pub fn secrets_page() -> ReadLockGuard<'static, SecretsPage> { + SECRETS_PAGE.lock_read() } -pub fn get_vmpck0() -> [u8; VMPCK_SIZE] { - unsafe { SECRETS_PAGE.vmpck[0] } +pub fn secrets_page_mut() -> WriteLockGuard<'static, SecretsPage> { + SECRETS_PAGE.lock_write() } diff --git a/src/svsm.rs b/src/svsm.rs index 562714d29..3cc7f3796 100755 --- a/src/svsm.rs +++ b/src/svsm.rs @@ -43,9 +43,8 @@ use svsm::mm::virtualrange::virt_log_usage; use svsm::mm::{init_kernel_mapping_info, PerCPUPageMappingGuard}; use svsm::requests::{request_loop, update_mappings}; use svsm::serial::SerialPort; -use svsm::sev::secrets_page::{copy_secrets_page, disable_vmpck0, SecretsPage}; use svsm::sev::utils::{rmp_adjust, RMPFlags}; -use svsm::sev::{init_hypervisor_ghcb_features, sev_status_init}; +use svsm::sev::{init_hypervisor_ghcb_features, secrets_page, secrets_page_mut, sev_status_init}; use svsm::svsm_console::SVSMIOPort; use svsm::svsm_paging::{init_page_table, invalidate_early_boot_memory}; use svsm::task::{create_task, TASK_FLAG_SHARE_PT}; @@ -57,7 +56,6 @@ use svsm::mm::validate::{init_valid_bitmap_ptr, migrate_valid_bitmap}; use core::ptr; extern "C" { - pub static mut SECRETS_PAGE: SecretsPage; pub static bsp_stack_end: u8; } @@ -92,11 +90,6 @@ global_asm!( bsp_stack: .fill 4*4096, 1, 0 bsp_stack_end: - - .align 4096 - .globl SECRETS_PAGE - SECRETS_PAGE: - .fill 4096, 1, 0 "#, options(att_syntax) ); @@ -127,35 +120,21 @@ fn copy_secrets_page_to_fw(fw_addr: PhysAddr, caa_addr: PhysAddr) -> Result<(), let guard = PerCPUPageMappingGuard::create_4k(fw_addr)?; let start = guard.virt_addr(); - let mut target = ptr::NonNull::new(start.as_mut_ptr::()).unwrap(); - // Zero target - unsafe { - let mut page_ptr = target.cast::(); - ptr::write_bytes(page_ptr.as_mut(), 0, PAGE_SIZE); - } + zero_mem_region(start, start + PAGE_SIZE); - // Copy and initialize data - unsafe { - let dst = target.as_ptr(); - *dst = SECRETS_PAGE; + // Copy secrets page + let mut fw_secrets_page = secrets_page().copy_for_vmpl(GUEST_VMPL); - // Copy Table - let fw_sp = target.as_mut(); + let &li = &*LAUNCH_INFO; - // Zero VMPCK key for VMPLs with more privileges than the guest - for vmpck in fw_sp.vmpck.iter_mut().take(GUEST_VMPL) { - vmpck.fill(0); - } - - let &li = &*LAUNCH_INFO; + fw_secrets_page.set_svsm_data( + li.kernel_region_phys_start, + li.kernel_region_phys_end - li.kernel_region_phys_start, + u64::from(caa_addr), + ); - fw_sp.svsm_base = li.kernel_region_phys_start; - fw_sp.svsm_size = li.kernel_region_phys_end - li.kernel_region_phys_start; - fw_sp.svsm_caa = u64::from(caa_addr); - fw_sp.svsm_max_version = 1; - fw_sp.svsm_guest_vmpl = GUEST_VMPL as u8; - } + fw_secrets_page.copy_to(start); Ok(()) } @@ -176,7 +155,7 @@ pub fn copy_tables_to_fw(fw_meta: &SevFWMetaData) -> Result<(), SvsmError> { let secrets_page = match fw_meta.secrets_page { Some(addr) => addr, - None => panic!("FW does not specify SECRETS_PAGE location"), + None => panic!("FW does not specify secrets-page location"), }; let caa_page = match fw_meta.caa_page { @@ -306,11 +285,9 @@ pub extern "C" fn svsm_start(li: &KernelLaunchInfo, vb_addr: usize) { register_cpuid_table(&CPUID_PAGE); dump_cpuid_table(); - unsafe { - let secrets_page_virt = VirtAddr::from(launch_info.secrets_page); - copy_secrets_page(&mut SECRETS_PAGE, secrets_page_virt); - zero_mem_region(secrets_page_virt, secrets_page_virt + PAGE_SIZE); - } + let secrets_page_virt = VirtAddr::from(launch_info.secrets_page); + secrets_page_mut().copy_from(secrets_page_virt); + zero_mem_region(secrets_page_virt, secrets_page_virt + PAGE_SIZE); cr0_init(); cr4_init(); @@ -464,7 +441,10 @@ pub extern "C" fn svsm_main() { #[panic_handler] fn panic(info: &PanicInfo) -> ! { - disable_vmpck0(); + secrets_page_mut().clear_vmpck(0); + secrets_page_mut().clear_vmpck(1); + secrets_page_mut().clear_vmpck(2); + secrets_page_mut().clear_vmpck(3); log::error!("Panic: CPU[{}] {}", this_cpu().get_apic_id(), info); From 5ffc83e1fdc9a1d4eec02a4d6e049b67007b6c66 Mon Sep 17 00:00:00 2001 From: Joerg Roedel Date: Wed, 24 Jan 2024 12:40:28 +0100 Subject: [PATCH 5/5] SVSM/mm/alloc: Fix compile warning With recent nightly compilers this warning appears: warning: field `0` is never read --> src/mm/alloc.rs:1321:28 | 1321 | pub struct TestRootMem<'a>(LockGuard<'a, ()>); | ----------- ^^^^^^^^^^^^^^^^^ | | | field in this struct | Silence this warning. Signed-off-by: Joerg Roedel --- src/mm/alloc.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/mm/alloc.rs b/src/mm/alloc.rs index 421b70dc5..2d0a8c68f 100644 --- a/src/mm/alloc.rs +++ b/src/mm/alloc.rs @@ -1318,6 +1318,7 @@ pub const DEFAULT_TEST_MEMORY_SIZE: usize = 16usize * 1024 * 1024; /// A dummy struct to acquire a lock over global memory for tests. #[cfg(any(test, fuzzing))] #[derive(Debug)] +#[allow(dead_code)] pub struct TestRootMem<'a>(LockGuard<'a, ()>); #[cfg(any(test, fuzzing))]