From cba8ca62bb56651f5c5cd5a50103588b69bcd6ee Mon Sep 17 00:00:00 2001 From: Tom Dohrmann Date: Thu, 29 Aug 2024 07:49:53 +0000 Subject: [PATCH] greq: retire new_box_zeroed in favor of FromZeroes derive The new derives allow us to use zerocopy's new_box_zeroed. The big advantage of this approach is that we don't have to write any unsafe code :) Signed-off-by: Tom Dohrmann --- Cargo.lock | 1 + Cargo.toml | 2 +- kernel/Cargo.toml | 1 + kernel/src/greq/driver.rs | 18 ++++----- kernel/src/greq/msg.rs | 79 ++++----------------------------------- 5 files changed, 19 insertions(+), 82 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1cfde9248..cdbbdec05 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -842,6 +842,7 @@ dependencies = [ "packit", "syscall", "test", + "zerocopy", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 999a54266..b7bdb4679 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -44,7 +44,7 @@ log = "0.4.17" p384 = { version = "0.13.0" } uuid = "1.6.1" # Add the derive feature by default because all crates use it. -zerocopy = { version = "0.7.32", features = ["derive"] } +zerocopy = { version = "0.7.32", features = ["alloc", "derive"] } # other repos packit = { git = "https://github.com/coconut-svsm/packit", version = "0.1.1" } diff --git a/kernel/Cargo.toml b/kernel/Cargo.toml index 0dddff43a..d2abf5a43 100644 --- a/kernel/Cargo.toml +++ b/kernel/Cargo.toml @@ -34,6 +34,7 @@ intrusive-collections.workspace = true log = { workspace = true, features = ["max_level_info", "release_max_level_info"] } packit.workspace = true libmstpm = { workspace = true, optional = true } +zerocopy.workspace = true [target."x86_64-unknown-none".dev-dependencies] test.workspace = true diff --git a/kernel/src/greq/driver.rs b/kernel/src/greq/driver.rs index f8a13f4c6..f85eb5475 100644 --- a/kernel/src/greq/driver.rs +++ b/kernel/src/greq/driver.rs @@ -13,6 +13,7 @@ extern crate alloc; use alloc::boxed::Box; use core::ptr::addr_of_mut; use core::{cell::OnceCell, mem::size_of}; +use zerocopy::FromZeroes; use crate::{ address::VirtAddr, @@ -80,22 +81,19 @@ struct SnpGuestRequestDriver { impl Drop for SnpGuestRequestDriver { fn drop(&mut self) { if self.request.set_encrypted().is_err() { - let new_req = - SnpGuestRequestMsg::boxed_new().expect("GREQ: failed to allocate request"); + let new_req = SnpGuestRequestMsg::new_box_zeroed(); let old_req = core::mem::replace(&mut self.request, new_req); log::error!("GREQ: request: failed to set page to encrypted. Memory leak!"); Box::leak(old_req); } if self.response.set_encrypted().is_err() { - let new_resp = - SnpGuestRequestMsg::boxed_new().expect("GREQ: failed to allocate response"); + let new_resp = SnpGuestRequestMsg::new_box_zeroed(); let old_resp = core::mem::replace(&mut self.response, new_resp); log::error!("GREQ: response: failed to set page to encrypted. Memory leak!"); Box::leak(old_resp); } if self.ext_data.set_encrypted().is_err() { - let new_data = - SnpGuestRequestExtData::boxed_new().expect("GREQ: failed to allocate ext_data"); + let new_data = SnpGuestRequestExtData::new_box_zeroed(); let old_data = core::mem::replace(&mut self.ext_data, new_data); log::error!("GREQ: ext_data: failed to set pages to encrypted. Memory leak!"); Box::leak(old_data); @@ -106,10 +104,10 @@ impl Drop for SnpGuestRequestDriver { impl SnpGuestRequestDriver { /// Create a new [`SnpGuestRequestDriver`] pub fn new() -> Result { - let request = SnpGuestRequestMsg::boxed_new()?; - let response = SnpGuestRequestMsg::boxed_new()?; - let staging = SnpGuestRequestMsg::boxed_new()?; - let ext_data = SnpGuestRequestExtData::boxed_new()?; + let request = SnpGuestRequestMsg::new_box_zeroed(); + let response = SnpGuestRequestMsg::new_box_zeroed(); + let staging = SnpGuestRequestMsg::new_box_zeroed(); + let ext_data = SnpGuestRequestExtData::new_box_zeroed(); let mut driver = Self { request, diff --git a/kernel/src/greq/msg.rs b/kernel/src/greq/msg.rs index c695445be..def1adadd 100644 --- a/kernel/src/greq/msg.rs +++ b/kernel/src/greq/msg.rs @@ -6,20 +6,14 @@ //! Message that carries an encrypted `SNP_GUEST_REQUEST` command in the payload -extern crate alloc; - -use alloc::{ - alloc::{alloc_zeroed, Layout}, - boxed::Box, -}; use core::{ mem::{offset_of, size_of}, ptr::{self, addr_of_mut}, - slice::{from_raw_parts, from_raw_parts_mut}, + slice::from_raw_parts, }; use crate::{ - address::{Address, VirtAddr}, + address::VirtAddr, crypto::aead::{Aes256Gcm, Aes256GcmTrait, AUTHTAG_SIZE, IV_SIZE}, mm::page_visibility::{make_page_private, make_page_shared}, protocols::errors::SvsmReqError, @@ -28,6 +22,8 @@ use crate::{ utils::MemoryRegion, }; +use zerocopy::FromZeroes; + /// Version of the message header const HDR_VERSION: u8 = 1; /// Version of the message payload @@ -74,7 +70,7 @@ pub const SNP_GUEST_REQ_MAX_DATA_SIZE: usize = 4 * PAGE_SIZE; /// `SNP_GUEST_REQUEST` message header format (AMD SEV-SNP spec. table 98) #[repr(C, packed)] -#[derive(Clone, Copy, Debug)] +#[derive(Clone, Copy, Debug, FromZeroes)] pub struct SnpGuestRequestMsgHdr { /// Message authentication tag authtag: [u8; 32], @@ -159,11 +155,6 @@ impl SnpGuestRequestMsgHdr { let slice = unsafe { from_raw_parts(ptr, size_of::()) }; &slice[algo_offset..] } - - /// Get [`SnpGuestRequestMsgHdr`] as a mutable slice reference - fn as_slice_mut(&mut self) -> &mut [u8] { - unsafe { from_raw_parts_mut(addr_of_mut!(*self).cast(), size_of::()) } - } } impl Default for SnpGuestRequestMsgHdr { @@ -190,7 +181,7 @@ impl Default for SnpGuestRequestMsgHdr { /// `SNP_GUEST_REQUEST` message format #[repr(C, align(4096))] -#[derive(Clone, Copy, Debug)] +#[derive(Clone, Copy, Debug, FromZeroes)] pub struct SnpGuestRequestMsg { hdr: SnpGuestRequestMsgHdr, pld: [u8; MSG_PAYLOAD_SIZE], @@ -200,28 +191,6 @@ pub struct SnpGuestRequestMsg { const _: () = assert!(size_of::() <= PAGE_SIZE); impl SnpGuestRequestMsg { - /// Allocate the object in the heap without going through stack as - /// this is a large object - /// - /// # Panics - /// - /// Panics if the new allocation is not page aligned. - pub fn boxed_new() -> Result, SvsmReqError> { - let layout = Layout::new::(); - - unsafe { - let addr = alloc_zeroed(layout); - if addr.is_null() { - return Err(SvsmReqError::invalid_request()); - } - - assert!(VirtAddr::from(addr).is_page_aligned()); - - let ptr = addr.cast::(); - Ok(Box::from_raw(ptr)) - } - } - /// Clear the C-bit (memory encryption bit) for the Self page /// /// # Safety @@ -242,7 +211,7 @@ impl SnpGuestRequestMsg { /// Fill the [`SnpGuestRequestMsg`] fields with zeros pub fn clear(&mut self) { - self.hdr.as_slice_mut().fill(0); + self.hdr.zero(); self.pld.fill(0); } @@ -396,7 +365,7 @@ fn set_shared_region_4k(vregion: MemoryRegion) -> Result<(), SvsmReqEr /// Data page(s) the hypervisor will use to store certificate data in /// an extended `SNP_GUEST_REQUEST` #[repr(C, align(4096))] -#[derive(Debug)] +#[derive(Debug, FromZeroes)] pub struct SnpGuestRequestExtData { /// According to the GHCB spec, the data page(s) must be contiguous pages if /// supplying more than one page and all certificate pages must be @@ -405,22 +374,6 @@ pub struct SnpGuestRequestExtData { } impl SnpGuestRequestExtData { - /// Allocate the object in the heap without going through stack as - /// this is a large object - pub fn boxed_new() -> Result, SvsmReqError> { - let layout = Layout::new::(); - unsafe { - let addr = alloc_zeroed(layout); - if addr.is_null() { - return Err(SvsmReqError::invalid_request()); - } - assert!(VirtAddr::from(addr).is_page_aligned()); - - let ptr = addr.cast::(); - Ok(Box::from_raw(ptr)) - } - } - /// Clear the C-bit (memory encryption bit) for the Self pages /// /// # Safety @@ -473,7 +426,6 @@ impl SnpGuestRequestExtData { #[cfg(test)] mod tests { use super::*; - use crate::mm::alloc::{TestRootMem, DEFAULT_TEST_MEMORY_SIZE}; #[test] fn test_snp_guest_request_hdr_offsets() { @@ -497,21 +449,6 @@ mod tests { assert_eq!(offset_of!(SnpGuestRequestMsg, pld), 0x60); } - #[test] - fn test_requestmsg_boxed_new() { - let _mem = TestRootMem::setup(DEFAULT_TEST_MEMORY_SIZE); - let mut data = SnpGuestRequestMsg::boxed_new().unwrap(); - assert!(data.hdr.as_slice_mut().iter().all(|c| *c == 0)); - assert!(data.pld.iter().all(|c| *c == 0)); - } - - #[test] - fn test_reqextdata_boxed_new() { - let _mem = TestRootMem::setup(DEFAULT_TEST_MEMORY_SIZE); - let data = SnpGuestRequestExtData::boxed_new().unwrap(); - assert!(data.data.iter().all(|c| *c == 0)); - } - #[test] fn aad_size() { let hdr = SnpGuestRequestMsgHdr::default();