tee_api: check for session->ctx before dereferencing it

[lorc]

We need to check if session->ctx is valid pointer befor dereferncing it.
Otherways it will lead to segfalut which is not desired behaviour for
library users.
This fill fix issue with segfaulting xtest when TEEC_OpenSession fails.

Signed-off-by: Volodymyr Babchuk vlad.babchuk@gmail.com

[jforissier]

Why do this? The client API is not supposed to be called with an invalid session or return an error status if that happens. I mean, why check ctx against zero when there are so many invalid virtual address values... In fact rather than adding tests, I would rather remove the NULL checks on the session pointer, except in TEE_CloseSession() for convenience, like free() accepts a NULL pointer. I'll check the GP spec and comment further tomorrow.

[lorc]

@jforissier , yes, that is the case. You can feed free() with NULL pointer and nothing bad will happen.
But in this case whole application crashes. In my opinion, library (even incorrectly used) should not crash application. It can return errors, write warnings to stderr, maybe even trigger asserts, but it should not cause segfault. It should be foolproof.

So, I have exactly opposite idea: lets leave NULL checks for session pointer and also lets add field with magic value to session structure. This will ensure that library always works with correctly created session. What do you think?

[jforissier]

@lorc there is no way you can make the client API 'foolproof', reliably, and safely. Adding a magic value to the session structure will only catch a limited number of invalid sessions. What if the session pointer itself is bogus? Even with your proposed magic number. you'll still crash when de-referencing the pointer to read the magic value.
Parameter validation by APIs is a somewhat debated topic, you can find many references on the Internet. This one for instance: https://blogs.msdn.microsoft.com/larryosterman/2004/05/18/should-i-check-the-parameters-to-my-function/. It's about the Windows environment, so it's not entirely applicable to our discussion, but it shows that adding validation steps may add more issues than it actually solves.

I checked the GP spec, and there is nothing wrong with our current behavior. Only TEEC_CloseSession() is expected to check for session == NULL and do nothing in this case. Passing invalid parameters falls in the category called "Programmer Error", for which: "the implementation is not required to gracefully handle the error or even behave consistently, but MAY choose to generate a programmer visible response. [...] the Implementation MUST still guarantee the stability and security of the TEE and the shared communication subsystem in the rich environment because these modules are shared amongst all Client Applications and must not be affected by the misbehavior of a single Client Application".

So I stand on my position: no new validity checks, and let's keep the NULL checks on the session pointer that we already have because they're cheap, safe, perhaps moderately helpful, and compliant with the spec.

PS: you mentioned segfaults in xtest when TEEC_OpenSession() fails, I wouldn't mind a patch to fix that ;)

[jenswi-linaro]

You're right Jerome. It doesn't become more robust with these added checks, what we do get is more unpredictable behavior. I think everyone prefers an early consistent crash compared to a late one that needs certain stuff to happen first.

[lorc]

@jforissier Okay, I see your point. I'll rework xtest then.

[jforissier]

@lorc thank you.