driver: tee: optee: implement OCALLs

[HernanGatta]

Overview

This PR modifies the TEE and OP-TEE drivers to support OCALLs.

The main change introduced by this PR is the addition of the TEE_IOC_ECALL IOCTL. This IOCTL mirrors and expands the functionality provided by TEE_IOC_INVOKE to support duplex communication. Where the latter provides a one-way command-and-reply transport from CA to TA, the former allows for two-way command-and-reply from CA to TA, and from TA to CA.

Additionally, a shared memory flag, TEE_IOCTL_SHM_OCALL, is added. When this flag is specified, shared memory is registered with the kernel, but not with OP-TEE. While the TEE Supplicant already can perform this operation by talking to a different device node with a different call table, introducing this flag enables all CAs to do it as well.

Lastly, the driver can now check for an additional OP-TEE capability, TEE_OPTEE_CAP_OCALL, which indicates that OP-TEE was built with and supports OCALLs.

Related PRs: OP-TEE, OP-TEE Client, OP-TEE Examples.

ECALL IOCTL

The fundamental working principle behind in-thread OCALL handling is the ability for the IOCTL that carries the initial function invocation request to return before the function invocation is complete. That is, given a CA that invokes a function on a TA, if the TA requests an OCALL, the IOCTL through which the original function invocation request was passed must return prematurely and indicate to the CA that it is not the original function invocation that completed. Rather, the latter is still pending, but has been suspended to relay the OCALL request to the CA. Once the CA handles the OCALL request, it invokes the IOCTL again, whose contents contain the result of the OCALL.

Since OCALL requests may require passing memory references, and given that TA memory must not be exposed to the CA, a shared memory allocation may be required ahead of the actual OCALL. When this occurs, the original IOCTL returns once to relay the shared memory allocation request to the CA, whereupon the CA calls the IOCTL again with the result of the allocation. Once the OCALL proper is complete, another IOCTL return and invocation cycle is required to free the previously allocated memory.

One may regard this duplex communication as a communication protocol. Each return of the IOCTL must indicate whether the original function invocation request is complete, and when not, it must indicate to the CA what it must do on behalf of the TA, and carry sufficient information to restart the original function invocation, whose secure thread is awaiting RPC resumption.

OCALL Contexts

During an call to TEE_IOC_INVOKE, several resources are allocated. These include memory shared with OP-TEE to carry the command arguments, any memory allocated to handle RPCs, as well as the call waiter. Seeing as an OCALL request is passed via an RPC, it is also necessary to keep RPC resumption information, such as the secure thread Id that sent the RPC.

All this information is kept in struct optee_ocall_ctx, which is allocated when the TA requests an OCALL. The resulting pointer to this structure is given an Id using the kernel's IDR mechanism, and is kept alive until the original function invocation is complete.

Values

To carry the information required by the new functionality, two additional values are necessary in the IOCTL parameters, in contrast with TEE_IOC_INVOKE. One value must carry a function ID that indicates the current state of the ECALL-OCALL cycle, and one other value must carry an identifier for the current OCALL context.

These two values are named func and ocall_id in struct tee_ioctl_ecall_arg. The element cmd_id is used to indicate the CA to TA command Id as well as the TA to CA OCALL Id in the opposite direction.

Typical Flows

There are two flows an OCALL can take: one that does not require passing memory references, and one which does.

Flow 1: No Shared Memory

