Storage Permissions: Implement to match TRD #4031
Conversation
|
Thinking about this more, we should wait until after TockWorld (specifically the tutorial). |
There can be many implementations.
We need a way to assign storage permissions to processes.
Variants: - individual - SelfOnly - null
Processes are assigned storage permissions based on this policy when loaded.
Need to wrap in a struct so that only the constructors can create the permissions.
Needs a wrapper struct to enforce capabilities in the constructor.
|
Ok I've rebased this and updated capabilities for the constructor. This is ready to go again. |
kernel/src/process_standard.rs
Outdated
| /// | ||
| /// The `storage_permissions_policy` is optional because any app without a | ||
| /// fixed `ShortId` cannot have storage permissions, so in cases where an | ||
| /// AppID assigner is not used it doesn't make sense to have a storage | ||
| /// permissions policy. |
There was a problem hiding this comment.
Don't we have the null-policy for that?
There was a problem hiding this comment.
That is an option. However, we would still need a &'static ProcessStandardStoragePermissionsPolicy which is, I think, a fair bit of confusing overhead for something that doesn't do anything.
There was a problem hiding this comment.
Rust allows us to create &'static references to zero-sized types out of thin air: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=2b0c391057f50adfb07c62608ff1ef0f (actually, not just ZST but any constant, they get promoted to a static implicitly)
Alternatively, as it's slightly weird to return references from a constructor, we can also make the constructor const and then store the result in a static (which, as it's zero-sized, will not have any runtime overhead): https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=9c1f4334f81afdbd6a43d39055de7f70
In particular, this doesn't seem too bad of an option as the null policy doesn't require a capability to construct it.
I don't like that we're using an Option here, as that's one additional branch point for something that we already have semantically encapsulated with the null policy.
There was a problem hiding this comment.
Ok! Implementing the null permissions for () worked and the Option<Policy> is gone.
kernel/src/process_standard.rs
Outdated
| if let Some(storage_perm_policy) = storage_permissions_policy { | ||
| process.storage_permissions = storage_perm_policy.get_permissions(process); | ||
| } else { | ||
| process.storage_permissions = StoragePermissions::new_null(); | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't know whether I like this pattern where we're assigning storage permissions in the Process "constructor", and passing the partially constructed process reference into this method here. I'm wondering whether it's not better to assign the storage_permissions field after process construction, and calling this method in the loader instead?
In my opinion, given the storage permissions policy is generic and receives a &ProcessStandard reference, it should be able to expect that reference to be fully constructed.
There was a problem hiding this comment.
The problem is that ProcessStandard::create() has this signature:
pub(crate) unsafe fn create<'a>(
kernel: &'static Kernel,
chip: &'static C,
pb: ProcessBinary,
remaining_memory: &'a mut [u8],
fault_policy: &'static dyn ProcessFaultPolicy,
storage_permissions_policy: Option<&'static dyn ProcessStandardStoragePermissionsPolicy<C>>,
app_id: ShortId,
index: usize,
) -> Result<(Option<&'static dyn Process>, &'a mut [u8]), (ProcessLoadError, &'a mut [u8])>It returns &Process so the caller cannot set .storage_permissions.
There was a problem hiding this comment.
I'm not in love with how this is structured in general, but this approach does balance tock use cases with rust limitations in a perhaps reasonable way.
There was a problem hiding this comment.
Yes, agreed. I really don't like how the process constructor works here, but this is sound reasoning.
Tangentially, do you know why we're not returning an owned ProcessStandard here that we then move into a static container and take a &'static dyn Process reference to? As stated above, it's kind of weird for a constructor to return a reference.
There was a problem hiding this comment.
Rust is hard and we're just doing our best to make the kernel compile.
|
(I'd appreciate if others could take a look though, as I'm no expert on anything AppID) |
This allows boards that do not use storage permissions to default to null storage permissions.
|
I updated the TRD to hopefully fix all of the impl changes between the original document and this implementation. |
|
Any other comments? |
Pull Request Overview
This pull request adds an implementation to match the storage permissions TRD.
The core is the
StoragePermissionsenum:Policies then create a storage permissions variant based on how they want to assign applications storage permissions.
To merge the general storage permissions framework with the storage permissions TBF header, the write_id field is just used as a boolean: if the field is 0 the app has no write permissions, otherwise it does.
I also added some components to help with instantiating storage permissions policies.
Testing Strategy
This pull request was tested by...
TODO or Help Wanted
Need to figure out how this works with the HOTP tutorial. Non credentialed apps don't have storage permissions anymore.
Documentation Updated
/docs, or no updates are required.Formatting
make prepush.