-
Notifications
You must be signed in to change notification settings - Fork 64
New Design for TA-to-TA Invocation #178
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
base: main
Are you sure you want to change the base?
Conversation
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.
The abstraction looks clear and well-structured. I've added a few minor comments for consideration. Great work overall!
optee-utee/src/tee_parameter.rs
Outdated
fn update_size_from_raw(&mut self, raw_param: &raw::TEE_Param) { | ||
match &mut self.content { | ||
ParamContent::MemrefOutput { buffer: _, written } => { | ||
*written = unsafe { raw_param.memref.size }; |
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.
I am curious why we consider accessing raw::TEE_Param
is unsafe
here. Can you add comment about the reason to the code? Or can we provide getter functions for raw::TEE_Param
and discuss the safe/unsafe behavior inside the raw
module.
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.
Accessing union
should be unsafe in Rust:
error[E0133]: access to union field is unsafe and requires unsafe function or block
--> /teaclave/optee-utee/src/tee_parameter.rs:152:28
|
152 | *written = raw_param.memref.size;
| ^^^^^^^^^^^^^^^^ access to union field
|
= note: the field may not be properly initialized: using uninitialized data will cause undefined behavior
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.
I suggest we follow https://github.com/rust-lang/rust-clippy/issues/9330
to explicitly add comment to unsafe
block; and run cargo lint
to help automatically enforce available rules.
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.
Will add SAFETY
comment for the modules inside this PR. Since cargo clippy
infects many other modules, there will be another PR to address all rules.
optee-utee/src/tee_parameter.rs
Outdated
ParamType::ValueInout => { | ||
param.update_value_from_raw(raw_param); | ||
} | ||
_ => {} |
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.
Should we check against other param types? We can at least ensure
ValueInput
contents are never changed; if not, raise an error;- out size for MemRef Out/InOut types always within the original bound; if not, raise an error;
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.
ValueInput
contents are never changed; if not, raise an error;
For ValueInput, we will not read the TEEParam buffer after invoke, which means if it was changed we will ignore it. It is the same strategy as MemrefInput. Should we add the check for those input?
For 2.
, agree and will update
optee-utee/src/tee_parameter.rs
Outdated
fn update_size_from_raw(&mut self, raw_param: &raw::TEE_Param) { | ||
match &mut self.content { | ||
ParamContent::MemrefOutput { buffer: _, written } => { | ||
*written = unsafe { raw_param.memref.size }; |
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.
I suggest we follow https://github.com/rust-lang/rust-clippy/issues/9330
to explicitly add comment to unsafe
block; and run cargo lint
to help automatically enforce available rules.
This commit provides the Rust API for GP API TEE_InvokeTACommand(), which enable the user TA calls another user TA or pseudo TA. - Add abstraction of TeeParameters and TaSession; - Add inter-ta example and test scripts. Signed-off-by: Yuan Zhuang <[email protected]>
Updated for all above comments |
|
Compared to #177, the Params and TaSession are re-organized and can be used in a more Rust style.
It also addressed some comments from the previous PR #177.