CA            -[TEE_IOC_ECALL/Func Invoke]-> TEE Driver
TEE Driver    -[dispatch]->                  OP-TEE Driver
OP-TEE Driver -[SMC/Func Invoke]->           OP-TEE
OP-TEE        -[dispatch]->                  TA
TA            -[TEE_InvokeCACommand]->       OP-TEE
OP-TEE        -[dispatch]->                  OCALL PTA
OCALL PTA     -[RPC/OCALL Req']->            OP-TEE Driver
OP-TEE Driver -[ret]->                       TEE Driver
TEE Driver    -[ret]->                       CA

CA services OCALL

CA            -[TEE_IOC_ECALL/OCALL Complete]-> TEE Driver
TEE Driver    -[dispatch]->                     OP-TEE Driver
OP-TEE Driver -[SMC/RPC Resume]->               OP-TEE
OP-TEE        -[dispatch]->                     OCALL PTA
OCALL PTA     -[ret]->                          TA
TA            -[ret]->                          OP-TEE
OP-TEE        -[ret]->                          TEE Driver
TEE Driver     -[ret]->                         CA

Flow 2: Shared Memory Requested

CA            -[TEE_IOC_ECALL/Func Invoke]->      TEE Driver
TEE Driver    -[dispatch]->                       OP-TEE Driver
OP-TEE Driver -[SMC/Func Invoke]->                OP-TEE
OP-TEE        -[dispatch]->                       TA
TA            -[TEE_InvokeCACommand]->            OP-TEE
OP-TEE        -[dispatch]->                       OCALL PTA
OCALL PTA     -[RPC/SHM Alloc Req']->             OP-TEE Driver
OP-TEE Driver -[ret]->                            TEE Driver
TEE Driver    -[ret]->                            CA

CA allocates shared memory

CA            -[TEE_IOC_ECALL/SHM Alloc Compl']-> TEE Driver
TEE Driver    -[dispatch]->                       OP-TEE Driver
OP-TEE Driver -[SMC/RPC Resume]->                 OP-TEE
OP-TEE        -[dispatch]->                       OCALL PTA
OCALL PTA     -[RPC/OCALL Req']->                 OP-TEE Driver
OP-TEE Driver -[ret]->                            TEE Driver
TEE Driver    -[ret]->                            CA

CA services OCALL

CA            -[TEE_IOC_ECALL/OCALL Complete]->  TEE Driver
TEE Driver    -[dispatch]->                      OP-TEE Driver
OP-TEE Driver -[SMC/RPC Resume]->                OP-TEE
OP-TEE        -[dispatch]->                      OCALL PTA
OCALL PTA     -[RPC/SHM Free Req']->             OP-TEE Driver
OP-TEE Driver -[ret]->                           TEE Driver
TEE Driver    -[ret]->                           CA

CA frees shared memory

CA            -[TEE_IOC_ECALL/SHM Free Compl']-> TEE Driver
TEE Driver    -[dispatch]->                      OP-TEE Driver
OP-TEE Driver -[SMC/RPC Resume]->                OP-TEE
OP-TEE        -[dispatch]->                      OCALL PTA
OCALL PTA     -[ret]->                           TA
TA            -[ret]->                           OP-TEE
OP-TEE        -[ret]->                           TEE Driver
TEE Driver    -[ret]->                           CA

Abnormal Termination

Seeing as OCALL requests are transmitted to the normal world via RPCs, the requesting secure world thread remains suspended, and is not freed for reuse until the OCALL is complete one way or another. Should the CA terminate while handling an OCALL, or a shared memory allocation/free request, it is imperative that that OCALL be completed, albeit with an error, so that the secure world thread be resumed and allowed to run to completion. Failure to do this results in the eventual exhaustion of secure world threads, not mention that their corresponding TAs remain resident in memory.

If an OCALL does not require any shared memory, a CA that terminates during OCALL handling will see its TEE context released once its reference count reaches zero, which happens as that process' resources are released by Linux.

On the other hand, if an OCALL requires shared memory, a reference to it will be acquired by the OP-TEE driver, which in turn increases the reference count on the TEE context. If the CA terminates prematurely, the reference count on the TEE context will never reach zero.

To break this cycle, a new function callback is added in to the TEE driver function table, pre_release. This function is called when Linux releases the TEE context, yet memory references are still outstanding. The callback implementation in the OP-TEE driver cancels all pending OCALLs, returning an error code to the secure world, and reducing the reference count on outstanding shared memory allocations.

Signed-off-by: Hernan Gatta hegatta@microsoft.com

[HernanGatta]

@jenswi-linaro, cleaning up this PR is next up after doing that for the OP-TEE one. Thank you for looking at it.

Design question: We could re-use the existing TEE_IOC_INVOKE if we pass one more parameter, i.e., five instead of four.

The only purely technical reason for adding a new IOCTL is to be able to pass the cmd_id and ocall_id values back to the CA, then have those echoed right back into the kernel during the next invocation, and so on.

I'm thinking that if we pass an extra "meta"-type parameter in TEE_IOC_INVOKE, we could do away with the additional IOCTL. If we did the same for TEE_IOC_OPEN_SESSION, we could also support OCALLs during session open, which I believe is something you pointed out we can't currently do.

Since the TEE stack already supports a dynamic number of parameters via num_params in both cases, we don't risk breaking the ABI.

I added the new IOCTL because I figured back then that doing what I proposed above would be a misuse of parameters (and I was also prototyping, so starting with a brand new IOCTL allowed me change anything I wanted), but then I remembered that there are meta parameters being used for session open between Linux and OP-TEE, and between the TEE Supplicant and Linux, so we might as well do the same thing for CAs.

At this stage, I'm leaning toward the option of scrapping the new IOCTL and using an additional parameter. Also, if we ever need to extend the number of values passed back and forth, we can just use another parameter, or repurpose an existing parameter, instead of adding a new / modifying an existing IOCTL.

What do you think?

[jenswi-linaro]

I'm reviewing and looking at different options in this PR. It's a bit complicated so it may take a few days.

[HernanGatta]

I'm reviewing and looking at different options in this PR. It's a bit complicated so it may take a few days.

Thanks, no rush.

On my end, I'm working on an implementation of this that uses meta parameters instead. If I manage to do it cleanly enough, this should help enable OCALLs during session open as well. Additionally, I'm simplifying the OCALL handling logic, and trying to do so in such a way so as to leave the door open for zero-copy OCALLs.

[jenswi-linaro]

Meta parameters sounds good, I have a feeling that they could simplify things a bit. When you update in this PR, feel free to squash commits etc. Rebasing could also be a good idea.

[HernanGatta]

Just a heads-up, I'm still working on this. I did rewrite the changes to use meta parameters instead of a new IOCTL and it works great.

However, I've been doing some more testing with increasingly complex scenarios and this in-thread approach is ripe for deadlocks. I must admit I didn't see this coming when suggesting this approach in the previous iteration of this PR; I hadn't studied the way in which registered shared memory is handled in great detail (explanation follows).

Consider OP-TEE built with two threads (CFG_NUM_THREADS=2) and dynamic shared memory. You start a CA, initialize a context, open a session, and invoke a function. This function has a tmpref parameter, so as part of TEEC_InvokeFunction, the TEE Client API registers shared memory. Then, it calls into the invoke IOCTL, and we go into the TA. The TA performs an OCALL, the CA gets it, and crashes.

What happens?

When the kernel begins releasing file descriptors, it'll release the DMA buf underlying the registered shared memory, which makes a call into OP-TEE. OP-TEE attempts to free the MOBJ, but a reference is held to it by the function invocation. When this happens, OP-TEE blocks on the MOBJ free until other users of the MOBJ free their handle, which in this case is held by the call stack leading to the OCALL.

The OCALL is stuck, because the client crashed, so the two secure threads are now waiting on each other (by proxy, anyway): one is stuck waiting for the OCALL reply, and the other is stuck waiting for the MOBJ to be freed by the OCALL thread.

At this stage, there is no more progress in normal world because the kernel is releasing file descriptors and blocks on releasing the FD corresponding to the registered shared memory, which is in turn blocked by secure world performing an RPC to wait for the associated MOBJ lock to be released. Hence, we are not guaranteed to ever get to the pre_release hook, which would get called when the FD corresponding to the TEE context gets released, and the OCALL is never cancelled.

I tried an approach where I export a file descriptor each time we return from the IOCTL with a pending OCALL (and close it when we resume it). The idea is that if the CA crashes in between, the FD gets released, and as part of the FD release, we cancel the OCALL. But we can't guarantee the order in which the FD release gets processed, so the DMA buf release may occur before the OCALL FD release, and we block anyway.

We can ask users to crank up the number of secure threads, but then if you have more registered shared memory elements, and those FD's get closed ahead of the OCALL FD, you can still exhaust the number of secure world threads and lock up. Of course, this is also a problem is you have CFG_NUM_THREADS=1.

We have to have a way to make progress in the non-secure kernel if the CA crashes, and cancel pending OCALLs. I'm trying to cook up some synchronization/signaling mechanism to avoid deadlocking, but I'm starting to think it might be impossible.

[jenswi-linaro]

There's problems that needs to be addressed when the CA crashes while processing an OCALL.

1. The OCALL must be completed with an error from OP-TEE point of view
   • This allows OP-TEE to decrease the MOBJ refcount
2. SHMs should not be attempted to be freed while in use in OP-TEE
   • Can be avoided by increasing refcount on passed SHMs at invoke and later decreased again after the invoke is complete

This should make sure that the OP-TEE thread is freed before before SHMs in use with that thread are freed.

What do you think, is this enough?

[HernanGatta]

Can be avoided by increasing refcount on passed SHMs at invoke and later decreased again after the invoke is complete

Good idea. What you suggested in conjunction with creating a file descriptor while an OCALL is in progress should do the trick.

Just increasing the refcount on passed SHMs in the driver won't cut it, because then those refcounts will never reach zero, the context will never get released, and the pre_release hook will never get called. Additionally, since the CA isn't around anymore, there's nothing to prompt the driver to decrease the refcounts, so we're stuck again (though it's no longer a deadlock).

But, if we combine increasing the refcounts with exporting an FD whose release function cancels the OCALL, if necessary, and decreases the refcounts, it should work such that the SHMs don't get released until the OCALL FD gets released, at which point the OCALL secure thread is unblocked, thereby avoiding the deadlock.

I just threw together a proof-of-concept and I'm working through it. Thank you for your suggestion!

EDIT: It seems to work. I'll be doing some more testing, and if things check out, I'll have design-wise cleanup to do first. The exporting of an FD on a pending OCALL looks to me to be a good candidate for a TEE-agnostic framework, à la tee_shm (assuming, of course, that you all think this is even a good idea to begin with; we'll find out during review one way or another).

Thanks again for your suggestion: tweaking the refcounts to ensure release ordering appears to have been the missing piece of the puzzle. I'll report later on any further findings.

[jenswi-linaro]

teedev_close_context() is supposed to be called when the file descriptor of the context is closed. How can some SHMs not freed prevent that? It seems to me that we should be able to use the context FD to clean up instead of adding yet another FD.

If the freeing of FDs is blocked in secure world I see how we deadlock, but with refcounts increased on the SHMs which we could block on that shouldn't happen. Instead should those SHMs be freed later when the invoke is done and the SHM refcounts are decreased.

[HernanGatta]

teedev_close_context() is supposed to be called when the file descriptor of the context is closed. How can some SHMs not freed prevent that?

My mistake; I was thinking of the context release function proper, which gets called once the context's refcount reaches zero. You are correct, teedev_close_context does get called, regardless of any outstanding SHMs that still hold a reference. I'll try moving the OCALL release logic back to the pre_release hook.

If the freeing of FDs is blocked in secure world I see how we deadlock, but with refcounts increased on the SHMs which we could block on that shouldn't happen. Instead should those SHMs be freed later when the invoke is done and the SHM refcounts are decreased.

Indeed, that's the behavior I'm observing after I implemented your idea.

I'm currently observing sporadic deadlocks elsewhere; I'm trying to figure out what's going on.

[jenswi-linaro]

mobj_reg_shm_release_by_cookie() is not supposed to block, but if the MOBJ still is in used we can't let normal world believe that it has managed to free the MOBJ. So we wait. If that happens it's because normal world tries to release the SHM a bit early. The reference counting in normal world should avoid this, but if it happens anyway it's not accurate enough.

[HernanGatta]

Just to clarify, by "that's the behavior I'm observing", I meant that we don't block anymore in secure world after increasing the SHMs' refcounts in normal world. They now don't get released until the original invoke that requested one or more OCALLs has fully completed.

The deadlocks I mentioned I am seeing now are something different which I'm still working through understanding (these tend to occur when running multiple CAs concurrently, one of which performs OCALLs, and secure world blocks on closing a session, regardless of whether the CA that performs OCALLs crashed or not).

[jenswi-linaro]

OK, thanks.

[HernanGatta]

@jenswi-linaro, I've a question, if you wouldn't mind.

Suppose you build the emulated test environment in the usual way via the corresponding repo manifests, ARM32 or ARM64, it does not matter. In this configuration, OP-TEE's RPC argument cache is automatically enabled (the global thread_prealloc_rpc_cache is set to true).

Now you are at the prompt on the emulated Linux system. You log in and run optee_example_hello_world.

During session open, multiple RPC's are performed to load the TA. Since the RPC argument for the secure thread handling the request has not yet been allocated, an RPC is performed to allocate an SHM for the RPC arguments. This allocation is charged to the TEE context under which executes the CA, in this case optee_example_hello_world.

Seeing as thread_prealloc_rpc_cache is true, the RPC arguments SHM is never freed. Therefore, the refcount corresponding to the TEE context of the CA never reaches zero, and the TEE context is leaked.

To verify this, I've added traces in teedev_ctx_get, teedev_ctx_put, and teedev_ctx_release. These traces display the action taken, the address of the TEE context on which the action was taken, and the resulting refcount. For example:

teedev_ctx_get: incr refcount 0xc0363b80/3

This says that teedev_ctx_get was called and that the refcount of the TEE context at address 0xc0363b80 was increased to 3. Immediately below such lines you'll find a stack dump.

Additionally, I've added traces to show which RPC commands OP-TEE sends to the driver.

Then, I started the emulator (ARM32 in this case [1], but this reproes on ARM64, too), and ran optee_example_hello_world twice, collecting the resulting traces each time. I've placed the traces in a GitHub gist.

Notice how on line 27 of the first run, OP-TEE sends an OPTEE_SMC_RPC_FUNC_ALLOC RPC, and notice how there's a call to teedev_ctx_get immediately following, where the stack shows that it was called as part of the RPC handler. This charges the allocation of the RPC arguments SHM to the TEE context of the current CA. If you scroll to the bottom of the trace, you'll see that the final TEE context's refcount is 1. If I take that address and dump it via GDB after the CA has terminated, I see:

(gdb) print *(struct tee_context *)0xc0363b80
$2 = {
  teedev = 0xc0156400,
  list_shm = {
    next = 0xc0363c88,
    prev = 0xc0363c88
  },
  data = 0xc0363bc0,
  refcount = {
    refcount = {
      refs = {
        counter = 1
      }
    }
  },
  releasing = false,
  supp_nowait = false,
  cap_memref_null = false
}

Notice how refcount->refcount.refs.counter is 1 and releasing is false. Additionally, in that trace, teedev_ctx_release is never called.

In contrast, in the second trace, the RPC to allocate the RPC arguments SHM is not present, the final refcount is 0, and teedev_ctx_release is called.

If I place a call to exit(1) in the Hello World example CA after the function invocation but before the call to close the session and to release the context (i.e., Line 100 in main.c), then run that modified example as the first CA after a cold boot, the session is leaked because the TEE context is never released, which would have closed the outstanding session.

Question: Is this intentional?

[1]: Used repo init -u https://github.com/OP-TEE/manifest.git -m default.xml -b master at 10/04@6:00AM UTC.

[jforissier]

@HernanGatta I don't want to derail the discussion here ;-) but I think you're raising a point I already made in https://github.com/OP-TEE/optee_os/issues/1918 (well, actually @etienne-lms made it) so I do think we need to address this "leak" in some way or another.

[HernanGatta]

@jforissier, thanks for digging that up. I don't think I even knew what OP-TEE was when you created that issue :) .

The reason I brought this up in the first place is because I was testing my changes and noticed that the session would always get left behind in OP-TEE on the first run, but never thereafter, and spent longer than I'd like to admit until I figured out that it wasn't my fault.

It seems to me that this behavior is not exactly as it should be, but I agree we needn't derail this PR.

Thanks again.

[HernanGatta]

Just a heads-up, I've pushed what I have so far mainly to show you the new direction I'm taking these changes in, the idea being that if you really don't like what you see, I can stop early and reassess.

The biggest changes in the new version are:

1. Removal of the new ECALL IOCTL and the usage of an extra parameter in the INVOKE IOCTL instead whose sole function is to pass OCALL-related values;
2. Redesign of the way OCALLs are handled; it's now simpler and there's less duplication;
3. It's now possible to use the same new functions to have OCALLs during session open (TBD);
4. Cleaned up the commit history and made checkpatch happy;
5. Rebased atop the latest lin/optee.

With respect to the overall completeness of this PR, I'm still finding new ways of locking the system up when I get creative with threads in the CA. As such, I'm currently reworking the threading model, which, it turns out, is far from trivial when it comes to OCALL cancellations. The way I've done it in the latest changes, I've discovered, is incorrect.

Thanks.

[HernanGatta]

I believe I've addressed all the feedback.

I've pushed individual fix commits instead of squashing. While the contribution guidelines suggest doing individual commits for addressing PR feedback, if you find the changes are too difficult to review this way, let me know and I'll squash them (melding as appropriate).

Some outstanding issues:

1. OCALL support during session open;
2. OCALL support in the kernel TEE Client API implementation;
3. I can't tell what's wrong with the IBART failure.

Lastly, the known issue I mentioned in the original PR description has gone away after re-implementing the changes. It would indeed appear that I had done something wrong.

[jforissier]

@HernanGatta

I can't tell what's wrong with the IBART failure.

Me neither. @jbech-linaro can you take a look please?

[jbech-linaro]

@jbech-linaro can you take a look please?

Couldn't see anything at the server, I tried cloning manually and that worked. It could have been ISP issues. I've manually restarted the job for this PR. Let's wait and see if that makes any difference, if not, then I'll continue digging.

[jenswi-linaro]

Please squash the commits as appropriate.

[jbech-linaro]

It failed again, after checking more it looks like it's a timeout when syncing the submitter branch from GitHub. I've raised the "post_clone" timeout from 20 to 60 seconds and that seems fix the issue.

[HernanGatta]

I've squashed the changes as requested. I've also updated the PR for libteec and rebased it on master.

I'm currently working through adding tests into xtest before I add new features in the driver (namely OCALLs during session open and enabling OCALLs for the kernel client API); so far I've been testing with a custom-built program.

Thank you Jérôme and Joakim for looking into the test failure.

[HernanGatta]

Thank you for your review; I've applied the tags.

[HernanGatta]

Minor rephrasing suggested in commit log:

-Supporting OCALLs, where a TA may request a function invocation on its
+Implement OCALLs where a TA may request a function invocation on its

This would change the meaning of the sentence, though: the commit does not implement OCALLs; rather it introduces changes that help enable them in a subsequent commit.

[HernanGatta]

Thank you Etienne for your review; I've applied the tags, except for commit "tee: add support for supplicant-style memory registration", pending resolution of my previous comment on your suggested edit.

[HernanGatta]

I'm not sure we need ocall.c. OCALL isn't a layer there's bits and pieces everywhere. [...] I think the code would be easier to follow if the OCALL functions weren't kept apart.

Besides the removal of the optee_ocall_notify_* functions, which I agree with and removed, would you like to me to wholesale move the code in ocall.c into call.c?

I for one don't have a strong preference, and I think I see your point: the logic in call.c is now intertwined with the logic in ocall.c.

Thank you Jens for your review.

[jenswi-linaro]

... would you like to me to wholesale move the code in ocall.c into call.c?

Yes please.

Thank you for your patience with addressing all the comments, I really appreciate it.

[jenswi-linaro]

It looks like optee_open_session() doesn't handle OCALLs. Any special reason why it's not supported there?

[HernanGatta]

... would you like to me to wholesale move the code in ocall.c into call.c?

Yes please.

OK, will do.

Thank you for your patience with addressing all the comments, I really appreciate it.

You're welcome. I thank you and everyone else in turn for your patience and feedback, this has truly been great in ensuring that these changes are as good as they can be, and I'm genuinely happy to discuss and modify them.

It looks like optee_open_session() doesn't handle OCALLs. Any special reason why it's not supported there?

No reason other than I haven't done the work yet, along with supporting for them in the kernel-mode client API. I've been focusing on addressing feedback and ensuring we coalesce around the design and general direction of the feature. In the meantime, I've been working on the test suite, for which I'll have a PR up soon.

But I will add support for OCALLs during session open and in the kernel-mode client API, that's on the plan.

[jenswi-linaro]

No reason other than I haven't done the work yet...

Makes sense.

[HernanGatta]

I've added support for OCALLs during session open in this last update. Still working on support for them in the kernel-mode client API.

Sorry for being away this last month.

[HernanGatta]

Comments for "tee: optee: implement OCALL support"

This is looking really good, now I've started to look more into details.

I'd like to have the tee subsystem changes drivers/tee/*.c, include/linux/tee_drv.h and include/uapi/linux/tee.h into a separate commit.

@jenswi-linaro, sorry it's been a while. I've addressed your comments, rebased, and split the commits as you requested. I've also added a patch to the AMD TEE driver so that it line up with the changes to the TEE subsystem interface.

Thank you for your careful review all these months ago.

[jenswi-linaro]

Welcome back @HernanGatta, thank you for your patience with all comments! I'm going through this PR again. I'll let you know what I find.

[HernanGatta]

Welcome back @HernanGatta, thank you for your patience with all comments! I'm going through this PR again. I'll let you know what I find.

Thank you, of course, and thank you very much, respectively!

Question: I'd like to propose leaving the implementation of kernel-mode OCALLs for a future PR. I'm interested in the feature personally (as in, I want to do it for completeness), though it's technically unnecessary for the purposes that this PR was originally created for, namely to enable OCALL support in the Open Enclave SDK against vanilla OP-TEE (i.e., not against our fork). I can work on kernel-mode OCALL support later on my own free time. In order to merge these changes sooner than later, would this be acceptable? It's OK if it isn't.

[jenswi-linaro]

I'd like to propose leaving the implementation of kernel-mode OCALLs for a future PR.

Sure, no problem.

[jenswi-linaro]

I think we need to simplify the rules around how or where a shared memory object (SHM) is reference counted.

When we're entering in open_session/invoke all SHMs are increased with shm_from_user(). Just before returning from open_session/invoke those SHMs are decreased. This is what was done before this PR.

With OCALLs we're returning early from open_session/invoke so the original "normal" SHMs can't be decreased yet. Instead we must save them in a list for later processing, either when the function does a "normal" (as opposed to a OCALL) return or if the session or file descriptor is closed.

With a OCALL reply we increase the SHMs and store them in a special list to be decreased next time open_session/invoke returns. In the case of a OCALL request we should only consume the "OCALL reply" list of SHMs, but for a "normal" return we should process the "normal" list too.

I think this should cover all cases and also allow us to reduce the special cases for OCALL processing the in common code at least. Does this make any sense? Am I missing something?

[HernanGatta]

Hello @jenswi-linaro, I'm back working on this. I'm sorry for the extended hiatus, I should have let you all know I'd be away for a while (I didn't quite plan it that way). I'm looking at your comments now; the latest push is merely a rebase and doubles as a sign of life :).

[jenswi-linaro]

OK, welcome back! :-)

[etienne-lms]

@HernanGatta, until now I reused you patches without modifications. No change on the Linux patches. I only split the optee_os patch in 2, splitting CFG_OCALL into CFG_CORE_OCALL and CFG_USER_OCALL. I've not used the optee_client patches aside applying them to run the tests and examples you provided.

I have no new comment on your patches. If i remember correctly there is somewhere I needed to use 5 params at least (2 meta + 3 or 4 std) in the Ocall context because of a test in a tee or optee ocall sequence. Not an issue so far. I'll check that once I'm ready with the SCMI PTA Ocall patches for Linux kernel.