TheButlah opened a new pull request, #278: URL: https://github.com/apache/teaclave-trustzone-sdk/pull/278
Fixes https://github.com/apache/teaclave-trustzone-sdk/issues/273 **Summarizing that issue**, the [current api](https://github.com/apache/teaclave-trustzone-sdk/blob/e6511571ef29dc7bd2dd971098027cdcfea86ded/optee-utee/src/parameter.rs#L79) to access a `ParamMemref`'s buffer is unsound when having to account for malicious CAs (and probably benign ones too). The main issue is that since this memory is not synchronized, Linux can change its contents at any time, and therefore it is essentially volatile memory. Constructing a reference to volatile memory is always unsound. Additionally, OP-TEE *only* allows reads when the memref is an `[in]` or `[inout]` param, and writes when its an `[out]` or `[inout]` param. The original API doesn't check the `ParamType` at all, which means that the end user can easily write to a buffer they can only read from, or read from a buffer they can only write to. **This PR fixes the issue by introducing two concepts**: * An "access" api in `access.rs`. This api has traits and zero sized types that allow us to encode the access permissions into the type system via a typestate pattern. This sort of approach is a zero cost abstraction and allows us to check the param_type *only once*, as opposed to every interaction with the memref. It also helps ensure that only the appropriate functions (such as .read() or .write()) are expose for a given access type. The typestate approach is also extremely common in the bare-metal rust ecosystem (see embedded-hal crates), so I figured it isn't overly unfamiliar for this domain of work. * A `VolatileBuf`, which is a type that ensures that all access to its memory is done through `core::ptr::read_volatile` or `core::ptr::write_volatile`, and leverages the access api to expose only the appropriate read/write functionality. *A few things reviewers should be aware of:* * I added a dependency on `bytemuck::Pod`. This was done because I wanted to support *arbitrary* buffers of `T` rather than just `u8`. This was done in an attempt to allow faster accessing of data without needing to repeatedly call deserialization code, as if it were only u8 some sort of deserialization (or subsequent bytemuck cast) would be required. The downside is the additional dependency, but bytemuck is a 1.0 crate and well known and commonly used. I didn't want to introduce a first-party Pod trait of our own, because it would have less compatibility with the broader ecosystem and would require us to then implement bytemuck-style derive macros for user-defined types. * I *didn't* use the [volatile crate](https://docs.rs/volatile) because so much of the crate's surface area requires nightly compiler features. Additionally, I found the API limiting in some ways, and too complicated to easily use in others. I am definitely open to workshopping this submission, please let me know your thoughts and I will adjust the PR accordingly. Thanks for your time :+1: -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
