core: ta flags fixup #1574
core: ta flags fixup #1574
Conversation
core/arch/arm/kernel/user_ta.c
Outdated
| @@ -313,6 +313,14 @@ static TEE_Result ta_load(const TEE_UUID *uuid, | |||
| goto error_return; | |||
| } | |||
|
|
|||
| if (ta_head->flags & TA_FLAG_MULTI_SESSION && | |||
There was a problem hiding this comment.
I don't fully agree with this approach. If it's a problem that those flags may be set then they shouldn't have been set from the beginning and we should just refuse to load the TA.
There was a problem hiding this comment.
From the GP spec, we should just ignore the unmatched flags but not refuse. Maybe a warning message is needed here.
There was a problem hiding this comment.
@jenswi-linaro maybe not. The GP Internal Core API v1.1, table 4-11 "Trusted Application Standard Configuration Properties" says (emphasis mine): "gpd.ta.multiSession Boolean [...] This property is ignored for multi-instance Trusted Applications".
As for the keep-alive flag, the wording is a bit more ambiguous (emphasis mine again): "gpd.ta.instanceKeepAlive Boolean [...] This property is meaningful only when the gpd.ta.singleInstance is set to true"
@prime-zeng does this patch actually change the behavior of OP-TEE? In other words:
- What happens if
TA_FLAG_SINGLE_INSTANCEis not set andTA_FLAG_MULTI_SESSIONis set, compared to both not set? - What happens if
TA_FLAG_SINGLE_INSTANCEis not set andTA_FLAG_INSTANCE_KEEP_ALIVEis set, compared to both not set?
There was a problem hiding this comment.
@jforissier I have test the case TA_FLAG_INSTANCE_KEEP_ALIVE is set and TA_FLAG_SINGLE_INSTANCE not set, when running the same TA, it will soon get the error message "Failed to allocate ASID", i think some resources are leaked .
There was a problem hiding this comment.
Given this I think we should not modify any flags, because doing so is a bit confusing. I'd rather interpret the flags correctly, in this case it's the if at the end of tee_ta_close_session() that need to take TA_FLAG_SINGLE_INSTANCE into account also.
There was a problem hiding this comment.
@jenswi-linaro do you mean that we force the TA_FLAG_SINGLE_INSTANCE flag when TA_FLAG_INSTANCE_KEEP_ALIVE or TA_FLAG_MULTI_SESSION is set? if so, i think it is a bit different from what the GP Internal Core API v1.1 describes.
There was a problem hiding this comment.
No, we should follow the spec. But I don't want to change any flags, instead whenever we look at for instance TA_FLAG_INSTANCE_KEEP_ALIVE we need to check TA_FLAG_SINGLE_INSTANCE also because if it's not set then TA_FLAG_INSTANCE_KEEP_ALIVE isn't relevant.
|
|
According to the The GP Internal Core API v1.1: The keepalive flag should be ignored when the single instance flag is not set. Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Signed-off-by: Zeng Tao <prime.zeng@hisilicon.com>
|
Update, tag applied. |
|
Thanks @prime-zeng. |
Ignore the TA_FLAG_MULTI_SESSION and TA_FLAG_INSTANCE_KEEP_ALIVE when
the TA_FLAG_SINGLE_INSTANCE is not set.
Signed-off-by: Zeng Tao prime.zeng@hisilicon.com