core: reset cancellation mask on TA entry

[jenswi-linaro]

Before this patch, the TA cancellation mask was only reset when the session was created, but the GP spec requires the cancellation mask to be reset each time a TA is entered via one of its entry points. So fix this by resetting the cancellation mask at the same time as when setting the cancellation request timeout.

Link: OP-TEE/optee_test#731
Fixes: b010477 ("Open-source the TEE Core")

Depends on OP-TEE/optee_test#732

@bvalliere @c-auger please provide your email addresses if you want a Reported-by: tag for this patch.

[c-auger]

My email adress is [email protected]

[c-auger]

By the way, I see that the cancel field of the session is reset to false in

optee_os/core/kernel/user_ta.c

Line 230 in 897aaf1

ta_sess->cancel = false;

At first glance, I would tell that the setting of cancel_mask and cancel should probably be done around the same location.

Also, now that I think of it, although TA_CloseSessionEntryPoint() and TA_DestroyEntryPoint() are not cancellable, maybe that we should set cancel_mask to false also in these cases, in case TEE_Wait() is called at some place in these places.

[jenswi-linaro]

At first glance, I would tell that the setting of cancel_mask and cancel should probably be done around the same location.

Yes, that makes sense.

Also, now that I think of it, although TA_CloseSessionEntryPoint() and TA_DestroyEntryPoint() are not cancellable, maybe that we should set cancel_mask to false also in these cases, in case TEE_Wait() is called at some place in these places.

I assume you mean to set cancel_mask to true as in masking cancellations. It seems that the spec intends to have cancellations masked by default so that a TA must actively enable cancellations each time it is entered.

TA_CloseSessionEntryPoint() is entered via user_ta_enter_close_session(), but TA_DestroyEntryPoint() called in the TA right after TA_CloseSessionEntryPoint() has returned so OP-TEE core can't reset cancel_mask in between. We could let the TA itself mask cancellations again, but that might be a bit too much.

[etienne-lms]

Typo in commit message?

- ... the TA cancellation was mask only reset when ...
+ ... the TA cancellation mask was only reset when ...

Acked-by: Etienne Carriere <[email protected]> for the implmentation.

[jforissier]

Reviewed-by: Jerome Forissier <[email protected]>

[jenswi-linaro]

Resetting the cancel flag in reset_entry_cancellation() isn't good since we may miss a previously queued cancellation. So better to restore the cancel_mask at the end of user_ta_enter() where we're currently restoring the cancel flag.

Are you OK with replacing it all with "core: reset cancellation mask on TA exit" instead?

[c-auger]

I think that indeed we should not reset cancel in reset_entry_cancellation(). I was wrong to suggest it the first time.

That said, when we were implementing our TEE using your frontend on non secure world, we spotted something that seemed strange to us, and is somehow out of the scope of this issue, so I will try to make it short.
Cancellations are by the specification bound to a TEEC_Operation which in turn is bound to a session. Still, in your implementation you set cancel_id of operations to 0, so I guess that you base your cancellation on sessions rather than operations. For instance, if you follow the following protocol:

• create an operation o1
• open a session with it
• create an operation o2
• create a thread
• invoke a command waiting for ever using o2 in one thread
• cancel the operation o1 (and not o2) in the other thread

Then I suspect but did not test (so I might be completely wrong about that) it that the invoke command will be cancelled, because you do not seem to track the operations themselves. Otherwise stated, I think that you cancel sessions and not operations.

Now to go back to the issue, yes, the sessions state should be reset after processing the operation, so that the next operation will start on a cleaned state. Maybe that it would be easier to follow the logic if you create a struct current_operation inside the structure of the session which will track the cancellation state. This structure requiring to be cleaned on operation termination and initiated on operation creation.

[jenswi-linaro]

Yes, we're currently canceling the current operation on a session rather than a specific operation. The ABI is defined to use a cancel_id in the current session, but we have never implemented that. That may change the day it becomes a problem.

If you're still using optee_client feel free to assign a cancel ID unique for the TEEC_Operation in the session.

[github-actions]

This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time.

[jforissier]

Very straightforward, LGTM. One minor comment re. the commit descriptions:

fix this by resetting the cancellation mask each time a TA exits.

"each time the TA returns" maybe? "Exits" sounds like it will not be entered again.

[jenswi-linaro]

Commit message updated and tag applied.

[jenswi-linaro]

I'm sorry, I forgot to squash the earlier commits and add Etienne's A-B, I've done that now.