From db7046d0aaa828d657337fea69923c38ba92587c Mon Sep 17 00:00:00 2001 From: Thomas Leroy Date: Wed, 29 May 2024 17:28:40 +0200 Subject: [PATCH 1/3] kernel/sev: make copy_* from SecretsPage unsafe copy_from and coy_to methods from SecretsPage copy raw content from or to arbitrary virtual addresses. Both functions should be mark unsafe to that callers are aware that argument's correctness has to be verified. Signed-off-by: Thomas Leroy --- kernel/src/sev/secrets_page.rs | 21 ++++++++++++++++++--- kernel/src/svsm.rs | 13 +++++++++++-- 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/kernel/src/sev/secrets_page.rs b/kernel/src/sev/secrets_page.rs index 49e7f4132..7f0f5486e 100644 --- a/kernel/src/sev/secrets_page.rs +++ b/kernel/src/sev/secrets_page.rs @@ -37,7 +37,7 @@ pub struct SecretsPage { impl SecretsPage { pub const fn new() -> Self { - SecretsPage { + Self { version: 0, gctxt: 0, fms: 0, @@ -57,7 +57,13 @@ impl SecretsPage { } } - pub fn copy_from(&mut self, source: VirtAddr) { + /// Copy secrets page's content pointed by a [`VirtAddr`] + /// + /// # Safety + /// + /// The caller should verify that `source` points to mapped memory whose + /// size is at least the size of the [`SecretsPage`] structure. + pub unsafe fn copy_from(&mut self, source: VirtAddr) { let from = source.as_ptr::(); unsafe { @@ -65,7 +71,16 @@ impl SecretsPage { } } - pub fn copy_to(&self, target: VirtAddr) { + /// Copy a secrets page's content to memory pointed by a [`VirtAddr`] + /// + /// # Safety + /// + /// The caller should verify that `target` points to mapped memory whose + /// size is at least the size of the [`SecretsPage`] structure. + /// + /// The caller should verify not to corrupt arbitrary memory, as this function + /// doesn't make any checks in that regard. + pub unsafe fn copy_to(&self, target: VirtAddr) { let to = target.as_mut_ptr::(); unsafe { diff --git a/kernel/src/svsm.rs b/kernel/src/svsm.rs index 3dc806ced..04c37f375 100755 --- a/kernel/src/svsm.rs +++ b/kernel/src/svsm.rs @@ -138,7 +138,10 @@ fn copy_secrets_page_to_fw(fw_addr: PhysAddr, caa_addr: PhysAddr) -> Result<(), u64::from(caa_addr), ); - fw_secrets_page.copy_to(start); + // SAFETY: start points to a new allocated and zeroed page. + unsafe { + fw_secrets_page.copy_to(start); + } Ok(()) } @@ -302,7 +305,13 @@ pub extern "C" fn svsm_start(li: &KernelLaunchInfo, vb_addr: usize) { init_cpuid_table(VirtAddr::from(launch_info.cpuid_page)); let secrets_page_virt = VirtAddr::from(launch_info.secrets_page); - secrets_page_mut().copy_from(secrets_page_virt); + + // SAFETY: the secrets page address directly comes from IGVM. + // We trust stage 2 to give the value provided by IGVM. + unsafe { + secrets_page_mut().copy_from(secrets_page_virt); + } + zero_mem_region(secrets_page_virt, secrets_page_virt + PAGE_SIZE); cr0_init(); From 1b6e5282a77eaf502a47c733cd2902f63d240715 Mon Sep 17 00:00:00 2001 From: Thomas Leroy Date: Fri, 31 May 2024 16:49:29 +0200 Subject: [PATCH 2/3] kernel/mm/guestmem: make write_u8() unsafe write_u8() can write arbitrary byte to abritrary address. Even though it already catches writes to unmapped memory, it doesn't prevent a malicious user to write at a dangerous place if it can control the parameters. It should be made unsafe. Signed-off-by: Thomas Leroy --- kernel/src/debug/gdbstub.rs | 25 ++++++++++++++++++------- kernel/src/mm/guestmem.rs | 18 ++++++++++++++++-- 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/kernel/src/debug/gdbstub.rs b/kernel/src/debug/gdbstub.rs index d4358e210..c56dc3d04 100644 --- a/kernel/src/debug/gdbstub.rs +++ b/kernel/src/debug/gdbstub.rs @@ -394,13 +394,22 @@ pub mod svsm_gdbstub { // mapping let Ok(phys) = this_cpu().get_pgtable().phys_addr(addr) else { - // The virtual address is not one that SVSM has mapped. Try safely - // writing it to the original virtual address - return write_u8(addr, value); + // The virtual address is not one that SVSM has mapped. + // Try safely writing it to the original virtual address + // SAFETY: it is up to the user to ensure that the address we + // are writing a breakpoint to is valid. + return unsafe { write_u8(addr, value) }; }; let guard = PerCPUPageMappingGuard::create_4k(phys.page_align())?; - write_u8(guard.virt_addr() + phys.page_offset(), value) + let dst = guard + .virt_addr() + .checked_add(phys.page_offset()) + .ok_or(SvsmError::InvalidAddress)?; + + // SAFETY: guard is a new mapped page, non controllable by user. + // We also checked that the destination address didn't overflow. + unsafe { write_u8(dst, value) } } } @@ -532,9 +541,11 @@ pub mod svsm_gdbstub { ) -> gdbstub::target::TargetResult<(), Self> { let start_addr = VirtAddr::from(start_addr); for (off, src) in data.iter().enumerate() { - if write_u8(start_addr + off, *src).is_err() { - return Err(TargetError::NonFatal); - } + let dst = start_addr.checked_add(off).ok_or(TargetError::NonFatal)?; + + // SAFETY: We trust the caller of this trait method to provide a valid address. + // We only cheked that start_adddr + off didn't overflow. + unsafe { write_u8(dst, *src).map_err(|_| TargetError::NonFatal)? } } Ok(()) } diff --git a/kernel/src/mm/guestmem.rs b/kernel/src/mm/guestmem.rs index 4f2a9427f..3277c0c08 100644 --- a/kernel/src/mm/guestmem.rs +++ b/kernel/src/mm/guestmem.rs @@ -39,9 +39,20 @@ pub fn read_u8(v: VirtAddr) -> Result { } } +/// Writes 1 byte at a virtual address. +/// +/// # Safety +/// +/// The caller must verify not to corrupt arbitrary memory, as this function +/// doesn't make any checks in that regard. +/// +/// # Returns +/// +/// Returns an error if the specified address is not mapped or is not mapped +/// with the appropriate write permissions. #[allow(dead_code)] #[inline] -pub fn write_u8(v: VirtAddr, val: u8) -> Result<(), SvsmError> { +pub unsafe fn write_u8(v: VirtAddr, val: u8) -> Result<(), SvsmError> { let mut rcx: u64; unsafe { @@ -244,7 +255,10 @@ mod tests { let test_address = VirtAddr::from(test_buffer.as_mut_ptr()); let data_to_write = 0x42; - write_u8(test_address, data_to_write).unwrap(); + // SAFETY: test_address points to the virtual address of test_buffer. + unsafe { + write_u8(test_address, data_to_write).unwrap(); + } assert_eq!(test_buffer[0], data_to_write); } From aabe1de64c6a029013704c6c741e66e6508f7528 Mon Sep 17 00:00:00 2001 From: Thomas Leroy Date: Tue, 11 Jun 2024 16:16:21 +0200 Subject: [PATCH 3/3] kernel/mm/guestmem: make read() and write*() unsafe Once creating a GuestPtr, the read(), write() and write_ret() methods could grant arbitrary read/write if an attacker (HV or guest) can control the GuestPtr.ptr value, and the write*() parameters. We need to verify that this is not the case for the current calls, and to enforce an unsafe block for the future calls to make the caller validating the parameters. Signed-off-by: Thomas Leroy --- kernel/src/cpu/apic.rs | 20 +++++++++++++----- kernel/src/cpu/vc.rs | 5 ++++- kernel/src/igvm_params.rs | 18 ++++++++++------ kernel/src/mm/guestmem.rs | 40 ++++++++++++++++++++++++++++++------ kernel/src/protocols/core.rs | 18 ++++++++++++---- kernel/src/protocols/vtpm.rs | 14 +++++++++++-- kernel/src/requests.rs | 15 ++++++++++++-- 7 files changed, 104 insertions(+), 26 deletions(-) diff --git a/kernel/src/cpu/apic.rs b/kernel/src/cpu/apic.rs index d4180bed4..2f350097d 100644 --- a/kernel/src/cpu/apic.rs +++ b/kernel/src/cpu/apic.rs @@ -205,7 +205,10 @@ impl LocalApic { if self.lazy_eoi_pending { if let Some(virt_addr) = caa_addr { let calling_area = GuestPtr::::new(virt_addr); - if let Ok(caa) = calling_area.read() { + // SAFETY: guest vmsa and ca are always validated before beeing updated + // (core_remap_ca(), core_create_vcpu() or prepare_fw_launch()) + // so they're safe to use. + if let Ok(caa) = unsafe { calling_area.read() } { if caa.no_eoi_required == 0 { assert!(self.isr_stack_index != 0); self.perform_eoi(); @@ -240,8 +243,11 @@ impl LocalApic { let virt_addr = caa_addr?; let calling_area = GuestPtr::::new(virt_addr); // Ignore errors here, since nothing can be done if an error occurs. - if let Ok(caa) = calling_area.read() { - let _ = calling_area.write(caa.update_no_eoi_required(0)); + // SAFETY: guest vmsa and ca are always validated before beeing updated + // (core_remap_ca(), core_create_vcpu() or prepare_fw_launch()) so + // they're safe to use. + if let Ok(caa) = unsafe { calling_area.read() } { + let _ = unsafe { calling_area.write(caa.update_no_eoi_required(0)) }; } Some(calling_area) } @@ -363,8 +369,12 @@ impl LocalApic { // delivery of the next interrupt. if self.scan_irr() == 0 { if let Some(calling_area) = guest_caa { - if let Ok(caa) = calling_area.read() { - if calling_area.write(caa.update_no_eoi_required(1)).is_ok() { + // SAFETY: guest vmsa and ca are always validated before beeing upated + // (core_remap_ca(), core_create_vcpu() or prepare_fw_launch()) + // so they're safe to use. + if let Ok(caa) = unsafe { calling_area.read() } { + if unsafe { calling_area.write(caa.update_no_eoi_required(1)).is_ok() } + { // Only track a pending lazy EOI if the // calling area page could successfully be // updated. diff --git a/kernel/src/cpu/vc.rs b/kernel/src/cpu/vc.rs index 3a6af1f63..e5587a211 100644 --- a/kernel/src/cpu/vc.rs +++ b/kernel/src/cpu/vc.rs @@ -272,7 +272,10 @@ fn vc_decode_insn(ctx: &X86ExceptionContext) -> Result, S // rip and rip+15 addresses should belong to a mapped page. // To ensure this, we rely on GuestPtr::read() that uses the exception table // to handle faults while fetching. - let insn_raw = rip.read()?; + // SAFETY: we trust the CPU-provided register state to be valid. Thus, RIP + // will point to the instruction that caused #VC to be raised, so it can + // safely be read. + let insn_raw = unsafe { rip.read()? }; let insn = Instruction::new(insn_raw); Ok(Some(insn.decode(ctx)?)) diff --git a/kernel/src/igvm_params.rs b/kernel/src/igvm_params.rs index de75bcafd..685d63f3e 100644 --- a/kernel/src/igvm_params.rs +++ b/kernel/src/igvm_params.rs @@ -196,29 +196,35 @@ impl IgvmParams<'_> { return Err(SvsmError::Firmware); } - mem_map - .offset(i as isize) - .write(IGVM_VHS_MEMORY_MAP_ENTRY { + mem_map.offset(i as isize); + // SAFETY: mem_map_va points to newly mapped memory, whose physical + // address is defined in the IGVM config. + unsafe { + mem_map.write(IGVM_VHS_MEMORY_MAP_ENTRY { starting_gpa_page_number: u64::from(entry.start()) / PAGE_SIZE as u64, number_of_pages: entry.len() as u64 / PAGE_SIZE as u64, entry_type: MemoryMapEntryType::default(), flags: 0, reserved: 0, })?; + } } // Write a zero page count into the last entry to terminate the list. let index = map.len(); if index < max_entries { - mem_map - .offset(index as isize) - .write(IGVM_VHS_MEMORY_MAP_ENTRY { + mem_map.offset(index as isize); + // SAFETY: mem_map_va points to newly mapped memory, whose physical + // address is defined in the IGVM config. + unsafe { + mem_map.write(IGVM_VHS_MEMORY_MAP_ENTRY { starting_gpa_page_number: 0, number_of_pages: 0, entry_type: MemoryMapEntryType::default(), flags: 0, reserved: 0, })?; + } } Ok(()) diff --git a/kernel/src/mm/guestmem.rs b/kernel/src/mm/guestmem.rs index 3277c0c08..643585823 100644 --- a/kernel/src/mm/guestmem.rs +++ b/kernel/src/mm/guestmem.rs @@ -200,8 +200,16 @@ impl GuestPtr { Self { ptr: p } } + /// # Safety + /// + /// The caller must verify not to read arbitrary memory, as this function + /// doesn't make any checks in that regard. + /// + /// # Returns + /// + /// Returns an error if the specified address is not mapped. #[inline] - pub fn read(&self) -> Result { + pub unsafe fn read(&self) -> Result { let mut buf = MaybeUninit::::uninit(); unsafe { @@ -210,13 +218,31 @@ impl GuestPtr { } } + /// # Safety + /// + /// The caller must verify not to corrupt arbitrary memory, as this function + /// doesn't make any checks in that regard. + /// + /// # Returns + /// + /// Returns an error if the specified address is not mapped or is not mapped + /// with the appropriate write permissions. #[inline] - pub fn write(&self, buf: T) -> Result<(), SvsmError> { + pub unsafe fn write(&self, buf: T) -> Result<(), SvsmError> { unsafe { do_movsb(&buf, self.ptr) } } + /// # Safety + /// + /// The caller must verify not to corrupt arbitrary memory, as this function + /// doesn't make any checks in that regard. + /// + /// # Returns + /// + /// Returns an error if the specified address is not mapped or is not mapped + /// with the appropriate write permissions. #[inline] - pub fn write_ref(&self, buf: &T) -> Result<(), SvsmError> { + pub unsafe fn write_ref(&self, buf: &T) -> Result<(), SvsmError> { unsafe { do_movsb(buf, self.ptr) } } @@ -269,7 +295,8 @@ mod tests { let test_buffer = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14]; let test_addr = VirtAddr::from(test_buffer.as_ptr()); let ptr: GuestPtr<[u8; 15]> = GuestPtr::new(test_addr); - let result = ptr.read().unwrap(); + // SAFETY: ptr points to test_buffer's virtual address + let result = unsafe { ptr.read().unwrap() }; assert_eq!(result, test_buffer); } @@ -279,8 +306,9 @@ mod tests { #[cfg_attr(not(test_in_svsm), ignore = "Can only be run inside guest")] fn test_read_invalid_address() { let ptr: GuestPtr = GuestPtr::new(VirtAddr::new(0xDEAD_BEEF)); - - let err = ptr.read(); + // SAFETY: ptr points to an invalid virtual address (0xDEADBEEF is + // unmapped). ptr.read() will return an error but this is expected. + let err = unsafe { ptr.read() }; assert!(err.is_err()); } } diff --git a/kernel/src/protocols/core.rs b/kernel/src/protocols/core.rs index da40e9b19..3db0fa048 100644 --- a/kernel/src/protocols/core.rs +++ b/kernel/src/protocols/core.rs @@ -338,7 +338,9 @@ fn core_pvalidate(params: &RequestParams) -> Result<(), SvsmReqError> { let start = guard.virt_addr(); let guest_page = GuestPtr::::new(start + offset); - let mut request = guest_page.read()?; + // SAFETY: start is a new mapped page address, thus valid. + // offset can't exceed a page size, so guest_page belongs to mapped memory. + let mut request = unsafe { guest_page.read()? }; let entries = request.entries; let next = request.next; @@ -356,7 +358,10 @@ fn core_pvalidate(params: &RequestParams) -> Result<(), SvsmReqError> { let guest_entries = guest_page.offset(1).cast::(); for i in next..entries { let index = i as isize; - let entry = match guest_entries.offset(index).read() { + // SAFETY: guest_entries comes from guest_page which is a new mapped + // page. index is between [next, entries) and both values have been + // validated. + let entry = match unsafe { guest_entries.offset(index).read() } { Ok(v) => v, Err(e) => { loop_result = Err(e.into()); @@ -372,7 +377,11 @@ fn core_pvalidate(params: &RequestParams) -> Result<(), SvsmReqError> { } } - if let Err(e) = guest_page.write_ref(&request) { + // SAFETY: guest_page is obtained from a guest-provided physical address + // (untrusted), so it needs to be valid (ie. belongs to the guest and only + // the guest). The physical address is validated by valid_phys_address() + // called at the beginning of SVSM_CORE_PVALIDATE handler (this one). + if let Err(e) = unsafe { guest_page.write_ref(&request) } { loop_result = Err(e.into()); } @@ -398,7 +407,8 @@ fn core_remap_ca(params: &RequestParams) -> Result<(), SvsmReqError> { let vaddr = mapping_guard.virt_addr() + offset; let pending = GuestPtr::::new(vaddr); - pending.write(SvsmCaa::zeroed())?; + // SAFETY: pending points to a new allocated page + unsafe { pending.write(SvsmCaa::zeroed())? }; // Clear any pending interrupt state before remapping the calling area to // ensure that any pending lazy EOI has been processed. diff --git a/kernel/src/protocols/vtpm.rs b/kernel/src/protocols/vtpm.rs index e3facd044..1c7ad3df8 100644 --- a/kernel/src/protocols/vtpm.rs +++ b/kernel/src/protocols/vtpm.rs @@ -230,7 +230,9 @@ fn vtpm_command_request(params: &RequestParams) -> Result<(), SvsmReqError> { // IN: platform command // OUT: platform command response size - let command = GuestPtr::::new(vaddr).read()?; + // SAFETY: vaddr comes from a new mapped region. + let command = unsafe { GuestPtr::::new(vaddr).read()? }; + let cmd = TpmPlatformCommand::try_from(command)?; if !is_vtpm_platform_command_supported(cmd) { @@ -243,7 +245,15 @@ fn vtpm_command_request(params: &RequestParams) -> Result<(), SvsmReqError> { TpmPlatformCommand::SendCommand => tpm_send_command_request(buffer)?, }; - GuestPtr::::new(vaddr).write(response_size)?; + // SAFETY: vaddr points to a new mapped region. + // if paddr + sizeof::() goes to the folowing page, it should + // not be a problem since the end of the requested region is + // (paddr + PAGE_SIZE), which requests another page. So + // write(response_size) can only happen on valid memory, mapped + // by PerCPUPageMappingGuard::create(). + unsafe { + GuestPtr::::new(vaddr).write(response_size)?; + } Ok(()) } diff --git a/kernel/src/requests.rs b/kernel/src/requests.rs index e3c382c79..25f1be202 100644 --- a/kernel/src/requests.rs +++ b/kernel/src/requests.rs @@ -120,8 +120,19 @@ fn check_requests() -> Result { let vmsa_ref = cpu.guest_vmsa_ref(); if let Some(caa_addr) = vmsa_ref.caa_addr() { let calling_area = GuestPtr::::new(caa_addr); - let caa = calling_area.read()?; - calling_area.write(caa.serviced())?; + // SAFETY: guest vmsa and ca are always validated before beeing updated + // (core_remap_ca(), core_create_vcpu() or prepare_fw_launch()) so + // they're safe to use. + let caa = unsafe { calling_area.read()? }; + + let caa_serviced = caa.serviced(); + + // SAFETY: guest vmsa is always validated before beeing updated + // (core_remap_ca() or core_create_vcpu()) so it's safe to use. + unsafe { + calling_area.write(caa_serviced)?; + } + Ok(caa.call_pending != 0) } else { Ok(false)