[update] Manifest Based Authorization feature update. #1828
Conversation
mhatrevi
requested review from
jhand2,
fdamato,
rusty1968,
bluegate010,
swenson and
vsonims
as code owners
|
Should we also add the changes to the README.md doc? |
This PR makes the following changes to the feature: 1. Adds a 'firmware id' to the image metadata entry (IME) structure. 2. Reduces max IME limit from 128 to 127 to account for size increase due to the firmware id addition. 3. SET_AUTH_MANIFEST command validates that the IME collection does not contain duplicate firmware ids. 4. SET_AUTH_MANIFEST command sorts the IME on the firmware ids. 5. Adds a new bit to the IME flags named 'SKIP AUTHORIZATION'. If set, AUTHORIZE_AND_STASH command authorizes an IME with a matching firmware id ignoring the image hash. 6. If a firmware id is not found, AUTHORIZE_AND_STASH command returns IMAGE_NOT_AUTHORIZED status code in the command output. 7. If a firmware id is found but the image hash does not match, AUTHORIZE_AND_STASH returns IMAGE_HASH_MISMATCH status code in the command output. 8. If a firmware id is found and the image hash matches or if the 'SKIP AUTHORIZATION' flag is set for the the firmware id, AUTHORIZE_AND_STASH command returns IMAGE_AUTHORIZED status code in the command output. 9. AUTHORIZE_AND_STASH command uses binary search on the firmware id to lookup an IME.
mhatrevi
force-pushed
the
vmhatre/auth-man-update
branch
from
cc7a92d
to
d025425
Compare
There was a problem hiding this comment.
Overall implementation looks good to me. I think the runtime readme will need some updates to reflect these changes.
You might also add a couple more test cases for the sorting and binary search. They look simple and correct to me. But I know corner cases can bite you on that. I'll still approve without it, just something to consider.
| let mut left = 0; | ||
| let mut right = auth_manifest_image_metadata_col.entry_count as usize; | ||
|
|
||
| while left < right { |
There was a problem hiding this comment.
Can we use auth_manifest_image_metadata_col.image_metadata_list.binary_search_by_key()?
Either way, we should add unit tests for this function.
| @@ -401,6 +415,34 @@ impl SetAuthManifestCmd { | |||
| soc_ifc, | |||
| )?; | |||
|
|
|||
| // Sort the image metadata list by firmware ID in place. Also check for duplicate firmware IDs. | |||
| let slice = | |||
| &mut metadata_mailbox.image_metadata_list[..metadata_mailbox.entry_count as usize]; | |||
There was a problem hiding this comment.
Why can't we use metadata_mailbox.image_metadata_list.sort_unstable_by_key()?
If we need to check for duplicates, we can do it after sorting.
This PR makes the following changes to the feature: