How to use as_memref() safely?

[TheButlah]

The docs say:

The caller must ensure that the raw pointer is valid and points to a properly initialized TEE_Param.

But how can we be sure the raw pointer is valid? An adversarial CA could be mutating the buffer concurrently to the TA reading it, and as such maybe this buffer should instead be considered as a volatile pointer. For that reason, constructing either a &[u8] or a &mut [u8] doesn't seem sound unless we can somehow assume the CA is playing nice, which a TA should never assume. I would think that the only safe mechanism here is to copy the buffer into the TA's stack or heap, so maybe the API should be reworked to expose a volatile pointer with a safe function to copy it into a slice?

Also, what is to stop the CA from providing a bogus pointer into the TA's own memory, thereby basically getting the TA to read from its own memory space (instead of from CA controlled memory)?

Please advise.

[TheButlah]

See also, this issue, section 4.3.6.2 of the TEE Internal core api spec:

And 4.11.1: TEE_CheckMemoryAccessRights

I also see this in 4.11.1:

When a particular parameter passed in the structure TEE_Param to a TA entry point is a Memory
Reference as specified in its parameter type, then this block, as described by the initial values of the
fields buffer and size in that structure, SHALL allow read and/or write accesses as specified in
the parameter type. As noted above, this buffer is not required to reside in memory owned by the TA
instance.

And I see this in 3.4.3:

he [inbuf] annotation applies to a pair of parameters, the first of which is of pointer type, such as a void*,
and the second of which is of type size_t. It means that the parameters describe an input data buffer. The
entire buffer SHALL be readable by the Trusted Application and there is no restriction on the owner of the
buffer: It can reside in shared memory or in private memory.
The implementation SHALL check that the buffer is entirely readable and SHALL panic the calling Trusted
Application instance if that is not the case.
Because the NULL pointer is never accessible, a Trusted Application cannot pass NULL in the first (pointer)
parameter unless the second (size_t) parameter is set to 0.
The [inoutbuf] annotation is equivalent to [inbuf] except that it indicates read-and-write access to the
data buffer. The implementation SHALL check that the buffer is entirely readable and writable and SHALL
panic the calling Trusted Application instance if that is not the case.

[TheButlah]

Yeah, im pretty sure that ParamMemref::buffer() is essentially always unsound. It should return a "volatile slice" and not a &mut [u8].

In addition to that, I can't figure out if optee always guarantees the pointer passed in as a param would always point to someplace outside the TA. If it doesn't guarantee it, maybe defensively calling TEE_CheckMemoryAccessRights to determine if its not a shared buffer is a good idea.

[DemesneGH]

I would think that the only safe mechanism here is to copy the buffer into the TA's stack or heap, so maybe the API should be reworked to expose a volatile pointer with a safe function to copy it into a slice?

For the "copy into slice": Due to the memory limitations of TAs, always copying the memory doesn't make much sense. Maybe the user can choose to copy it the first time they get a slice? Or we can provide a wrapper function in the SDK to return a copied slice.

For the "volatile pointer": I agree with you. We could use ptr::read_volatile to access the memref to prevent the compiler from optimizing out repeated reads, ensuring that we always fetch the latest data from the shared memory.

In addition to that, I can't figure out if optee always guarantees the pointer passed in as a param would always point to someplace outside the TA.

AFAIK OP-TEE OS checks the memref during memory mapping in https://github.com/OP-TEE/optee_os/blob/master/core/mm/mobj_dyn_shm.c#L378. This is an important security primitive, and I believe OP-TEE OS should ensure this check.

[TheButlah]

Maybe the user can choose to copy it the first time they get a slice?

Getting it as a slice will always be unsound (at least if you assume a malicious CA, which we should always assume). The only way to work with slices is to copy first, then present a slice

Or we can provide a wrapper function in the SDK to return a copied slice.

I think we could use the volatile crate as the return value of ParamMemref::as_buffer() instead of a raw slice, and then the user gets to decide whether to copy or not.

Would a PR for this be accepted?

[TheButlah]

I also wonder if maybe it makes sense to properly track the mutablity of the paramref, based on whether its in/out/inout, and only expose the ability to write to it based on those values.

[DemesneGH]

I think we could use the volatile crate as the return value of ParamMemref::as_buffer() instead of a raw slice, and then the user gets to decide whether to copy or not.

Would a PR for this be accepted?

Yes we would appreciate it.

I also wonder if maybe it makes sense to properly track the mutablity of the paramref, based on whether its in/out/inout, and only expose the ability to write to it based on those values.

I agree with you.ParamMemref has the member ParamType which can be checked when trying to write into it.
If you're willing to help with this, please feel free to open a PR. Thanks!

