[RFC] new TEE parameter type: memref_secure (SWG-186)

[etienne-lms]

this is a proposal for implementing the ability for a TA to safely handle memory buffer arguments related to secure buffers:

• linux side refers to buffer using dma_buf file handles and "memref_secure" type id.
• tee/optee linux driver gets/puts buffer ressources before/after invoking TEE.

Related to jira SWG-186.

This PR is related to changes in the optee_client PR57 and optee_os PR1052

[jenswi-linaro]

I don't like the way secure shm buffers are treated differently from normal shm buffers. I'd prefer if we could hide all special treatment of a secure shm buffer behind the already present shm functions. As it is now it's a kind of a hybrid approach that I fear will not age well. For instance, I see no reason why you need to call the special function put_secure_memref() instead of tee_shm_put().

[etienne-lms]

ok. understand and agree. i have put the part of the "secure shm" code in the core tee and optee. i will move all in tee_shm.c.

One thing bothers me: in struct tee_shm flag SHM_SECURE gets rid of SHM_DMA_BUF and SHM_MAPPED flags values. I think we need a dedicated nonsecure/secure flag for secure bufffers, but SHM_DMA_BUF could cover both nonsecure and secure memref cases.

[jenswi-linaro]

Agree, SHM_DMA_BUF should cover both nonsecure and secure memrefs.

[jenswi-linaro]

If needed we can add another ioctl to convert a dma-buf from SMAF into an TEE SHM object (with correct SHM_SECURE flag set etc).

[etienne-lms]

If needed we can add another ioctl to convert a dma-buf from SMAF into an TEE SHM object (with correct SHM_SECURE flag set etc).

You mean a new TEEC_RegisterSecureMemory() for client to register the buffer and have kernel allocating a tee_shm structure it could later use to find back the reference using an "idr" id ?

In my current code, kernel creates a brand new tee_shm struct right before invoking TEE and destroys it once back from TEE. i though it was more simple but it requires client to filled an "implementation defined" field of the structure TEEC_SharedMemory to provide the dmabuf handle. I hijacked the shm_id that used to store the shm idr ID.

With such a TEEC_RegisterSecureMemory(), client will have to call TEEC_ReleaseSecureMemory() (or TEEC_ReleaseSharedMemory()`) to insure kernel releases the resource. Such an API would also add 2 userland/kernel traversals when invoking its TA.

[jenswi-linaro]

My primary concern is the TEE driver and the API (linux/include/uapi/linux/tee.h). If SMAF could register the dma-buf as a shared memory object and pass that ID to user space we wouldn't need to change tee.h more than define another flag for secure shared memory objects. We would probably not need to touch the code in the invoke path at all.

[etienne-lms]

When allocating memory through SMAF, the client gets a file handle: a dmabuf file handle. Client can use this file handle to

• release the buffer: close handle
• mmap the buffer: in case it has no security constrain
• invoke an optee TA with the handle as reference (+ an offset/size info).

SMAF can operate without any TEE. I don't think it's nice to change the SMAF memory so that it uses specific TEE structure instead of the generic dma_buf framework.

[etienne-lms]

Could the struct tee_shm be changed to store a dmabuf id in the id field, instead of an idr of the target dmabuf structure address, registered from teedrv ?

When tee driver gets a shm structure reference, the TEEC_SHM_DMA_BUF would indicate that id stores a dmabuf id. TEE drvier can converts the dmabuf id into a pointer to the struct dma_buf of the allocated buffer and get back the struct tee_shm address for the struct dma_buf through a lightweight container_of().

This would change current tee driver.

[jenswi-linaro]

What's a dmabuf id? A file descriptor? If so I'd rather not. That was the old way of doing it, it was changed after some harsh comments from Al Viro.

[etienne-lms]

about fordidding another client to steal the shm of an application ? as smaf is done, the buffer could have been allocated by some other user or kernel thread.

On 16 September 2016 at 11:25, Jens Wiklander notifications@github.com wrote:

What's a dmabuf id? A file descriptor? If so I'd rather not. That was the old way of doing it, it was changed after some harsh comments from Al Viro.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/linaro-swg/linux/pull/23#issuecomment-247557021, or mute the thread https://github.com/notifications/unsubscribe-auth/ATEHJhgus78UQHqLztSBXD4yet9N7_cOks5qqmCmgaJpZM4J8kSK .

[jenswi-linaro]

Here's a link to the mail. https://marc.info/?l=linux-arm-kernel&m=145525285408168&w=2 Another problem with file descriptors when used as identifier is that they are only unique per process.

[etienne-lms]

ok. i will talk to benjamin about that, in his smaf framework.

In the meantime, we need an API for client to convert a dmabuf file handle into a regsitered "secure" TEEC_SharedMemory structre. A brand new API: TEEC_RegisterMemoryReference() with a "DMA_BUF_HANDLE" flag as input arg to specific the input reference is a dma_buf file handle ?

[etienne-lms]

@jenswi-linaro, the mail you refer to points out an issue in getting a file descriptor, while the memref_secure code i proposed simply uses an already handle file descriptor. Is there really a potential issue here ?

actually i did not cache why getting/putting file descriptor the Al Viro warned about.

[jenswi-linaro]

Once you've fully allocated a file descriptor the only practical way to free it is via the syscall close from user space. The problem is if you fail passing the descriptor to user space for some reason you're leaking file descriptors.

I can see some drawbacks with having file descriptors in structs that you pass around between different functions inside the kernel, but as you may understand I didn't want to ask Al to point out exactly where the problem is. Especially as it wasn't hard to replace the file descriptors.

By passing a dma-buf file descriptor instead of an shm identifier you introduce special cases to deal with. Are the secure buffers really only expected to be used during one invoke and then destroyed?

[etienne-lms]

I have rebased the changes to optee driver v12 branch. It's the same proposal (i did not fix anything from current review comments).

@jenswi-linaro

Are the secure buffers really only expected to be used during one invoke and then destroyed ?

It is likely that the buffers are allocated somewhere, eventually used by some component to inject data, then used by TEE, then passed to another component, and once useless freed or reused. From TEE point of view, the buffer must be at least available while TEE is invoked. What happends before or after is out of TEE scope.

The tee driver could only use the dmabuf file descriptor to find the dma_buf structure, retrieve the buffer physical location and then release the file descriptor. This way tee driver "gets" dmabuf reference resources but it will not "get" from kernel land the "dmabuf file descriptor" resources.

[etienne-lms]

I have discussed with benjamin. I believe the issue pointed by Al Viro was to call to "put_fd" in original tee_shm.c code. Kernel land cannot put a file descriptor. But using a file descriptor to get the target dmabuf structure seems to right way to use DMA. I always remains to the userland to close the handle once done with it.

[jenswi-linaro]

Regardless of this there's still the issue with the impact on the interface. It's much cleaner to treat all shared memory objects the same.

I understand that there's extra overhead by converting a dma-buf into a shared memory object. How big problem is it? Can the cost be amortized? How expensive is it compared to securing a buffer through secure world (which I assume has to be done if a secure buffer isn't reused)?

[etienne-lms]

OK, I will leave SMAF as it is: at buffer allocation, client get a dmabuf file descriptor. i'll add a new libteec API TEEC_RegisterMemoryFileDescriptor() to get a consistent shm object out of a SMAF buffer reference.

[etienne-lms]

discard this PR. Current plans is that nonsecure world will NOT use a specific 'parameter type identifier' for secure memory buffer. Instead, a new TEE Client API will allow userland to register a memory buffer file descriptor and get a referenced shared memory object that can be used to invoke a TA.