Skip to content

Commit

Permalink
Move MappedPages::as_func() into LoadedSection::as_func() to impr…
Browse files Browse the repository at this point in the history 
…ove safety. (#419)

* When reinterpreting a region of memory as an executable function, this enables us to check the full size of the function rather than just the function signature type. 

* Only support reinterpreting `LoadedSection`s of type `Text` as functions, rather than any arbitrary memory region. 

* Refactor loadable mode code to use the new simpler `LoadedSection::as_func()` method.
  • Loading branch information
@kevinaboos
kevinaboos authored Aug 4, 2021
1 parent 168a4ef commit fbe80b0
Show file tree
Hide file tree
Showing 7 changed files with 76 additions and 128 deletions.
58 changes: 58 additions & 0 deletions kernel/crate_metadata/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -790,6 +790,64 @@ impl LoadedSection {
Err("this source section has a different length than the destination section")
}
}

/// Reinterprets this section's underlying `MappedPages` memory region as an executable function.
///
/// The generic `F` parameter is the function type signature itself, e.g., `fn(String) -> u8`.
///
/// Returns a reference to the function that is formed from the underlying memory region,
/// with a lifetime dependent upon the lifetime of this section.
///
/// # Locking
/// Obtains the lock on this section's `MappedPages` object.
///
/// # Note
/// Ideally, we would use debug information to know the size of the entire function
/// and test whether that fits within the bounds of the memory region, rather than just checking
/// the size of `F`, the function pointer/signature.
/// Without debug information, checking the size is restricted to in-bounds memory safety
/// rather than actual functional correctness.
///
/// # Examples
/// Here's how you might call this function:
/// ```
/// type MyPrintFuncSignature = fn(&str) -> Result<(), &'static str>;
/// let section = mod_mgmt::get_symbol_starting_with("my_crate::print::").upgrade().unwrap();
/// let print_func: &MyPrintFuncSignature = section.as_func().unwrap();
/// print_func("hello there");
/// ```
///
pub fn as_func<F>(&self) -> Result<&F, &'static str> {
if false {
debug!("Requested LoadedSection {:#X?} as function {:?}", self, core::any::type_name::<F>());
}

let mp = self.mapped_pages.lock();
// Check flags to make sure these pages are executable (otherwise a page fault would occur when this func is called)
if self.typ != SectionType::Text || !mp.flags().is_executable() {
error!("Requested LoadedSection as function {:?}, but was not an executable text section! (flags: {:?})",
core::any::type_name::<F>(), mp.flags()
);
return Err("as_func(): section was not an executable text section");
}

// Check that the bounds of this entire section fit within its MappedPages
let end = self.mapped_pages_offset + self.size();
if end > mp.size_in_bytes() {
error!("Requested LoadedSection as function {:?}, but section's end offset ({:X?}) was beyond its MappedPages ({:X?})",
core::any::type_name::<F>(), end, mp.size_in_bytes()
);
return Err("requested type and offset would not fit within the MappedPages bounds");
}

// SAFE: above, we check the section type, executability, and size bounds of its underlying MappedPages
// and tie the lifetime of the returned function reference to this section's lifetime.
Ok(unsafe {
core::mem::transmute(
&(mp.start_address().value() + self.mapped_pages_offset)
)
})
}
}

