Type definitions in tee_client_api.h violate the GlobalPlatform TEE Client API spec

[tchebb]

Background

The GlobalPlatform TEE Client API Specification v1.0, which OP-TEE's libteec adheres to, defines API objects as structures visible to and allocated by the user rather than as opaque pointers.

While some of those objects—like TEEC_UUID—are fully specified by the spec, others—specifically TEEC_Context, TEEC_Session, TEEC_SharedMemory, and TEEC_Operation—end with an imp field of a type defined by the implementation. For example, TEEC_SharedMemory is specified as follows:

typedef struct
{
	void* buffer;
	size_t size;
	uint32_t flags;
	<Implementation-Defined Type> imp;
} TEEC_SharedMemory;

And this is what the spec has to say about the imp field (emphasis mine):

Implementations are allowed to extend some of the data structures defined in this specification to include a single field of implementation-defined type, named imp. Implementations MUST NOT add new fields outside of imp.

The problem

But here's how we currently define our version of TEEC_SharedMemory:

optee_client/public/tee_client_api.h

Lines 291 to 306 in 8533e0e

	typedef struct {
	void *buffer;
	size_t size;
	uint32_t flags;
	/*
	* Implementation-Defined
	*/
	int id;
	size_t alloced_size;
	void *shadow_buffer;
	int registered_fd;
	union {
	bool dummy;
	uint8_t flags;
	} internal;
	} TEEC_SharedMemory;

We don't have a field named imp at all! We instead put multiple implementation-specific fields right in the struct, in direct violation of the spec. I imagine we did this because it seemed an immaterial distinction, but I've found a case where it makes a difference for me as a user of libteec:

Why it matters

I'm currently working on making Apache's Teaclave TrustZone SDK, a Rust abstraction over libteec, work with more implementations than just OP-TEE. Part of that work is making its FFI structure definitions, which currently hardcode OP-TEE's implementation-specific fields, generic over multiple implementations. The obvious way to do that is to use Rust generics, adding a type parameter that defines the concrete type of the imp field. But that requires me to split the OP-TEE implementation-specific fields into their own structure, resulting in this concrete type:

typedef struct
{
	void* buffer;
	size_t size;
	uint32_t flags;
	struct {
	 	int id;
 	 	size_t alloced_size;
  		void *shadow_buffer;
 	 	int registered_fd;
 	 	union {
 	 		bool dummy;
 	 		uint8_t flags;
 	 	} internal;
	} imp;
} TEEC_SharedMemory;

Unfortunately, in C, that type does not have the same layout as the one from tee_client_api.h shown above. The nested struct changes how padding is calculated: on a 64-bit system, the type shown above has 4 bytes of padding between flags and id that libteec doesn't expect. (Note that I haven't verified the exact layouts empirically, but I'm pretty sure I'm right.)

I can work around this, but it means making each implementation-specific struct implement a common trait full of boilerplate getters and setters instead of just being able to access the fields directly from generic code. I'd prefer not to do that if I can avoid it.

Other implementations

There's at least one TEE that does follow my reading of the spec: Trustonic defines TEEC_SharedMemory as follows:

typedef struct TEEC_SharedMemory {
	void *buffer;
	size_t size;
	uint32_t flags;
	TEEC_SharedMemory_IMP imp;
} TEEC_SharedMemory;

[jenswi-linaro]

When we implemented this we read " imp;" in TEEC_SharedMemory as "here follow implementation specific fields". Feel free to propose a fix with a pull request to this git.

[tchebb]

Cool, thanks for confirming there's not some intentional reason for how we do it that I missed.

I probably won't have time to set up an OP-TEE dev environment and make/test a PR any time soon, sadly. I'm already on a bit of a tangent with the Teaclave modifications I'm making, and last night I discovered that the other implementation I'm trying to target actually has the same bug, which means I'd still need to workaround it in Rust even if OP-TEE gets fixed.

I'll leave this issue open as documentation and in case anyone else wants to take it on.

[jenswi-linaro]

Fixed by #349