OP-TEE / optee_test

Test suite
Other
81 stars 199 forks source link

Should this open session fail in xtest 1021? #561

Closed jingdlu closed 3 years ago

jingdlu commented 3 years ago

https://github.com/OP-TEE/optee_test/blob/13c4de15fa0370bfd37de476f02c103fd47ccc47/host/xtest/regression_1000.c#L1762

Because this line code will open session to sims_keepalive_test_ta. But this TA is keepalive and panic before. I think the panic status of TA ctx will be kept always once TA panic happens before.

Thanks, Jingdong

jforissier commented 3 years ago

Hi @jingdlu,

I checked the GlobalPlatform TEE Internal Core API specification and I find it unclear on what should happen in this scenario.

So should a panic in a keep alive TA destroy the instance or not? I do not think it makes sense to keep the instance (TA context) because the resources used by this context would never be released yet the TA would never be able to make progress (since it panicked). So I think the current behavior is correct, but I can ask GP for clarifications if you think it can be helpful.

jingdlu commented 3 years ago

Hi @jforissier

Thanks for your reply in details.

It's OK for me to destroy the instance. But I'm using OP-TEE 3.4 code. In tee_ta_close_session function, it will not destroy the TA context if it's keep alive TA: if (!ctx->ref_count && !keep_alive) { DMSG("Destroy TA ctx");

So when I run xtest 1021, open session to sims_keepalive_test_ta second time will fail due to panic in TA context. if (ctx->panicked) { DMSG("panicked, call tee_ta_close_session()"); tee_ta_close_session(s, open_sessions, KERN_IDENTITY); *err = TEE_ORIGIN_TEE; return TEE_ERROR_TARGET_DEAD; }

What's the possible reason?

Thanks, Jingdong

jforissier commented 3 years ago

So I think the current behavior is correct, but I can ask GP for clarifications if you think it can be helpful.

Confirmed. This issue has already been discussed in the past, see here: https://github.com/OP-TEE/optee_os/issues/2671#issuecomment-443723358.

As for any difference between OP-TEE 3.4.0 and the current version, I would say the current code is correct and the older one is not.

jingdlu commented 3 years ago

Thanks for pointing out this.