Fixes and improvements to secgate macros and upcalls #170
Conversation
dbittman
force-pushed
the
dbittman-secgate-cut1
branch
from
f6c3022 to
aab885c
Compare
dbittman
requested review from
PandaZ3D and
gvnn3
and removed request for
gvnn3
| @@ -44,18 +50,17 @@ impl<Backing: BackingData> Compartment<Backing> { | |||
| let alloc_layout = tls_info | |||
| .allocation_layout::<T>() | |||
| .map_err(DynlinkErrorKind::from)?; | |||
| let base = alloc(alloc_layout).ok_or_else(|| DynlinkErrorKind::FailedToAllocate { | |||
There was a problem hiding this comment.
I know the RFC mentions this, but can we add a comment saying that the the idea here is that each compartment has its own copy of std so using alloc directly here is ok?
| object::{MAX_SIZE, NULLPAGE_SIZE}, | ||
| syscall::{sys_object_create, BackingType, LifetimeType, ObjectCreate, ObjectCreateFlags}, | ||
| }; | ||
| use twizzler_object::ObjID; |
There was a problem hiding this comment.
Where should ObjID come from? twizzler_object, twizzler_abi, or twizzler_runtime_api?
There was a problem hiding this comment.
I'm going to merge these types in a future PR, I'll open an issue
| let temp = Box::new(TlsTemplateInfo::from(template_info)); | ||
| let temp = Box::leak(temp); | ||
| let cc = comp | ||
| .monitor_new(SharedCompConfig::new(sctx_id, temp)) |
There was a problem hiding this comment.
How do the addresses in the TLS template info make sense for SharedCompConfig? Wouldn't they be different for the monitor and the compartment since the same alloc obj would be mapped to different slots?
There was a problem hiding this comment.
So, the box::new here is wrong, but the addresses in the tls template get rebased over to the compartment when it loads the template into a new tls region for a new thread. See TlsTemplateInfo::init_new_tls_region.
I have fixed the Box::new issue in a future upcoming patch, since it doesn't matter yet in this PR (none of it is secure anyway)
| @@ -29,10 +29,6 @@ pub trait BackingData: Clone { | |||
| /// region is valid. | |||
| fn data(&self) -> (*mut u8, usize); | |||
|
|
|||
| /// Make a new backing data for holding allocated data for the dynamic linker. | |||
| fn new_data() -> Result<Self, DynlinkError> | |||
There was a problem hiding this comment.
What was the reason for removing new data? Is this related to the use of alloc directly?
There was a problem hiding this comment.
Yeah, different design for dealing with allocating objects within a compartment.
This PR fixes a number of issues with upcalls and sec gates.