-
Notifications
You must be signed in to change notification settings - Fork 90
Add upcall for delegation
#556
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| //! Upcall for platforms | ||
| //! | ||
| //! This module defines types and trait to provide upcalls for platforms. | ||
| //! A platform may receive some messages or requests from the host, devices, | ||
| //! or remote parties that it does not know how to handle. Rather than | ||
| //! making the platform be aware of all these messages or requests itself, | ||
| //! we implement upcalls to let platforms delegate handling of unknown | ||
| //! messages or requests to other layers of LiteBox (i.e., runner or shim). | ||
| //! Examples of such messages or requests include HVCI/Heki requests from | ||
| //! VTL0 and OP-TEE SMC calls from the normal world. | ||
| //! | ||
| //! # Security considerations | ||
| //! | ||
| //! Unlike other upcalls like hyperupcalls from the hypervisor to a guest VM, | ||
| //! this upcall is handled within LiteBox's TCB (i.e., by either runner or shim). | ||
| //! Therefore, the security considerations for this upcall are similar to function | ||
| //! calls within LiteBox. However, care must be taken to ensure that the upcall's | ||
| //! parameters and return values are properly validated and sanitized to prevent | ||
| //! potential security vulnerabilities. This is because the parameters might be | ||
|
Comment on lines
+17
to
+19
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "care must be taken" -> by whom?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the upcall handler. will clarify this. |
||
| //! provided by untrusted sources and return values might contain sensitive | ||
| //! information. We assume that the upcall providers must implement necessary | ||
| //! security checks and validations. The platform would simply pass the parameters | ||
| //! and return values between untrusted sources and upcall providers (because it | ||
| //! does not have semantics to validate them). We can specify a function for early | ||
| //! validation at the platform side if needed but its advantages are not clear at | ||
| //! this moment (since there is no costly context switch within LiteBox). | ||
|
Comment on lines
+24
to
+26
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: this sentence is surprising to see as part of the doc string. Might be better just as a comment, rather than being in the doc string. |
||
|
|
||
| use thiserror::Error; | ||
|
|
||
| /// An interface for upcalls from the platform to other layers of LiteBox. | ||
| pub trait Upcall { | ||
| /// The upcall parameter type to be passed from the platform. | ||
| type Parameter; | ||
|
|
||
| /// The upcall return type to be returned to the platform. | ||
| type Return; | ||
|
|
||
| /// Initialize the upcall handler. Must be called by the platform exactly once. | ||
| /// Per-thread initialization is possible but all threads must share the same | ||
| /// upcall handler. | ||
|
Comment on lines
+38
to
+40
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment is confusing. What happens if the platform does not invoke this? (I might have missed something but I didn't see the platform invoke this here). I also don't understand the "per-thread initialization is possible but ..." comment here. Should each thread invoke this? Or should different threads within the platform coordinate to make sure they have not invoked this more than once? A slightly easier-to-use interface is likely something like "get singleton", so that the platform doesn't need to store/manage this, and the runner side can choose to make it more/less performant as it needs. Alternatively, one can just eliminate initialization and set up the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Basically, this is a callback registration. I agree that the comment is confusing. I'll fix this. |
||
| fn init( | ||
| &self, | ||
| ) -> alloc::boxed::Box< | ||
| dyn crate::upcall::Upcall<Parameter = Self::Parameter, Return = Self::Return>, | ||
| >; | ||
|
Comment on lines
+41
to
+45
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You need
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the runner has a function and registers it to the platform through this ( |
||
|
|
||
| /// Execute the upcall with the given parameter. Since we do not expect that the | ||
| /// platform validates the parameters, the implementation of `execute` must validate | ||
| /// parameters to avoid potential security vulnerabilities. Also, it must sanitize | ||
| /// the return values before returning them to the platform. | ||
|
Comment on lines
+47
to
+50
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: might be good to point at |
||
| fn execute(&self, ctx: &mut Self::Parameter) -> Result<Self::Return, UpcallError>; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm mildly surprised by seeing the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. Technically, we don't need to struggle with lifetime. let me use values. |
||
| } | ||
|
|
||
| /// The error type for upcalls | ||
| #[non_exhaustive] | ||
| #[derive(Error, Debug)] | ||
| pub enum UpcallError { | ||
| #[error("Upcall failed")] | ||
| Failure, | ||
|
Comment on lines
+58
to
+59
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Failed due to? It becomes impossible in the current design for the upcall to give more useful information to the place below. Would be good to make this generic in some way (can do it generically, or can do a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me do that. |
||
| #[error("Upcall needs to be retried")] | ||
| Retry, | ||
|
Comment on lines
+60
to
+61
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the upcall needs to be retried, it needs to mention when it needs to be retried, right? Otherwise, if "retry immediately" is allowed, might as well do the retrying internally. The failure case is that "it cannot be handled right now, please try later", but yeah, something else feels necessary here in the output so that a platform can know when to retry.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. Technically, there is no easy way to know when the platform can try it again. Let me think. |
||
| #[error("Upcall parameter is invalid")] | ||
| InvalidParameter, | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -389,6 +389,19 @@ impl<Host: HostInterface> LinuxKernel<Host> { | |
| ) { | ||
| syscall_entry::init(shim); | ||
| } | ||
|
|
||
| /// Register the upcall. | ||
| pub fn register_upcall( | ||
| upcall: &'static ( | ||
| dyn litebox::upcall::Upcall< | ||
| Parameter = litebox_common_linux::PtRegs, | ||
| Return = litebox_common_linux::PtRegs, | ||
| > + Send | ||
| + Sync | ||
| ), | ||
|
Comment on lines
+395
to
+401
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oof, this looks like rustfmt did a horrifyingly bad job here. Consider putting the type into a type alias ( |
||
| ) { | ||
| UPCALL.call_once(|| upcall); | ||
| } | ||
| } | ||
|
|
||
| impl<Host: HostInterface> RawMutexProvider for LinuxKernel<Host> { | ||
|
|
@@ -755,6 +768,17 @@ impl<Host: HostInterface> StdioProvider for LinuxKernel<Host> { | |
| } | ||
| } | ||
|
|
||
| // Use static for upcall (we can use kernel TLS if needed) and `PtRegs` for now. | ||
| static UPCALL: spin::Once< | ||
| &'static ( | ||
| dyn litebox::upcall::Upcall< | ||
| Parameter = litebox_common_linux::PtRegs, | ||
| Return = litebox_common_linux::PtRegs, | ||
| > + Send | ||
| + Sync | ||
| ), | ||
| > = spin::Once::new(); | ||
|
|
||
| // NOTE: The below code is a naive workaround to let LVBS code to access the platform. | ||
| // Rather than doing this, we should implement LVBS interface/provider for the platform. | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,12 @@ pub(crate) fn optee_smc_dispatch(optee_smc_args_pfn: u64) -> i64 { | |
| with_per_cpu_variables_mut(|per_cpu_variables| { | ||
| per_cpu_variables.save_extended_states(HV_VTL_SECURE); | ||
| }); | ||
| // TODO: Implement OP-TEE SMC for TA command invocation here. | ||
|
|
||
| // Placeholder for now | ||
| let upcall = crate::UPCALL.get().expect("OP-TEE upcall not registered"); | ||
| let mut ctx = litebox_common_linux::PtRegs::default(); | ||
| let _ = upcall.execute(&mut ctx); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you intend to ignore the return the value here? Seems like this should be properly handled/propagated.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As wrote in the comment, this is just a placeholder. we should handle the return value.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor nit: keeping a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Didn't put TODO here because the actual implementation already exists in another PR. |
||
|
|
||
| debug_serial_println!("VSM function call for OP-TEE message"); | ||
| with_per_cpu_variables_mut(|per_cpu_variables| { | ||
| per_cpu_variables.restore_extended_states(HV_VTL_SECURE); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,7 @@ | ||
| #![no_std] | ||
|
|
||
| extern crate alloc; | ||
|
|
||
| use core::panic::PanicInfo; | ||
| use litebox_platform_lvbs::{ | ||
| arch::{gdt, get_core_id, instrs::hlt_loop, interrupts}, | ||
|
|
@@ -91,6 +93,7 @@ pub fn init() -> Option<&'static Platform> { | |
| interrupts::init_idt(); | ||
| x86_64::instructions::interrupts::enable(); | ||
| Platform::register_shim(&litebox_shim_optee::OpteeShim); | ||
| Platform::register_upcall(&crate::OpteeMsgHandler); | ||
|
|
||
| ret | ||
| } | ||
|
|
@@ -99,6 +102,30 @@ pub fn run(platform: Option<&'static Platform>) -> ! { | |
| vtl_switch_loop_entry(platform) | ||
| } | ||
|
|
||
| pub struct OpteeMsgHandler; | ||
| impl litebox::upcall::Upcall for OpteeMsgHandler { | ||
| type Parameter = litebox_common_linux::PtRegs; | ||
| type Return = litebox_common_linux::PtRegs; | ||
|
Comment on lines
+107
to
+108
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious to know why you need the registers here? Do you intend to run a thread during upcall.execute?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is just for defining parameter and return types. We don't specify any particular data types here yet. The reason I use |
||
|
|
||
| fn init( | ||
| &self, | ||
| ) -> alloc::boxed::Box< | ||
| dyn litebox::upcall::Upcall< | ||
| Parameter = litebox_common_linux::PtRegs, | ||
| Return = litebox_common_linux::PtRegs, | ||
| >, | ||
| > { | ||
| alloc::boxed::Box::new(OpteeMsgHandler {}) | ||
| } | ||
|
|
||
| fn execute( | ||
| &self, | ||
| _ctx: &mut Self::Parameter, | ||
| ) -> Result<Self::Return, litebox::upcall::UpcallError> { | ||
| todo!("invoke OP-TEE message handlers") | ||
| } | ||
| } | ||
|
|
||
| #[panic_handler] | ||
| fn panic(info: &PanicInfo) -> ! { | ||
| serial_println!("{}", info); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would signals from the outside world be a similar situation (i.e., should they be handled through this mechanism)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unlike the Linux shim case that all signals will be handled by the shim, certain exceptions/interrupts will be handled by the LVBS platform whereas some other ones are handled by the OP-TEE shim.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine right? The platform can just decide not to forward along things to the shim. There is nothing in the design that says that the platform must forward everything over, right? Maybe I am misunderstanding why the signals world and the upcall world are drastically different. Might be easier to chat in-person?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think the current explanation is misleading. It is more about there are something the platform doesn't know how to handle and delegates them to the others if applicable.