fix!: introduce access api and VolatileBuf to safely interact with volatile ParamMemref buffers

[TheButlah]

Fixes #273

Summarizing that issue, the current api 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 in volatile.rs, 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, and itself has no dependencies. 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 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. It should be noted that the volatile crate also utilizes a type-state access api.
• This is a breaking change. It breaks the example code too, and I wanted to hold off on fixing that to keep the diff small and let us discuss first, and align on the approach. Once I get a green light that this direction is desired, I'll fix the rest.

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 👍

[DemesneGH]

Thanks for your commit!
The approach looks good to me, and I've left two tiny comments.
Additionally, could you help with the following:

• fix the license ci;
• modify one example to demonstrate the API change

Thanks!

[TheButlah]

OK I've addressed the feedback so far, let me know what you think 👍
I updated the serde example

[DemesneGH]

Thanks for the changes! Since this is a core part of the API design, it's worth taking the extra time to consider it carefully. I really appreciate your patience and collaboration on this!

Some new suggestions:
I’m wondering if we can also eliminate the another unreachable!() block in updating size.
Since checking capacity, copying memory, and updating the size are almost always performed together, how about merging them into a single atomic method? This makes the example code much cleaner and user-friendly, also prevents developers from forgetting necessary checks or size updates.
Like this:

impl ParamMemref<'_, _> {
    /// Copies data into the buffer and automatically updates the memref size.
    /// Returns `ShortBufferErr` and updates the size hint if the capacity is insufficient.
    pub fn copy_from_and_update_size(&mut self, src: &[u8]) -> Result<(), Error> {
        let len = src.len();
        
        // 1. Check capacity
        self.ensure_capacity(len)?;

        // 2. Access the buffer and perform the copy
        let mut buf = self.buffer()?;
        buf.copy_from(src)?;

        // 3. Update the raw memref size, the len has been checked
        unsafe { self.raw.as_mut() }.size = len;

        Ok(())
    }
}

In the example:

let bytes = serialized.as_bytes();

// Atomically check capacity, copy data, and update the size
p.copy_from_and_update_size(bytes)?;

Let me know what you think, thanks!

[TheButlah]

My concern would be, what if someone doesn't want to update the capacity?

Im curious on your thoughts if this could be a potential solution. The copy_from(buf) function could return a struct, and it would have three functions on this struct (builder pattern). One would be .update_size(), one would be .ensure_capacity() and another would be .finish()

The only problem is, this doesn't really change the return types at all or prevent any ? but I suppose it saves having to pass the length of the buffer into the function?

I think we would still want to keep the non-builder .ensure_capacity() method too right?

Edit: Hmm on second thought, maybe we should just always ensure capacity and set updated size when doing copy_from?

[DemesneGH]

I believe the Rust SDK should provide primitives that are as safe as possible. In our context, checking capacity, copying data, and updating the size should be treated as a single atomic operation to maintain consistency. Decoupling them only creates more opportunities for logic bugs (like forgetting to update the size) and leads to redundant error handling.

what if someone doesn't want to update the capacity?

I cannot think of a valid use case for skipping the update... If the capacity is insufficient, why not updating the capacity to inform the client？

maybe we should just always ensure capacity and set updated size when doing copy_from?

It also makes sense to me. To keep the API explicit, we may need a more descriptive name than just copy_from to indicate it handles the full lifecycle (check, copy, and size update).

---

Also requesting feedback from @ivila for this API design (once he is available). Since we are heading into the Chinese New Year holiday, our responses would be slower than usual. Thanks for your patience!