[TheButlah]

I'm not sure anymore that using the volatile crate is the right choice. Most of the apis require unstable compiler intrinsics. Here's the general approach I think we should take instead. Let me know what you think.

The general approach is that we have a generic A (meaning "Access"), with traits and zst structs like this:

mod private {
    pub trait Sealed{}
}

pub mod access {
    pub struct Read;
    pub struct Write;
    pub struct ReadWrite;

    pub trait Access: private::Sealed {}
    pub trait Readable: Access {}
    pub trait Writable: Access {}

    impl private::Sealed for Read {}
    impl private::Sealed for Write {}
    impl private::Sealed for ReadWrite {}

    impl Access for Read {}
    impl Access for Write {}
    impl Access for ReadWrite {}

    impl Readable for Read {}
    impl Readable for ReadWrite {}

    impl Writeable for Write {}
    impl Writeable for ReadWrite {}
}

calling .as_memref() results in a ParamMemref<'parameter, ()> which exposes no reading or writing functionality, only basic c-style access like getting the raw pointer, the size, etc.

To be able to read to the buffer, we call .in(), .inout(), or .out(). Alternative naming if you prefer could be .access::<Read>(), .access::<ReadWrite>(), or .access::<Write>(). Either way, these cast the type to the correct access type, checking the ParamType and returning an error if the requested access doesn't match the param.

Then, we can call .buffer() which returns a VolatileBuf<'parameter, A> type, itself exposing functionality to read and write to the volatile buffer.

So here is roughly what the API would look like:

impl <'parameter, A> ParamMemref<'parameter, A> {
    pub fn in(self) -> Result<ParamMemref<'parameter, Read>, InvalidAccessErr> { /* ... */ }
    pub fn out(self) -> Result<ParamMemref<'parameter, Write>, InvalidAccessErr> { /* ... */ }
    pub fn inout(self) -> Result<ParamMemref<'parameter, ReadWrite>, InvalidAccessErr> { /* ... */ }

    pub fn raw(&mut self) -> *mut raw::Memref { /* ... */ }

    /// The size of the allocated memory region (i.e. the original value of `self.size`)
    pub fn capacity(&self) -> usize { /* ... */ }
}

impl <'parameter, A: access::Access> ParamMemref<'parameter, A> {
    pub fn buffer(&mut self) -> VolatileBuf<'parameter, A> {  /* ... */ }
}

impl <'parameter, A: access::Writeable> ParamMemref<'parameter, A> {
    /// Errors if the new size would be bigger than `self.capacity()`
    pub fn set_updated_size(&mut self, length: usize) -> Result<(), BiggerThanCapacityErr> { /* ... */ }
    
    /// By convention, setting the raw size to something larger than what was initially passed by OP-TEE, is a signal to
    /// the CA that there is insufficient memory allocated to output a result, and that the CA should invoke the function again
    /// with a larger buffer. See section `4.3.6.4` of the Global Platform TEE Internal Core API.
    /// Note that this does *not* update `self.capacity()`.
    /// Returns an error if the requested amount is not any bigger than the existing capacity.
    pub fn request_more_capacity(&mut self, new_capacity: usize) -> Result<(), NotBiggerThanCapacityErr>{ /* ... */  }
}

Finally the volatile types would be like this:

pub struct VolatileBuf<'parameter, A> {
    _phantom: PhantomData<&'parameter mut [u8]>,
    ptr: *mut u8,
    capacity: usize,
}

impl <'parameter, A: access::Readable> VolatileBuf<'parameter, A> {
    //// Returns `WouldOverflowErr` if the output buffer is smaller than `self.capacity()`.
    pub fn copy_to(&self, out: &mut [u8]) -> Result<(), WouldOverflowErr> { /* ... */ }

    //// Reads the value at the given index. Returns an error if `index >= self.capacity()`.
    pub fn read(&self, index: usize) -> Result<u8, OutOfBoundsErr> { /* ... */ }
}

impl <'parameter, A: access::Writeable> VolatileBuf<'parameter, A> {
    /// Copies all values from `values` into `self`. Returns `WouldOverflowErr` if the copy would exceed the capacity of `self`.
    pub fn copy_from(&mut self, values: &[u8]) -> Result<(), WouldOverflowErr> { /* ... */ }

    /// Writes to a location in the buffer. Returns an error if `index >= self.capacity()`.
    pub fn write(&mut self, index: usize) -> Result<(), OutOfBoundsErr> { /* ... */ }
}

Let me know what you think and if this is a good approach that would be accepted if I submit a PR.

[TheButlah]

I've submitted a PR that implements this. Let me know your feedback in the PR: #278