Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

greq: retire new_box_zeroed in favor of FromZeroes derive #454

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

greq: retire new_box_zeroed in favor of FromZeroes derive #454

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
Loading