Fix potential unsoundness in dynamic grant allocation #2052
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In the original code, dynamic grant allocation directly returns `Owned`. This is problematic, as if a capsule stores these Owned values outside of another app-specific grant, then it could keep it around after the app has crashed/restarted, and then access deallocated memory. This fixes that by instead returning a new `DynamicGrant` type, which requires an `enter` call to access. Signed-off-by: David Ross <David.Ross@wdc.com>
2 tasks
There was a problem hiding this comment.
One minor comment, overall I think this looks good.
I do think that eventually this will need to add some mechanism for freeing data (Owned implemented Drop, which called process.free() when it went out of scope). But given that we have yet to implement actually implement free() I don't think it makes sense to block on that.
Comment on lines
+62
to
+72
| pub fn enter<F, R>(&mut self, fun: F) -> Option<R> | ||
| where | ||
| F: FnOnce(Borrowed<'_, T>) -> R, | ||
| { | ||
| self.appid.kernel.process_map_or(None, self.appid, |_| { | ||
| let data = unsafe { self.data.as_mut() }; | ||
| let borrowed = Borrowed::new(data, self.appid); | ||
| Some(fun(borrowed)) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Do you think it makes more sense for this function to return something like a Result<R, ReturnCode>?
bors bot
added a commit
that referenced
this pull request
Sep 17, 2020
2053: Add utilities for allocating dynamic grants. r=bradjc a=daboross ### Pull Request Overview This pull request is intended to expand the basic functionality for dynamically allocating values. It: - changes the existing `alloc` method to accept non-`Default` values - adds a new method, `alloc_n_with`, for allocating a runtime-determined number of contiguous values - splits the internal `alloc_unowned` into `alloc_raw` and `alloc_default_unowned` to allow for allocating non-`Default` values I believe all of these changes will be useful, but the one which motivated this PR was `alloc_n_with`. To support connections in the Rubble BLE capsule, we'll need to support attributes, and the capsule won't know ahead of time how many attributes any given app will want to have in a connection. Attributes are most naturally stored in an array, and since the kernel won't know at compile time how many attributes an app has, we need dynamic allocation. Hence, `alloc_n_with`. Note that this PR leaves the usage of `Owned`, which I believe is unsound. Once we re-export `Owned` so that applications can store them, those applications could potentially store them outside of app-specific grant memory, and then access them after the app has been restarted (thus accessing deallocated memory). I have a fix for this, but since the changes are fairly separate, I've submitted it as a separate PR - #2052. If one PR is merged first, the other will need a rebase, but it shouldn't matter which one that is. CC @alevy ### Testing Strategy I created a temporary test capsule and app which calls `alloc` and `alloc_n_with` every other tick until we hit `OutOfMemory`, keeps each allocation one tick, and prints the data out after retrieving it. ### TODO or Help Wanted While this was manually tested, automated testing would be better. Do we have a place for unit tests for kernel functionality exposed to capsules? ### Documentation Updated - [x] Updated the relevant files in `/docs`, or no updates are required. ### Formatting - [x] Ran `make prepush`. Co-authored-by: David Ross <David.Ross@wdc.com>
ppannuto
added
the
release-blocker
|
@daboross, we discussed this on the call today and decided we would like to include this fix in the upcoming 1.6 release. It looks like there is currently a merge conflict, however. |
bradjc
removed
the
release-blocker
4 tasks
2 tasks
alistair23
added a commit
to alistair23/tock
that referenced
this pull request
Dec 3, 2020
This is a rebase of the original PR: tock#2052 In the original code, dynamic grant allocation directly returns `Owned`. This is problematic, as if a capsule stores these Owned values outside of another app-specific grant, then it could keep it around after the app has crashed/restarted, and then access deallocated memory. This fixes that by instead returning a new `DynamicGrant` type, which requires an `enter` call to access. Signed-off-by: David Ross <David.Ross@wdc.com> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
|
replaced by #2229 |
bors bot
added a commit
that referenced
this pull request
Jan 11, 2021
2229: kernel: Fix potential unsoundness in dynamic grant allocation r=alevy a=alistair23 ### Pull Request Overview This is a rebase of the original PR: #2052 In the original code, dynamic grant allocation directly returns `Owned`. This is problematic, as if a capsule stores these Owned values outside of another app-specific grant, then it could keep it around after the app has crashed/restarted, and then access deallocated memory. This fixes that by instead returning a new `DynamicGrant` type, which requires an `enter` call to access. Signed-off-by: David Ross <David.Ross@wdc.com> Signed-off-by: Alistair Francis <alistair.francis@wdc.com> ### Testing Strategy CI ### TODO or Help Wanted ### Documentation Updated - [X] Updated the relevant files in `/docs`, or no updates are required. ### Formatting - [X] Ran `make prepush`. Co-authored-by: Alistair Francis <alistair.francis@wdc.com>
bors bot
added a commit
that referenced
this pull request
Feb 15, 2021
2137: Simplify grant implementation and fix soundness errors r=alevy a=alevy ### Pull Request Overview This pull request addresses #2135 and partially the issue that #2052 addresses: - Grants do not currently prevent re-entrancy which, in turn, allows mutable aliasing of Rust memory (that happens to be located in grant regions) - The Allocator interface (which is currently totally unused except for @daboross's in progress BLE work) has similar, but harder to fix issues. This pull request does three things: 1. It removes the `Allocator` struct and replaces the closure `enter` expects to take a `()` instead. This works because the allocator is totally unused at the moment, so users of `enter` don't care about the type. We _should_ bring this back and #2052 is likely close to the solution, but as an initial step, removing it is good too. 2. It attempts to dramatically simplify the grant module, leaving only four types: - `Borrowed` - a wrapper around a mutable reference to reified Grant memory and an appid that's yielded when "entering" a grant - `AppliedGrant` which is type that contains a grant that's guaranteed to be allocated and usable - `Grant` the main type a user interacts with. `Grant`s differ from `AppliedGrant` in that calling `enter` may initialize the grant for a particular process if it has not yet been allocated. - `Iter` - a helper type for iterating over all processes for a particular grant. In addition, it moves all use of `unsafe` to the creation of `AppliedGrant`s and implements everything else in terms of creation and access to `AppliedGrant`s. 3. Finally, it fixes #2135 by applying `TakeCell`-like semantics to the Grant's `enter` API. Entering a grant leaves behind a placeholder `0xffffffff` value in place of the pointer that means it's currently being used. Re-entrance to a grant checks for this value and will fail if it is present. ### Testing Strategy So far tested on Imix and Hail with the `alarm` driver (which had reentrant code) and the UDP driver (which has reentrant code). Without fixing these capsules, both now fail, but can be fixed to work. ### TODO or Help Wanted * [ ] Document the grant module better * [ ] Find and fix other incorrectly written capsules ### Documentation Updated - [ ] Updated the relevant files in `/docs`, or no updates are required. ### Formatting - [x] Ran `make prepush`. Co-authored-by: Amit Aryeh Levy <amit@amitlevy.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request Overview
In the original code,
Allocator::allocreturnedOwned, a type which asserts that the memory it is pointing to is accessible.If a capsule stores the returned
Ownedoutside of an app-specificGrant, and then tries to access it after the app has crashed/restarted, it will access deallocated memory.To fix this, I've changed
Allocator::allocto return a new type,DynamicGrant, which requires anentercall, and then exposesBorrowed.This API exposes
Borrowedrather thanOwnedas I believe it matches the nature of thisenterfunction better. In particular, the value will be accessible multiple times, and the closure doesn't actually get ownership of it - only temporary access.CC @alevy
Testing Strategy
I applied changes from #2053 on top of these changes, and then ran the tests from PR. Mainly, creating a temporary test capsule and app which allocate, check the results, and then re-allocate every other tick until running out of grant memory.
TODO or Help Wanted
Documentation Updated
/docs, or no updates are required.Formatting
make prepush.