impl fmt::Debug for LoadedSection {
Expand Down
7 changes: 1 addition & 6 deletions kernel/crate_swap/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -513,12 +513,7 @@ pub fn swap_crates(
// as a backup, search fuzzily to accommodate state transfer function symbol names without full hashes
.or_else(|| namespace_of_new_crates.get_symbol_starting_with(&symbol).upgrade())
.ok_or("couldn't find specified state transfer function in the new CrateNamespace")?;

let mut space: usize = 0;
let st_fn = {
let mapped_pages = state_transfer_fn_sec.mapped_pages.lock();
mapped_pages.as_func::<StateTransferFunction>(state_transfer_fn_sec.mapped_pages_offset, &mut space)?
};
let st_fn = state_transfer_fn_sec.as_func::<StateTransferFunction>()?;
#[cfg(not(loscd_eval))]
debug!("swap_crates(): invoking the state transfer function {:?} with old_ns: {:?}, new_ns: {:?}", symbol, this_namespace.name(), namespace_of_new_crates.name());
st_fn(this_namespace, &namespace_of_new_crates)?;
Expand Down
77 changes: 0 additions & 77 deletions kernel/memory/src/paging/mapper.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -748,83 +748,6 @@ impl MappedPages {

Ok(slc)
}


/// Reinterprets this `MappedPages`'s underlying memory region as an executable function with any signature.
///
/// # Arguments
/// * `offset`: the offset (in number of bytes) into the memory region at which the function starts.
/// * `space`: a hack to satisfy the borrow checker's lifetime requirements.
///
/// Returns a reference to the function that is formed from the underlying memory region,
/// with a lifetime dependent upon the lifetime of the given `space` object.
///
/// TODO FIXME: this isn't really safe as it stands now.
/// Ideally, we need to have an integrated function that checks with the mod_mgmt crate
/// to see if the size of the function can fit (not just the size of the function POINTER, which will basically always fit)
/// within the bounds of this `MappedPages` object;
/// this integrated function would be based on the given string name of the function, like "task::this::foo",
/// and would invoke this as_func() function directly.
///
/// We have to accept space for the function pointer to exist, because it cannot live in this function's stack.
/// It has to live in stack of the function that invokes the actual returned function reference,
/// otherwise there would be a lifetime issue and a guaranteed page fault.
/// So, the `space` arg is a hack to ensure lifetimes;
/// we don't care about the actual value of `space`, as the value will be overwritten,
/// and it doesn't matter both before and after the call to this `as_func()`.
///
/// The generic `F` parameter is the function type signature itself, e.g., `fn(String) -> u8`.
///
/// # Examples
/// Here's how you might call this function:
/// ```
/// type PrintFuncSignature = fn(&str) -> Result<(), &'static str>;
/// let mut space = 0; // this must persist throughout the print_func being called
/// let print_func: &PrintFuncSignature = mapped_pages.as_func(func_offset, &mut space).unwrap();
/// print_func("hi");
/// ```
/// Because Rust has lexical lifetimes, the `space` variable must have a lifetime at least as long as the `print_func` variable,
/// meaning that `space` must still be in scope in order for `print_func` to be invoked.
///
#[doc(hidden)]
pub fn as_func<'a, F>(&self, offset: usize, space: &'a mut usize) -> Result<&'a F, &'static str> {
let size = mem::size_of::<F>();
if true {
#[cfg(not(downtime_eval))]
debug!("MappedPages::as_func(): requested {} with size {} at offset {}, MappedPages size {}!",
core::any::type_name::<F>(),
size, offset, self.size_in_bytes()
);
}

// check flags to make sure these pages are executable (otherwise a page fault would occur when this func is called)
if !self.flags.is_executable() {
error!("MappedPages::as_func(): requested {}, but MappedPages weren't executable (flags: {:?})",
core::any::type_name::<F>(),
self.flags
);
return Err("as_func(): MappedPages were not executable");
}

// check that size of the type F fits within the size of the mapping
let end = offset + size;
if end > self.size_in_bytes() {
error!("MappedPages::as_func(): requested type {} with size {} at offset {}, which is too large for MappedPages of size {}!",
core::any::type_name::<F>(),
size, offset, self.size_in_bytes()
);
return Err("requested type and offset would not fit within the MappedPages bounds");
}

*space = self.pages.start_address().value() + offset;

// SAFE: we guarantee the size and lifetime are within that of this MappedPages object
let t: &'a F = unsafe {
mem::transmute(space)
};

Ok(t)
}
}

