Skip to content

Commit

Permalink
greq: retire new_box_zeroed in favor of FromZeroes derive
Browse files Browse the repository at this point in the history
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 :)
  • Loading branch information
Freax13 committed Aug 29, 2024
1 parent 6d8a3e9 commit 174274d
Show file tree
Hide file tree
Showing 5 changed files with 19 additions and 82 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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" }
Expand Down
1 change: 1 addition & 0 deletions kernel/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
18 changes: 8 additions & 10 deletions kernel/src/greq/driver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand All @@ -106,10 +104,10 @@ impl Drop for SnpGuestRequestDriver {
impl SnpGuestRequestDriver {
/// Create a new [`SnpGuestRequestDriver`]
pub fn new() -> Result<Self, SvsmReqError> {
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,
Expand Down
79 changes: 8 additions & 71 deletions kernel/src/greq/msg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
Expand Down Expand Up @@ -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],
Expand Down Expand Up @@ -159,11 +155,6 @@ impl SnpGuestRequestMsgHdr {
let slice = unsafe { from_raw_parts(ptr, size_of::<Self>()) };
&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::<Self>()) }
}
}

impl Default for SnpGuestRequestMsgHdr {
Expand All @@ -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],
Expand All @@ -200,28 +191,6 @@ pub struct SnpGuestRequestMsg {
const _: () = assert!(size_of::<SnpGuestRequestMsg>() <= 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<Box<Self>, SvsmReqError> {
let layout = Layout::new::<Self>();

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::<Self>();
Ok(Box::from_raw(ptr))
}
}

/// Clear the C-bit (memory encryption bit) for the Self page
///
/// # Safety
Expand All @@ -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);
}

Expand Down Expand Up @@ -396,7 +365,7 @@ fn set_shared_region_4k(vregion: MemoryRegion<VirtAddr>) -> 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
Expand All @@ -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<Box<Self>, SvsmReqError> {
let layout = Layout::new::<Self>();
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::<Self>();
Ok(Box::from_raw(ptr))
}
}

/// Clear the C-bit (memory encryption bit) for the Self pages
///
/// # Safety
Expand Down Expand Up @@ -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() {
Expand All @@ -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();
Expand Down

0 comments on commit 174274d

Please sign in to comment.