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.

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

Add CapbilityPtr and Add SuccessAddr and SuccessPtr syscall variants #4174

Merged
merged 47 commits into from
Nov 17, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
12abe15
MetaPtr + usize/u32 changes
Aug 17, 2024
c32507d
Have MetaPtr wrap a pointer not usize
Oct 14, 2024
677183f
Link to CHERI tracking issue
Oct 14, 2024
6254b9e
Add explicit return codes for usize/ptr
Oct 14, 2024
a5ee396
Better comment for MetaPtr
Oct 14, 2024
fd51098
Fix typo
Oct 14, 2024
c97deea
Apply comment suggestions
LawrenceEsswood Oct 21, 2024
291c8f4
Rename MetaPtr -> CapabilityPtr
Oct 29, 2024
695af1b
Add comment saying into_compat is for legacy only
Oct 29, 2024
7ec6bdd
capability_ptr: update doc to reflect meaning
alevy Nov 4, 2024
d0fa5fc
Update kernel/src/syscall.rs
LawrenceEsswood Nov 8, 2024
be62a66
Update kernel/src/process.rs
LawrenceEsswood Nov 8, 2024
950deb2
Revert notes change
Nov 8, 2024
236a8ac
Remove extra cast step
Nov 8, 2024
a4e5936
Preface internal Google bug tracker
Nov 8, 2024
9ac666b
Remove as_checked_ptr until CHERI lands
Nov 8, 2024
010bf22
Change panic for assert and checked alignment too
Nov 8, 2024
604460c
Make the oddly chosen ANY permission a NONE permission
Nov 8, 2024
79b2b8d
Add provenance notes
Nov 8, 2024
6feb692
Refine CapabilityPtr description slightly
Nov 8, 2024
f800ddd
Minor changes to encode
Nov 8, 2024
2cd33d2
Move capability_ptr to utilities
Nov 8, 2024
bf24e72
Update kernel/src/process.rs
LawrenceEsswood Nov 11, 2024
d569776
Rename into_compat
Nov 11, 2024
743c8cd
Document what authority the CapabilityPtr from brk/sbrk should have
Nov 11, 2024
144269e
Do not pollute scope with Execute
Nov 11, 2024
cc0b53e
Add comment to construction of intial fn
Nov 11, 2024
45f8a44
Remove mention of CHERI
Nov 11, 2024
391dfc8
Use .cast_mut(), not as
Nov 11, 2024
8f8ce44
Document all methods on CapabilityPtr
Nov 11, 2024
a5e5a6d
More formatting/comments/name change for permissions
Nov 11, 2024
f3feec6
Style changes
Nov 11, 2024
a001b33
kernel/syscall: split out encode_syscall_return, create TRD104 subset
lschuermann Nov 8, 2024
7ea663f
kernel: fix rustdoc links
lschuermann Nov 12, 2024
9560125
Merge remote-tracking branch 'upstream/master' into meta_ptr
alevy Nov 13, 2024
670a5d0
kernel: handle_syscall: elaborate on NonNull change for CapabilityPtr
lschuermann Nov 13, 2024
3810ca8
kernel/capability_ptr: make new_with_metadata an unsafe method
lschuermann Nov 13, 2024
0709e6a
kernel/arch_helpers: remove 32bit infix from encode_syscall_return_tr…
lschuermann Nov 13, 2024
1ebf509
kernel: remove CapabilityPtr aliases for now
alevy Nov 14, 2024
d23676d
kernel: remove stale reference in upcall doc
alevy Nov 14, 2024
38a5188
kernel: remove unused syscall return variant
alevy Nov 14, 2024
08caaa5
kernel: rename capability_ptr constructor
alevy Nov 14, 2024
c6156c8
kernel: documentation nits
alevy Nov 14, 2024
a803e1e
kernel: remove SuccessUsize in favor of SuccessPtr
alevy Nov 14, 2024
096536c
kernel: fix import order nit
alevy Nov 14, 2024
3c1876b
kernel: memop: add provenance to memop returns
alevy Nov 14, 2024
37cb959
Merge remote-tracking branch 'upstream/master' into meta_ptr
lschuermann Nov 14, 2024
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
13 changes: 9 additions & 4 deletions arch/cortex-m/src/syscall.rs
Original file line number Diff line number Diff line change
Expand Up @@ -232,9 +232,9 @@ impl<A: CortexMVariant> kernel::syscall::UserspaceKernelBoundary for SysCall<A>
// - Stack offset 4 is R12, which the syscall interface ignores
let stack_bottom = state.psp as *mut usize;
ptr::write(stack_bottom.offset(7), state.psr); //......... -> APSR
ptr::write(stack_bottom.offset(6), callback.pc | 1); //... -> PC
ptr::write(stack_bottom.offset(6), usize::from(callback.pc) | 1); //... -> PC
lschuermann marked this conversation as resolved.
Show resolved Hide resolved
ptr::write(stack_bottom.offset(5), state.yield_pc | 1); // -> LR
ptr::write(stack_bottom.offset(3), callback.argument3); // -> R3
ptr::write(stack_bottom.offset(3), callback.argument3.into()); // -> R3
ptr::write(stack_bottom.offset(2), callback.argument2); // -> R2
ptr::write(stack_bottom.offset(1), callback.argument1); // -> R1
ptr::write(stack_bottom.offset(0), callback.argument0); // -> R0
Expand Down Expand Up @@ -300,8 +300,13 @@ impl<A: CortexMVariant> kernel::syscall::UserspaceKernelBoundary for SysCall<A>

