Conversation
sangho2
left a comment
There was a problem hiding this comment.
Hi Angelina, Thank you for your PR!
I've requested a minor change to maintain the core behavior of load_ldelf.
| if ta_bin.is_none() { | ||
| return Err(loader::elf::ElfLoaderError::InvalidUuid); | ||
| } |
There was a problem hiding this comment.
ta_bin == None will trigger dynamic TA loading (once it is ready). So, None is an expected value.
There was a problem hiding this comment.
Thank you for the review!
There is an issue with loading the OP-TEE module if I remove this.
As part of optee_probe(), there is a call to optee_enumerate_devices() which opens a session with device enumeration PTA. On the LiteBox side, this is handled as a normal TA.
Currently, Some(TA_BINARY) is passed to load_ldelf() as the ta_bin: Option<&[u8]> argument and the function returns Err(loader::elf::ElfLoaderError::InvalidUuid) at this point:
if let Some(ta_bin) = ta_bin
&& !entrypoints.task.global.store_ta_bin(&ta_uuid, ta_bin)
{
return Err(loader::elf::ElfLoaderError::InvalidUuid);
}
Then, returning to VTL0 and skipping the rest of the device enumeration code.
With my changes, None is passed to load_ldelf(), instead of Some(TA_BINARY), so the rest of load_ldelf() runs and returns Ok() and eventually there is a panic when ldelf tries to load the TA:
ldelf/TA_CreateEntryPoint failed: return_code=ItemNotFound
panicked at litebox_runner_lvbs/src/lib.rs:1209:5:
How would you suggest handling this?
There was a problem hiding this comment.
I think we shouldn't call load_ldelf in that case. load_ldelf is for loading a TA ELF binary through ldelf. PTA is not an ELF binary. I think it might be better to call find_ta_binary before load_ldelf and return InvalidUuid if it fails to find a corresponding TA.
Signed-off-by: Angelina Vu <angelinavu@microsoft.com>
|
🤖 SemverChecks 🤖 No breaking API changes detected Note: this does not mean API is unchanged, or even that there are no breaking changes; simply, none of the detections triggered. |
This PR adds support for having multiple built-in TAs.