impl Drop for MappedPages {
Expand Down
14 changes: 4 additions & 10 deletions kernel/nano_core/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,8 +191,7 @@ pub extern "C" fn nano_core_start(
}

// if in loadable mode, parse the crates we always need: the core library (Rust no_std lib), the panic handlers, and the captain
#[cfg(loadable)]
{
#[cfg(loadable)] {
use mod_mgmt::CrateNamespace;
println_raw!("nano_core_start(): loading the \"captain\" crate...");
let (captain_file, _ns) = try_exit!(CrateNamespace::get_crate_object_file_starting_with(default_namespace, "captain-").ok_or("couldn't find the singular \"captain\" crate object file"));
Expand All @@ -206,14 +205,12 @@ pub extern "C" fn nano_core_start(
// at this point, we load and jump directly to the Captain, which will take it from here.
// That's it, the nano_core is done! That's really all it does!
println_raw!("nano_core_start(): invoking the captain...");
#[cfg(not(loadable))]
{
#[cfg(not(loadable))] {
try_exit!(
captain::init(kernel_mmi_ref, identity_mapped_pages, stack, ap_realmode_begin, ap_realmode_end)
);
}
#[cfg(loadable)]
{
#[cfg(loadable)] {
use alloc::vec::Vec;
use memory::{MmiRef, MappedPages};

Expand All @@ -225,10 +222,7 @@ pub extern "C" fn nano_core_start(
info!("The nano_core (in loadable mode) is invoking the captain init function: {:?}", section.name);

type CaptainInitFunc = fn(MmiRef, Vec<MappedPages>, stack::Stack, VirtualAddress, VirtualAddress) -> Result<(), &'static str>;
let mut space = 0;
let func: &CaptainInitFunc = {
try_exit!(section.mapped_pages.lock().as_func(section.mapped_pages_offset, &mut space))
};
let func: &CaptainInitFunc = try_exit!(section.as_func());

try_exit!(
func(kernel_mmi_ref, identity_mapped_pages, stack, ap_realmode_begin, ap_realmode_end)
Expand Down
23 changes: 6 additions & 17 deletions kernel/panic_entry/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,10 @@ fn panic_entry_point(info: &PanicInfo) -> ! {
let kernel_mmi_ref = memory::get_kernel_mmi_ref();
let res = if kernel_mmi_ref.is_some() {
// proceed with calling the panic_wrapper, but don't shutdown with try_exit() if errors occur here
#[cfg(not(loadable))]
{
#[cfg(not(loadable))] {
panic_wrapper::panic_wrapper(info)
}
#[cfg(loadable)]
{
#[cfg(loadable)] {
// An internal function for calling the panic_wrapper, but returning errors along the way.
// We must make sure to not hold any locks when invoking the panic_wrapper function.
fn invoke_panic_wrapper(info: &PanicInfo) -> Result<(), &'static str> {
Expand All @@ -48,10 +46,7 @@ fn panic_entry_point(info: &PanicInfo) -> ! {
.and_then(|namespace| namespace.get_symbol_starting_with(PANIC_WRAPPER_SYMBOL).upgrade())
.ok_or("Couldn't get single symbol matching \"panic_wrapper::panic_wrapper::\"")?
};
let mut space = 0;
let func: &PanicWrapperFunc = {
section.mapped_pages.lock().as_func(section.mapped_pages_offset, &mut space)?
};
let func: &PanicWrapperFunc = section.as_func()?;
func(info)
}

Expand Down Expand Up @@ -99,12 +94,10 @@ extern "C" fn rust_eh_personality() -> ! {
/// but does so dynamically in loadable mode.
#[no_mangle]
extern "C" fn _Unwind_Resume(arg: usize) -> ! {
#[cfg(not(loadable))]
{
#[cfg(not(loadable))] {
unwind::unwind_resume(arg)
}
#[cfg(loadable)]
{
#[cfg(loadable)] {
// An internal function for calling the real unwind_resume function, but returning errors along the way.
// We must make sure to not hold any locks when invoking the function.
fn invoke_unwind_resume(arg: usize) -> Result<(), &'static str> {
Expand All @@ -115,11 +108,7 @@ extern "C" fn _Unwind_Resume(arg: usize) -> ! {
.and_then(|namespace| namespace.get_symbol_starting_with(UNWIND_RESUME_SYMBOL).upgrade())
.ok_or("Couldn't get single symbol matching \"unwind::unwind_resume::\"")?
};
let mut space = 0;
let func: &UnwindResumeFunc = {
section.mapped_pages.lock().as_func(section.mapped_pages_offset, &mut space)?
};

let func: &UnwindResumeFunc = section.as_func()?;
func(arg)
}
match invoke_unwind_resume(arg) {
Expand Down
18 changes: 6 additions & 12 deletions kernel/simd_personality/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,10 +159,8 @@ fn internal_setup_simd_personality(simd_ext: SimdExt) -> Result<(), &'static str
let section_ref1 = simd_app_namespace.get_symbol_starting_with("simd_test::test1::")
.upgrade()
.ok_or("no single symbol matching \"simd_test::test1\"")?;
let mut space1 = 0;
let (mapped_pages1, mapped_pages_offset1) = (section_ref1.mapped_pages.clone(), section_ref1.mapped_pages_offset);
let func1: &SimdTestFunc = mapped_pages1.lock().as_func(mapped_pages_offset1, &mut space1)?;
let _task1 = spawn::new_task_builder(func1, ())
let func1: &SimdTestFunc = section_ref1.as_func()?;
let _task1 = spawn::new_task_builder(*func1, ())
.name(format!("simd_test_1-{}", simd_app_namespace.name()))
.pin_on_core(this_core)
.simd(simd_ext)
Expand All @@ -173,10 +171,8 @@ fn internal_setup_simd_personality(simd_ext: SimdExt) -> Result<(), &'static str
let section_ref2 = simd_app_namespace.get_symbol_starting_with("simd_test::test2::")
.upgrade()
.ok_or("no single symbol matching \"simd_test::test2\"")?;
let mut space2 = 0;
let (mapped_pages2, mapped_pages_offset2) = (section_ref2.mapped_pages.clone(), section_ref2.mapped_pages_offset);
let func: &SimdTestFunc = mapped_pages2.lock().as_func(mapped_pages_offset2, &mut space2)?;
let _task2 = spawn::new_task_builder(func, ())
let func2: &SimdTestFunc = section_ref2.as_func()?;
let _task2 = spawn::new_task_builder(*func2, ())
.name(format!("simd_test_2-{}", simd_app_namespace.name()))
.pin_on_core(this_core)
.simd(simd_ext)
Expand All @@ -187,10 +183,8 @@ fn internal_setup_simd_personality(simd_ext: SimdExt) -> Result<(), &'static str
let section_ref3 = simd_app_namespace.get_symbol_starting_with("simd_test::test_short::")
.upgrade()
.ok_or("no single symbol matching \"simd_test::test_short\"")?;
let mut space3 = 0;
let (mapped_pages3, mapped_pages_offset3) = (section_ref3.mapped_pages.clone(), section_ref3.mapped_pages_offset);
let func: &SimdTestFunc = mapped_pages3.lock().as_func(mapped_pages_offset3, &mut space3)?;
let _task3 = spawn::new_task_builder(func, ())
let func3: &SimdTestFunc = section_ref3.as_func()?;
let _task3 = spawn::new_task_builder(*func3, ())
.name(format!("simd_test_short-{}", simd_app_namespace.name()))
.pin_on_core(this_core)
.simd(simd_ext)
Expand Down
7 changes: 1 addition & 6 deletions kernel/spawn/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,12 +163,7 @@ pub fn new_application_task_builder(
};
let main_func_sec = main_func_sec_opt.ok_or("spawn::new_application_task_builder(): couldn't find \"main\" function, expected function name like \"<crate_name>::main::<hash>\"\
--> Is this an app-level library or kernel crate? (Note: you cannot spawn a library crate with no main function)")?;

let mut space: usize = 0; // must live as long as main_func, see MappedPages::as_func()
let main_func = {
let mapped_pages = main_func_sec.mapped_pages.lock();
mapped_pages.as_func::<MainFunc>(main_func_sec.mapped_pages_offset, &mut space)?
};
let main_func = main_func_sec.as_func::<MainFunc>()?;

// Create the underlying task builder.
// Give it a default name based on the app crate's name, but that can be changed later.
Expand Down

0 comments on commit fbe80b0

Please sign in to comment.