// Use the helper function to convert these raw values into a Tock
// `Syscall` type.
let syscall =
kernel::syscall::Syscall::from_register_arguments(svc_num, r0, r1, r2, r3);
let syscall = kernel::syscall::Syscall::from_register_arguments(
svc_num,
r0,
r1.into(),
r2.into(),
r3.into(),
);
lschuermann marked this conversation as resolved.
Show resolved Hide resolved

match syscall {
Some(s) => kernel::syscall::ContextSwitchReason::SyscallFired { syscall: s },
Expand Down
10 changes: 5 additions & 5 deletions arch/rv32i/src/syscall.rs
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ impl kernel::syscall::UserspaceKernelBoundary for SysCall {
state.regs[R_A0] = callback.argument0 as u32;
state.regs[R_A1] = callback.argument1 as u32;
state.regs[R_A2] = callback.argument2 as u32;
state.regs[R_A3] = callback.argument3 as u32;
state.regs[R_A3] = callback.argument3.as_ptr() as usize as u32;

// We also need to set the return address (ra) register so that the new
// function that the process is running returns to the correct location.
Expand All @@ -206,7 +206,7 @@ impl kernel::syscall::UserspaceKernelBoundary for SysCall {
state.regs[R_RA] = state.pc;

// Save the PC we expect to execute.
state.pc = callback.pc as u32;
state.pc = callback.pc.as_ptr() as usize as u32;

Ok(())
}
Expand Down Expand Up @@ -631,9 +631,9 @@ impl kernel::syscall::UserspaceKernelBoundary for SysCall {
let syscall = kernel::syscall::Syscall::from_register_arguments(
state.regs[R_A4] as u8,
state.regs[R_A0] as usize,
state.regs[R_A1] as usize,
state.regs[R_A2] as usize,
state.regs[R_A3] as usize,
(state.regs[R_A1] as usize).into(),
(state.regs[R_A2] as usize).into(),
(state.regs[R_A3] as usize).into(),
);

match syscall {
Expand Down
4 changes: 2 additions & 2 deletions kernel/src/grant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -707,8 +707,8 @@ impl<'a> GrantKernelData<'a> {
#[repr(C)]
#[derive(Default)]
struct SavedUpcall {
appdata: usize,
fn_ptr: Option<NonNull<()>>,
appdata: crate::upcall::AppdataType,
fn_ptr: crate::upcall::FnPtrType,
lschuermann marked this conversation as resolved.
Show resolved Hide resolved
}

/// A minimal representation of a read-only allow from app, used for storing a
Expand Down
13 changes: 7 additions & 6 deletions kernel/src/kernel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
//! etc.) is defined in the `scheduler` subcrate and selected by a board.

use core::cell::Cell;
use core::ptr::NonNull;

use crate::capabilities;
use crate::config;
Expand Down Expand Up @@ -877,15 +876,17 @@ impl Kernel {
subscribe_num: subdriver_number,
};

// TODO: when the compiler supports capability types bring this back
// as a NonNull type.
LawrenceEsswood marked this conversation as resolved.
Show resolved Hide resolved
// First check if `upcall_ptr` is null. A null
// `upcall_ptr` will result in `None` here and
// represents the special "unsubscribe" operation.
let ptr = NonNull::new(upcall_ptr);
// let ptr = NonNull::new(upcall_ptr);

// For convenience create an `Upcall` type now. This is
// just a data structure and doesn't do any checking or
// conversion.
let upcall = Upcall::new(process.processid(), upcall_id, appdata, ptr);
let upcall = Upcall::new(process.processid(), upcall_id, appdata, upcall_ptr);

// If `ptr` is not null, we must first verify that the
// upcall function pointer is within process accessible
Expand All @@ -895,8 +896,8 @@ impl Kernel {
// > process executable memory...), the kernel...MUST
// > immediately return a failure with a error code of
// > `INVALID`.
let rval1 = ptr.and_then(|upcall_ptr_nonnull| {
if !process.is_valid_upcall_function_pointer(upcall_ptr_nonnull) {
let rval1 = upcall_ptr.map_or(None, |upcall_ptr_nonnull| {
if !process.is_valid_upcall_function_pointer(upcall_ptr_nonnull.as_ptr()) {
Some(ErrorCode::INVAL)
} else {
None
Expand Down Expand Up @@ -1010,7 +1011,7 @@ impl Kernel {
process.processid(),
driver_number,
subdriver_number,
upcall_ptr as usize,
upcall_ptr,
appdata,
rval
);
Expand Down
1 change: 1 addition & 0 deletions kernel/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ pub mod grant;
pub mod hil;
pub mod introspection;
pub mod ipc;
pub mod metaptr;
pub mod platform;
pub mod process;
pub mod process_checker;
Expand Down
22 changes: 11 additions & 11 deletions kernel/src/memop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,48 +52,48 @@ pub(crate) fn memop(process: &dyn Process, op_type: usize, r1: usize) -> Syscall
// Op Type 1: SBRK
alevy marked this conversation as resolved.
Show resolved Hide resolved
1 => process
.sbrk(r1 as isize)
.map(|addr| SyscallReturn::SuccessU32(addr as u32))
.map(|addr| SyscallReturn::SuccessPtr(addr))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...although into_compat will reverse this again.

.unwrap_or(SyscallReturn::Failure(ErrorCode::NOMEM)),

// Op Type 2: Process memory start
2 => SyscallReturn::SuccessU32(process.get_addresses().sram_start as u32),
2 => SyscallReturn::SuccessUSize(process.get_addresses().sram_start),

// Op Type 3: Process memory end
3 => SyscallReturn::SuccessU32(process.get_addresses().sram_end as u32),
3 => SyscallReturn::SuccessUSize(process.get_addresses().sram_end),

// Op Type 4: Process flash start
4 => SyscallReturn::SuccessU32(process.get_addresses().flash_start as u32),
4 => SyscallReturn::SuccessUSize(process.get_addresses().flash_start),

// Op Type 5: Process flash end
5 => SyscallReturn::SuccessU32(process.get_addresses().flash_end as u32),
5 => SyscallReturn::SuccessUSize(process.get_addresses().flash_end),

// Op Type 6: Grant region begin
6 => SyscallReturn::SuccessU32(process.get_addresses().sram_grant_start as u32),
6 => SyscallReturn::SuccessUSize(process.get_addresses().sram_grant_start),

// Op Type 7: Number of defined writeable regions in the TBF header.
7 => SyscallReturn::SuccessU32(process.number_writeable_flash_regions() as u32),
7 => SyscallReturn::SuccessUSize(process.number_writeable_flash_regions()),

// Op Type 8: The start address of the writeable region indexed by r1.
8 => {
let flash_start = process.get_addresses().flash_start as u32;
let flash_start = process.get_addresses().flash_start;
let (offset, size) = process.get_writeable_flash_region(r1);
if size == 0 {
SyscallReturn::Failure(ErrorCode::FAIL)
} else {
SyscallReturn::SuccessU32(flash_start + offset)
SyscallReturn::SuccessUSize(flash_start + offset)
}
}

// Op Type 9: The end address of the writeable region indexed by r1.
// Returns (void*) -1 on failure, meaning the selected writeable region
// does not exist.
9 => {
let flash_start = process.get_addresses().flash_start as u32;
let flash_start = process.get_addresses().flash_start;
let (offset, size) = process.get_writeable_flash_region(r1);
if size == 0 {
SyscallReturn::Failure(ErrorCode::FAIL)
} else {
SyscallReturn::SuccessU32(flash_start + offset + size)
SyscallReturn::SuccessUSize(flash_start + offset + size)
}
}

Expand Down
113 changes: 113 additions & 0 deletions kernel/src/metaptr.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
// Licensed under the Apache License, Version 2.0 or the MIT License.
// SPDX-License-Identifier: Apache-2.0 OR MIT
// Copyright Google LLC 2024.

//! Defines the MetaPtr type

use core::fmt::{Formatter, LowerHex, UpperHex};
use core::ops::AddAssign;

/// A pointer with target specific metadata.
/// This should be used any time the kernel wishes to grant authority to the user, or any time
/// the user should be required to prove validity of a pointer.
alevy marked this conversation as resolved.
Show resolved Hide resolved
#[derive(Default, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)]
#[repr(transparent)]
LawrenceEsswood marked this conversation as resolved.
Show resolved Hide resolved
pub struct MetaPtr {
ptr: usize,
}

#[derive(Copy, Clone, PartialEq)]
pub enum MetaPermissions {
Any,
Read,
Write,
ReadWrite,
Execute,
}

impl From<MetaPtr> for usize {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is OK because a MetaPtr is always convertible into a raw pointer by just losing any meta information. However, might be better to implement this for <T> *const T (which can always be cast to a usize anyway)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add that too (it might even exist as a method?), but this one gets used a lot in syscall / return argument handling.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah got it. Does this imply that those arguments or returns should actually be *const T?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its happening a lot because values that are usize, or even eventually u32, are being passed in registers that can hold something as wide as MetaPtr (the widest type, or so I have assumed).

@bradjc has suggested that maybe those should be a separate RegisterData type, something large enough to hold u32/isize/*const()/MetaPtr.

I suppose, fairly, one could imagine a platform with pointers that are smaller than u32, in which case really RegisterData would wrap around the largest type.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documenting that CapabilityPtr is a pointer then using it to store other things is very confusing, and I think having a separate RegisterData [1] type would help that confusion. CapabilityPtr is already a confusing enough type to understand, especially for contributors who are unfamiliar with CHERI.

The fact that some CHERI systems use different registers for integers and capabilities however, does throw a wrench into the works. However, there are two obvious ways that come to mind for how we might choose to handle that at the ABI level:

  1. We can pass integers in the integer registers and capabilities in the capability registers. In this case, the kernel knows which are which so we can can use usize for the integer registers and CapabilityPtr for the capability registers [2].
  2. We can pass everything in the capability registers and ignore the integer registers. This is just treating the system as a CHERI system that uses the same registers for everything, and we'd just use RegisterData for all the register values.

In both cases, it makes sense to only use CapabilityPtr for pointer types, so I don't think it's really a blocker for having both CapabilityPtr and RegisterData. Option 1 would be somewhat awkward, though, because we would only use RegisterData for the capability registers.

[1] libtock-rs just calls this Register
[2] This ignores the possibility of non-pointer capabilities, which is another complication

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not convinced RegisterData is really a single type. Machines have different width registers. To say that RegisterData should refer to just one of them is odd to me. If we did introduce a RegisterData here, I would expect it to wrap CapabilityPtr (with an appropriate comment saying the type is being used because it is expected to be widest). It would certainly throw a spanner if ever a different ABI were introduced that actually used different width registers, to have a RegisterData and then SomeOtherRegisterData. But I am happy to add RegisterData if that is the consensus and makes the syscall layer easier to understand.

  1. is/was the ABI for those split file machines. In a similar fashion to how you would select float/integer registers on platforms that split them.

#[inline]
fn from(from: MetaPtr) -> Self {
from.ptr
}
}

impl From<usize> for MetaPtr {
jrvanwhy marked this conversation as resolved.
Show resolved Hide resolved
#[inline]
fn from(from: usize) -> Self {
Self { ptr: from }
}
}

impl UpperHex for MetaPtr {
#[inline]
fn fmt(&self, f: &mut Formatter<'_>) -> core::fmt::Result {
UpperHex::fmt(&self.ptr, f)
}
}

impl LowerHex for MetaPtr {
#[inline]
fn fmt(&self, f: &mut Formatter<'_>) -> core::fmt::Result {
LowerHex::fmt(&self.ptr, f)
}
}

impl AddAssign<usize> for MetaPtr {
alevy marked this conversation as resolved.
Show resolved Hide resolved
#[inline]
fn add_assign(&mut self, rhs: usize) {
self.ptr.add_assign(rhs)
}
}

impl MetaPtr {
pub fn as_ptr(&self) -> *const () {
LawrenceEsswood marked this conversation as resolved.
Show resolved Hide resolved
self.ptr as *const ()
}

/// Convert to a raw pointer, checking that metadata allows a particular set of permissions over
/// a given number of bytes.
/// If the metadata does not allow for this, returns null.
/// If no such metadata exists, this succeeds.
#[inline]
pub fn as_ptr_checked(&self, _length: usize, _perms: MetaPermissions) -> *const () {
self.ptr as *const ()
}

#[inline]
pub fn new_with_metadata(
ptr: *const (),
_base: usize,
_length: usize,
_perms: MetaPermissions,
) -> Self {
Self { ptr: ptr as usize }
}

#[inline]
pub fn map_or<U, F>(&self, default: U, f: F) -> U
where
F: FnOnce(&Self) -> U,
{
if self.ptr == 0usize {
default
} else {
f(self)
}
}

#[inline]
pub fn map_or_else<U, D, F>(&self, default: D, f: F) -> U
where
D: FnOnce() -> U,
F: FnOnce(&Self) -> U,
{
let addr: usize = (*self).into();

if addr == 0 {
default()
} else {
f(self)
}
}
}
17 changes: 9 additions & 8 deletions kernel/src/process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use crate::capabilities;
use crate::errorcode::ErrorCode;
use crate::ipc;
use crate::kernel::Kernel;
use crate::metaptr::MetaPtr;
use crate::platform::mpu::{self};
use crate::processbuffer::{ReadOnlyProcessBuffer, ReadWriteProcessBuffer};
use crate::storage_permissions;
Expand Down Expand Up @@ -539,7 +540,7 @@ pub trait Process {
/// process's memory region.
/// - [`Error::KernelError`] if there was an internal kernel error. This is
/// a bug.
fn brk(&self, new_break: *const u8) -> Result<*const u8, Error>;
fn brk(&self, new_break: *const u8) -> Result<MetaPtr, Error>;

/// Change the location of the program break by `increment` bytes,
/// reallocate the MPU region covering program memory, and return the
Expand All @@ -559,7 +560,7 @@ pub trait Process {
/// process's memory region.
/// - [`Error::KernelError`] if there was an internal kernel error. This is
/// a bug.
fn sbrk(&self, increment: isize) -> Result<*const u8, Error>;
fn sbrk(&self, increment: isize) -> Result<MetaPtr, Error>;

/// How many writeable flash regions defined in the TBF header for this
/// process.
Expand All @@ -574,10 +575,10 @@ pub trait Process {
///
/// ## Returns
///
/// A tuple containing the a `u32` of the offset from the beginning of the
/// process's flash region where the writeable region starts and a `u32` of
/// A tuple containing the a `usize` of the offset from the beginning of the
/// process's flash region where the writeable region starts and a `usize` of
/// the size of the region in bytes.
fn get_writeable_flash_region(&self, region_index: usize) -> (u32, u32);
fn get_writeable_flash_region(&self, region_index: usize) -> (usize, usize);

/// Debug function to update the kernel on where the stack starts for this
/// process. Processes are not required to call this through the memop
Expand Down Expand Up @@ -790,7 +791,7 @@ pub trait Process {
///
/// Returns `true` if the upcall function pointer is valid for this process,
/// and `false` otherwise.
fn is_valid_upcall_function_pointer(&self, upcall_fn: NonNull<()>) -> bool;
LawrenceEsswood marked this conversation as resolved.
Show resolved Hide resolved
fn is_valid_upcall_function_pointer(&self, upcall_fn: *const ()) -> bool;

// functions for processes that are architecture specific

Expand Down Expand Up @@ -1078,9 +1079,9 @@ pub struct FunctionCall {
/// The third argument to the function.
pub argument2: usize,
/// The fourth argument to the function.
LawrenceEsswood marked this conversation as resolved.
Show resolved Hide resolved
pub argument3: usize,
pub argument3: MetaPtr,
/// The PC of the function to execute.
pub pc: usize,
pub pc: MetaPtr,
}

/// This is similar to `FunctionCall` but for the special case of the Null
Expand Down
Loading
Loading