From b2bd86e43b516fa293268f0f701b9c6e2201bfa5 Mon Sep 17 00:00:00 2001 From: Sam Leffler Date: Thu, 23 Jun 2022 03:19:08 +0000 Subject: [PATCH] kata-security-coordinator::fakeimpl: correct LoadModel & LoadApplication Change the fake to behave as the real impl will for LoadModel & LoadApplication: return a deep copy of the saved package contents as would happen if the data were pulled from flash. Match this behaviour in the kata-shell SecurityCoordinator test commands and the MlCoordinator by taking ownership of the received objects and free'ing them when no longer needed. With these changes one can install a bundle and repeatedly load_application without leaking any memory, capabilities, or slots in the toplevel CNode of the caller (DebugConsole in this case). Likewise doing install of a model, test_mlexecute, and then uninstall of the model's bundle returns all resources. Specific changes: - correctly release resources in kata-shell load_application & load_model - correct release of bundle_frames in seL4BundleImpl::stop - release resources in MlCoordinator::load_model - connect the MemoryInterface to the MlCoordinator so it can return memory - setup two copyregions in the SecurityCoordinator to do the deep copy - add ObjdDescBundle::cptr_iter for iterating over the set of seL4_CPtr's - hack kata_frame_alloc_in_cnode to split requests according to the kernel's config on the max Retype count - while here switch test_mailbox to use one of the copyregions TODO: - deep_copy allocates all frames at once which requires a band-aid; either hide that in MemoryManager or maybe allocate a page at a time Change-Id: Ia425976b31ea7a32b1d0e4affc3a0ef9ba966c87 GitOrigin-RevId: 31d5bc99b569a5eab9c33c7e1014793bfe57161e --- .../src/test_security_coordinator.rs | 17 ++- .../kata-memory-interface/src/lib.rs | 22 ++- .../MlCoordinator/MlCoordinator.camkes | 2 + .../kata-ml-coordinator/src/lib.rs | 10 +- .../kata-proc-manager/src/sel4bundle/mod.rs | 2 +- .../SecurityCoordinator.camkes | 5 +- .../src/fakeimpl/mod.rs | 127 ++++++++++++------ apps/system/system.camkes | 4 +- 8 files changed, 136 insertions(+), 53 deletions(-) diff --git a/apps/system/components/DebugConsole/kata-shell/src/test_security_coordinator.rs b/apps/system/components/DebugConsole/kata-shell/src/test_security_coordinator.rs index 5a8122f..34ed2f4 100644 --- a/apps/system/components/DebugConsole/kata-shell/src/test_security_coordinator.rs +++ b/apps/system/components/DebugConsole/kata-shell/src/test_security_coordinator.rs @@ -8,6 +8,7 @@ use crate::CommandError; use crate::HashMap; use kata_io as io; +use kata_memory_interface::kata_object_free_in_cnode; use kata_os_common::cspace_slot::CSpaceSlot; use kata_security_interface::*; use kata_storage_interface::KEY_VALUE_DATA_SIZE; @@ -76,9 +77,13 @@ fn load_application_command( _builtin_cpio: &[u8], ) -> Result<(), CommandError> { let bundle_id = args.next().ok_or(CommandError::BadArgs)?; - let container_slot = CSpaceSlot::new(); + let mut container_slot = CSpaceSlot::new(); match kata_security_load_application(bundle_id, &container_slot) { - Ok(frames) => writeln!(output, "{:?}", &frames)?, + Ok(frames) => { + container_slot.release(); // NB: take ownership + writeln!(output, "{:?}", &frames)?; + let _ = kata_object_free_in_cnode(&frames); + }, Err(status) => writeln!(output, "LoadApplication failed: {:?}", status)?, } Ok(()) @@ -92,9 +97,13 @@ fn load_model_command( ) -> Result<(), CommandError> { let bundle_id = args.next().ok_or(CommandError::BadArgs)?; let model_id = args.next().ok_or(CommandError::BadArgs)?; - let container_slot = CSpaceSlot::new(); + let mut container_slot = CSpaceSlot::new(); match kata_security_load_model(bundle_id, model_id, &container_slot) { - Ok(frames) => writeln!(output, "{:?}", &frames)?, + Ok(frames) => { + container_slot.release(); // NB: take ownership + writeln!(output, "{:?}", &frames)?; + let _ = kata_object_free_in_cnode(&frames); + } Err(status) => writeln!(output, "LoadApplication failed: {:?}", status)?, } Ok(()) diff --git a/apps/system/components/MemoryManager/kata-memory-interface/src/lib.rs b/apps/system/components/MemoryManager/kata-memory-interface/src/lib.rs index e9b982c..8fd4a97 100644 --- a/apps/system/components/MemoryManager/kata-memory-interface/src/lib.rs +++ b/apps/system/components/MemoryManager/kata-memory-interface/src/lib.rs @@ -184,6 +184,11 @@ impl ObjDescBundle { } } + // Returns an iterator that enumerates each object's seL4_CPtr. + pub fn cptr_iter(&self) -> impl Iterator + '_ { + self.objs.iter().flat_map(|od| od.cptr..(od.cptr + od.retype_count())) + } + // Move objects to dynamically-allocated slots in the top-level // CNode and mutate the Object Descriptor with the new cptr's. // TODO(sleffler) make generic (requires supplying slot allocator)? @@ -584,9 +589,20 @@ pub fn kata_frame_alloc_in_cnode(space_bytes: usize) } // NB: always allocate 4K pages let npages = howmany(space_bytes, 1 << seL4_PageBits); - kata_object_alloc_in_cnode(vec![ - ObjDesc::new( seL4_RISCV_4K_Page, npages, /*cptr=*/ 0) - ]) + // XXX horrible band-aid to workaround Retype "fanout" limit of 256 + // objects: split our request accordingly. This shold be handled in + // MemoryManager using the kernel config or bump the kernel limit. + assert!(npages <= 512); // XXX 2MB + if npages > 256 { + kata_object_alloc_in_cnode(vec![ + ObjDesc::new( seL4_RISCV_4K_Page, 256, /*cptr=*/ 0), + ObjDesc::new( seL4_RISCV_4K_Page, npages - 256, /*cptr=*/ 256), + ]) + } else { + kata_object_alloc_in_cnode(vec![ + ObjDesc::new( seL4_RISCV_4K_Page, npages, /*cptr=*/ 0), + ]) + } } #[inline] diff --git a/apps/system/components/MlCoordinator/MlCoordinator.camkes b/apps/system/components/MlCoordinator/MlCoordinator.camkes index f107435..0328fae 100644 --- a/apps/system/components/MlCoordinator/MlCoordinator.camkes +++ b/apps/system/components/MlCoordinator/MlCoordinator.camkes @@ -1,4 +1,5 @@ import ; +import ; import ; import ; @@ -19,6 +20,7 @@ component MlCoordinator { dataport Buf(0x1000000) dtcm; uses LoggerInterface logger; + uses MemoryInterface memory; uses SecurityCoordinatorInterface security; // Enable KataOS CAmkES support. diff --git a/apps/system/components/MlCoordinator/kata-ml-coordinator/src/lib.rs b/apps/system/components/MlCoordinator/kata-ml-coordinator/src/lib.rs index e106d63..ed205fc 100644 --- a/apps/system/components/MlCoordinator/kata-ml-coordinator/src/lib.rs +++ b/apps/system/components/MlCoordinator/kata-ml-coordinator/src/lib.rs @@ -6,6 +6,7 @@ extern crate alloc; use alloc::string::String; use alloc::vec::Vec; +use kata_memory_interface::kata_object_free_in_cnode; use kata_ml_interface::MlCoordError; use kata_os_common::cspace_slot::CSpaceSlot; use kata_security_interface::*; @@ -89,10 +90,11 @@ impl MLCoordinator { // To load the model into the vector core the pages must be // mapped into the MlCoordinator's VSpace before being copied // to their destination. - let container_slot = CSpaceSlot::new(); + let mut container_slot = CSpaceSlot::new(); match kata_security_load_model(&model.bundle_id, &model.model_id, &container_slot) { Ok(model_frames) => { - match MlCore::load_image(&model_frames) { + container_slot.release(); // NB: take ownership + let ret_status = match MlCore::load_image(&model_frames) { Err(e) => { error!( "Load of {}:{} failed: {:?}", @@ -109,7 +111,9 @@ impl MLCoordinator { self.loaded_model = Some(model_idx); Ok(()) } - } + }; + let _ = kata_object_free_in_cnode(&model_frames); + ret_status } Err(e) => { error!( diff --git a/apps/system/components/ProcessManager/kata-proc-manager/src/sel4bundle/mod.rs b/apps/system/components/ProcessManager/kata-proc-manager/src/sel4bundle/mod.rs index a375995..a23b4ab 100644 --- a/apps/system/components/ProcessManager/kata-proc-manager/src/sel4bundle/mod.rs +++ b/apps/system/components/ProcessManager/kata-proc-manager/src/sel4bundle/mod.rs @@ -592,7 +592,7 @@ impl BundleImplInterface for seL4BundleImpl { } fn stop(&mut self) -> Result<(), ProcessManagerError> { self.suspend()?; - kata_object_free(&self.bundle_frames) + kata_object_free_in_cnode(&self.bundle_frames) .map_err(|_| ProcessManagerError::StopFailed)?; kata_object_free_in_cnode(&self.dynamic_objs) .map_err(|_| ProcessManagerError::StopFailed)?; diff --git a/apps/system/components/SecurityCoordinator/SecurityCoordinator.camkes b/apps/system/components/SecurityCoordinator/SecurityCoordinator.camkes index 0df2672..c690f2d 100644 --- a/apps/system/components/SecurityCoordinator/SecurityCoordinator.camkes +++ b/apps/system/components/SecurityCoordinator/SecurityCoordinator.camkes @@ -18,6 +18,7 @@ component SecurityCoordinator { // Add free slots for processing requests. attribute int cnode_headroom = 32; - // For mailbox use. What should we call this? - has copyregion COPYREGION; + // For fakeimpl deep_copy (re-used by test_mailbox). + has copyregion DEEP_COPY_SRC; + has copyregion DEEP_COPY_DEST; } diff --git a/apps/system/components/SecurityCoordinator/kata-security-coordinator/src/fakeimpl/mod.rs b/apps/system/components/SecurityCoordinator/kata-security-coordinator/src/fakeimpl/mod.rs index 17510b8..7f5ab7f 100644 --- a/apps/system/components/SecurityCoordinator/kata-security-coordinator/src/fakeimpl/mod.rs +++ b/apps/system/components/SecurityCoordinator/kata-security-coordinator/src/fakeimpl/mod.rs @@ -4,13 +4,32 @@ extern crate alloc; use alloc::fmt; use alloc::string::{String, ToString}; use core::mem::size_of; +use core::ptr; use hashbrown::HashMap; -use kata_memory_interface::*; -use kata_os_common::sel4_sys::*; +use kata_memory_interface::kata_frame_alloc; +use kata_memory_interface::kata_frame_alloc_in_cnode; +use kata_memory_interface::kata_object_free_in_cnode; +use kata_memory_interface::kata_object_free_toplevel; +use kata_memory_interface::ObjDescBundle; +use kata_os_common::copyregion::CopyRegion; +use kata_os_common::cspace_slot::CSpaceSlot; +use kata_os_common::sel4_sys; use kata_security_interface::*; use kata_storage_interface::KeyValueData; use log::trace; +use sel4_sys::seL4_Page_GetAddress; +use sel4_sys::seL4_PageBits; +use sel4_sys::seL4_Word; + +const PAGE_SIZE: usize = 1 << seL4_PageBits; + +extern "C" { + // Regions for deep_copy work. + static mut DEEP_COPY_SRC: [seL4_Word; PAGE_SIZE / size_of::()]; + static mut DEEP_COPY_DEST: [seL4_Word; PAGE_SIZE / size_of::()]; +} + struct BundleData { pkg_contents: ObjDescBundle, pkg_size: usize, @@ -19,10 +38,9 @@ struct BundleData { } impl BundleData { fn new(pkg_contents: &ObjDescBundle) -> Self { - let size_bytes = pkg_contents.objs.len() * 4096; // XXX BundleData { pkg_contents: pkg_contents.clone(), - pkg_size: size_bytes, + pkg_size: pkg_contents.size_bytes(), manifest: String::from( r##" # Comments like this @@ -80,6 +98,47 @@ impl FakeSecurityCoordinator { } pub type KataSecurityCoordinatorInterface = FakeSecurityCoordinator; +// Returns a deep copy (including seL4 objects) of |src|. The container +// CNode is in the toplevel (allocated from the slot allocator). +fn deep_copy(src: &ObjDescBundle) -> ObjDescBundle { + let dest = kata_frame_alloc_in_cnode(src.size_bytes()) + .expect("deep_copy:alloc"); + // Src top-level slot & copy region + let src_slot = CSpaceSlot::new(); + let mut src_region = CopyRegion::new( + unsafe { ptr::addr_of_mut!(DEEP_COPY_SRC[0]) }, PAGE_SIZE + ); + // Dest top-level slot & copy region + let dest_slot = CSpaceSlot::new(); + let mut dest_region = CopyRegion::new( + unsafe { ptr::addr_of_mut!(DEEP_COPY_DEST[0]) }, PAGE_SIZE + ); + for (src_cptr, dest_cptr) in src.cptr_iter().zip(dest.cptr_iter()) { + // Map src & dest frames and copy data. + src_slot.copy_to(src.cnode, src_cptr, src.depth) + .and_then(|_| src_region.map(src_slot.slot)) + .expect("src_map"); + dest_slot.copy_to(dest.cnode, dest_cptr, dest.depth) + .and_then(|_| dest_region.map(dest_slot.slot)) + .expect("dest_map"); + + unsafe { + ptr::copy_nonoverlapping( + src_region.as_ref().as_ptr(), + dest_region.as_mut().as_mut_ptr(), + PAGE_SIZE, + ); + } + + // Unmap & clear top-level slot required for mapping. + src_region.unmap().and_then(|_| src_slot.delete()) + .expect("src_unmap"); + dest_region.unmap().and_then(|_| dest_slot.delete()) + .expect("dest_unmap"); + } + dest +} + impl SecurityCoordinatorInterface for FakeSecurityCoordinator { fn install(&mut self, pkg_contents: &ObjDescBundle) -> Result { // TODO(sleffler): get bundle_id from the manifest; for now use the @@ -108,8 +167,10 @@ impl SecurityCoordinatorInterface for FakeSecurityCoordinator { } fn load_application(&self, bundle_id: &str) -> Result { let bundle_data = self.get_bundle(bundle_id)?; + // Clone everything (struct + associated seL4 objects) so the + // return is as though it was newly instantiated from flash. // XXX just return the package for now - Ok(bundle_data.pkg_contents.clone()) + Ok(deep_copy(&bundle_data.pkg_contents)) } fn load_model( &self, @@ -118,8 +179,10 @@ impl SecurityCoordinatorInterface for FakeSecurityCoordinator { ) -> Result { let bundle_data = self.get_bundle(bundle_id)?; // TODO(sleffler): check model id + // Clone everything (struct + associated seL4 objects) so the + // return is as though it was newly instantiated from flash. // XXX just return the package for now - Ok(bundle_data.pkg_contents.clone()) + Ok(deep_copy(&bundle_data.pkg_contents)) } fn read_key(&self, bundle_id: &str, key: &str) -> Result<&KeyValueData, SecurityRequestError> { let bundle = self.get_bundle(bundle_id)?; @@ -148,17 +211,11 @@ impl SecurityCoordinatorInterface for FakeSecurityCoordinator { fn test_mailbox(&mut self) -> Result<(), SecurityRequestError> { trace!("test_mailbox_command()"); - const PAGE_SIZE: usize = 1 << seL4_PageBits; const MESSAGE_SIZE_DWORDS: usize = 17; // Just a random message size for testing. extern "C" { fn mailbox_api_send(paddr: u32, size: u32); fn mailbox_api_receive(paddr: *mut u32, size: *mut u32); - static SELF_VSPACE_ROOT: seL4_CPtr; - - // This is not actually a block of memory, it's a reserved range in - // the virtual address space that we can map physical memory into. - static mut COPYREGION: [u32; 1024]; } // Allocate a 4k page to serve as our message buffer. @@ -168,31 +225,25 @@ impl SecurityCoordinatorInterface for FakeSecurityCoordinator { unsafe { // Map the message buffer into our copyregion so we can access it. - // FIXME(aappleby): We need a drop() impl here somewhere so this - // doesn't leak if something fails. - let message_ptr = core::ptr::addr_of_mut!(COPYREGION[0]); - seL4_Page_Map( - /*sel4_page=*/ frame_bundle.objs[0].cptr, - /*seL4_pd=*/ SELF_VSPACE_ROOT, - /*vaddr=*/ message_ptr as usize, - seL4_CapRights::new( - // NB: RW 'cuz W-only silently gets upgraded by kernel - /*grant_reply=*/ - 0, /*grant=*/ 0, /*read=1*/ 1, /*write=*/ 1, - ), - seL4_Default_VMAttributes, - ) - .map_err(|_| SecurityRequestError::SreTestFailed)?; + // NB: re-use one of the deep_copy copyregions. + let mut msg_region = CopyRegion::new( + ptr::addr_of_mut!(DEEP_COPY_SRC[0]), + PAGE_SIZE + ); + msg_region.map(frame_bundle.objs[0].cptr) + .map_err(|_| SecurityRequestError::SreTestFailed)?; + + let message_ptr = msg_region.as_word_mut(); // Write to the message buffer through the copyregion. - let offset_a = 0 as isize; - let offset_b = (MESSAGE_SIZE_DWORDS - 1) as isize; - message_ptr.offset(offset_a).write(0xDEADBEEF); - message_ptr.offset(offset_b).write(0xF00DCAFE); + let offset_a = 0; + let offset_b = MESSAGE_SIZE_DWORDS - 1; + message_ptr[offset_a] = 0xDEADBEEF; + message_ptr[offset_b] = 0xF00DCAFE; trace!( "test_mailbox: old buf contents 0x{:X} 0x{:X}", - message_ptr.offset(offset_a).read(), - message_ptr.offset(offset_b).read() + message_ptr[offset_a], + message_ptr[offset_b] ); // Send the _physical_ address of the message buffer to the security @@ -216,14 +267,14 @@ impl SecurityCoordinatorInterface for FakeSecurityCoordinator { trace!("test_mailbox: expected contents 0x12345678 0x87654321"); trace!( "test_mailbox: new buf contents 0x{:X} 0x{:X}", - message_ptr.offset(offset_a).read(), - message_ptr.offset(offset_b).read() + message_ptr[offset_a], + message_ptr[offset_b] ); - let dword_a = message_ptr.offset(offset_a).read(); - let dword_b = message_ptr.offset(offset_b).read(); + let dword_a = message_ptr[offset_a]; + let dword_b = message_ptr[offset_b]; - seL4_Page_Unmap(frame_bundle.objs[0].cptr) + msg_region.unmap() .map_err(|_| SecurityRequestError::SreTestFailed)?; // Done, free the message buffer. diff --git a/apps/system/system.camkes b/apps/system/system.camkes index f22b2ff..4f51d70 100644 --- a/apps/system/system.camkes +++ b/apps/system/system.camkes @@ -155,7 +155,7 @@ assembly { from debug_console.memory, from process_manager.memory, from security_coordinator.memory, - // TOOD(sleffler): from ml_coordinator.memory, + from ml_coordinator.memory, to memory_manager.memory); // Connect the SecurityCoordinatorInterface to each component that needs @@ -164,7 +164,7 @@ assembly { connection seL4RPCOverMultiSharedData multi_security( from debug_console.security, // NB: for debug/test from process_manager.security, - from ml_coordinator.security, // NB: for LoadModel but not in design + from ml_coordinator.security, // NB: for LoadModel from storage_manager.security, to security_coordinator.security);