Re: [PATCH v2] tee: amdtee: unload TA only when its refcount becomes 0
On 14/04/21 10:16 pm, Jens Wiklander wrote:
> Hi Jiro,
>
> On Mon, Apr 12, 2021 at 12:20 PM Rijo Thomas wrote:
>>
>>
>>
>> On 12/04/21 1:06 pm, Jens Wiklander wrote:
>>> On Mon, Apr 5, 2021 at 11:43 AM Rijo Thomas
>>> wrote:
Same Trusted Application (TA) can be loaded in multiple TEE contexts.
If it is a single instance TA, the TA should not get unloaded from AMD
Secure Processor, while it is still in use in another TEE context.
Therefore reference count TA and unload it when the count becomes zero.
Fixes: 757cc3e9ff1d ("tee: add AMD-TEE driver")
Reviewed-by: Devaraj Rangasamy
Signed-off-by: Rijo Thomas
---
v2:
* Unload TA if get_ta_refcount() fails
drivers/tee/amdtee/amdtee_private.h | 13
drivers/tee/amdtee/call.c | 94 ++---
drivers/tee/amdtee/core.c | 15 +++--
3 files changed, 106 insertions(+), 16 deletions(-)
>>>
>>> Looks good to me. Please address Dan's comment.
>>>
>>
>> Hi Jens,
>>
>> I have replied to Dan's comment.
>>
>> If you are okay with the current patch, request you to pull this.
>
> By addressing the comment I meant fixing it not just replying that
> you're going to ignore it. :-)
> I can't see any reason why the preferred style shouldn't be used.
>
:-) Sure Jens I will post patch v3 to address Dan's comment.
Thanks,
Rijo
> Cheers,
> Jens
>
Re: [PATCH v2] tee: amdtee: unload TA only when its refcount becomes 0
Hi Jiro,
On Mon, Apr 12, 2021 at 12:20 PM Rijo Thomas wrote:
>
>
>
> On 12/04/21 1:06 pm, Jens Wiklander wrote:
> > On Mon, Apr 5, 2021 at 11:43 AM Rijo Thomas
> > wrote:
> >>
> >> Same Trusted Application (TA) can be loaded in multiple TEE contexts.
> >>
> >> If it is a single instance TA, the TA should not get unloaded from AMD
> >> Secure Processor, while it is still in use in another TEE context.
> >>
> >> Therefore reference count TA and unload it when the count becomes zero.
> >>
> >> Fixes: 757cc3e9ff1d ("tee: add AMD-TEE driver")
> >> Reviewed-by: Devaraj Rangasamy
> >> Signed-off-by: Rijo Thomas
> >> ---
> >> v2:
> >> * Unload TA if get_ta_refcount() fails
> >>
> >> drivers/tee/amdtee/amdtee_private.h | 13
> >> drivers/tee/amdtee/call.c | 94 ++---
> >> drivers/tee/amdtee/core.c | 15 +++--
> >> 3 files changed, 106 insertions(+), 16 deletions(-)
> >
> > Looks good to me. Please address Dan's comment.
> >
>
> Hi Jens,
>
> I have replied to Dan's comment.
>
> If you are okay with the current patch, request you to pull this.
By addressing the comment I meant fixing it not just replying that
you're going to ignore it. :-)
I can't see any reason why the preferred style shouldn't be used.
Cheers,
Jens
Re: [PATCH v2] tee: amdtee: unload TA only when its refcount becomes 0
On 12/04/21 1:06 pm, Jens Wiklander wrote:
> On Mon, Apr 5, 2021 at 11:43 AM Rijo Thomas wrote:
>>
>> Same Trusted Application (TA) can be loaded in multiple TEE contexts.
>>
>> If it is a single instance TA, the TA should not get unloaded from AMD
>> Secure Processor, while it is still in use in another TEE context.
>>
>> Therefore reference count TA and unload it when the count becomes zero.
>>
>> Fixes: 757cc3e9ff1d ("tee: add AMD-TEE driver")
>> Reviewed-by: Devaraj Rangasamy
>> Signed-off-by: Rijo Thomas
>> ---
>> v2:
>> * Unload TA if get_ta_refcount() fails
>>
>> drivers/tee/amdtee/amdtee_private.h | 13
>> drivers/tee/amdtee/call.c | 94 ++---
>> drivers/tee/amdtee/core.c | 15 +++--
>> 3 files changed, 106 insertions(+), 16 deletions(-)
>
> Looks good to me. Please address Dan's comment.
>
Hi Jens,
I have replied to Dan's comment.
If you are okay with the current patch, request you to pull this.
Thanks,
Rijo
> Cheers,
> Jens
>
Re: [PATCH v2] tee: amdtee: unload TA only when its refcount becomes 0
On 09/04/21 2:15 pm, Dan Carpenter wrote:
> On Mon, Apr 05, 2021 at 03:13:09PM +0530, Rijo Thomas wrote:
>> @@ -340,7 +398,8 @@ int handle_open_session(struct
>> tee_ioctl_open_session_arg *arg, u32 *info,
>>
>> int handle_load_ta(void *data, u32 size, struct tee_ioctl_open_session_arg
>> *arg)
>> {
>> -struct tee_cmd_load_ta cmd = {0};
>> +struct tee_cmd_unload_ta unload_cmd = {0};
>> +struct tee_cmd_load_ta load_cmd = {0};
>
> It's better style to write:
>
> struct tee_cmd_unload_ta unload_cmd = {};
>
> It doesn't make a difference in this case, but if the first struct
> member is a pointer then {0} can generate a Sparse warning. Or
> depending on which bugs your version of GCC has it can affect whether
> struct holes are initialized. But mostly it's just the prefered style.
>
Hi Dan,
We do not have any pointers nor do I see a possibility of structure holes,
since all data
members are u32 in both struct tee_cmd_load_ta and struct tee_cmd_unload_ta.
So, will prefer
to use {0} for now.
Thanks,
Rijo
>
> regards,
> dan carpenter
>
Re: [PATCH v2] tee: amdtee: unload TA only when its refcount becomes 0
On Mon, Apr 5, 2021 at 11:43 AM Rijo Thomas wrote:
>
> Same Trusted Application (TA) can be loaded in multiple TEE contexts.
>
> If it is a single instance TA, the TA should not get unloaded from AMD
> Secure Processor, while it is still in use in another TEE context.
>
> Therefore reference count TA and unload it when the count becomes zero.
>
> Fixes: 757cc3e9ff1d ("tee: add AMD-TEE driver")
> Reviewed-by: Devaraj Rangasamy
> Signed-off-by: Rijo Thomas
> ---
> v2:
> * Unload TA if get_ta_refcount() fails
>
> drivers/tee/amdtee/amdtee_private.h | 13
> drivers/tee/amdtee/call.c | 94 ++---
> drivers/tee/amdtee/core.c | 15 +++--
> 3 files changed, 106 insertions(+), 16 deletions(-)
Looks good to me. Please address Dan's comment.
Cheers,
Jens
Re: [PATCH v2] tee: amdtee: unload TA only when its refcount becomes 0
On Mon, Apr 05, 2021 at 03:13:09PM +0530, Rijo Thomas wrote:
> @@ -340,7 +398,8 @@ int handle_open_session(struct tee_ioctl_open_session_arg
> *arg, u32 *info,
>
> int handle_load_ta(void *data, u32 size, struct tee_ioctl_open_session_arg
> *arg)
> {
> - struct tee_cmd_load_ta cmd = {0};
> + struct tee_cmd_unload_ta unload_cmd = {0};
> + struct tee_cmd_load_ta load_cmd = {0};
It's better style to write:
struct tee_cmd_unload_ta unload_cmd = {};
It doesn't make a difference in this case, but if the first struct
member is a pointer then {0} can generate a Sparse warning. Or
depending on which bugs your version of GCC has it can affect whether
struct holes are initialized. But mostly it's just the prefered style.
regards,
dan carpenter
[PATCH v2] tee: amdtee: unload TA only when its refcount becomes 0
Same Trusted Application (TA) can be loaded in multiple TEE contexts.
If it is a single instance TA, the TA should not get unloaded from AMD
Secure Processor, while it is still in use in another TEE context.
Therefore reference count TA and unload it when the count becomes zero.
Fixes: 757cc3e9ff1d ("tee: add AMD-TEE driver")
Reviewed-by: Devaraj Rangasamy
Signed-off-by: Rijo Thomas
---
v2:
* Unload TA if get_ta_refcount() fails
drivers/tee/amdtee/amdtee_private.h | 13
drivers/tee/amdtee/call.c | 94 ++---
drivers/tee/amdtee/core.c | 15 +++--
3 files changed, 106 insertions(+), 16 deletions(-)
diff --git a/drivers/tee/amdtee/amdtee_private.h
b/drivers/tee/amdtee/amdtee_private.h
index 337c8d82f74e..6d0f7062bb87 100644
--- a/drivers/tee/amdtee/amdtee_private.h
+++ b/drivers/tee/amdtee/amdtee_private.h
@@ -21,6 +21,7 @@
#define TEEC_SUCCESS 0x
#define TEEC_ERROR_GENERIC 0x
#define TEEC_ERROR_BAD_PARAMETERS 0x0006
+#define TEEC_ERROR_OUT_OF_MEMORY 0x000C
#define TEEC_ERROR_COMMUNICATION 0x000E
#define TEEC_ORIGIN_COMMS 0x0002
@@ -93,6 +94,18 @@ struct amdtee_shm_data {
u32 buf_id;
};
+/**
+ * struct amdtee_ta_data - Keeps track of all TAs loaded in AMD Secure
+ *Processor
+ * @ta_handle: Handle to TA loaded in TEE
+ * @refcount: Reference count for the loaded TA
+ */
+struct amdtee_ta_data {
+ struct list_head list_node;
+ u32 ta_handle;
+ u32 refcount;
+};
+
#define LOWER_TWO_BYTE_MASK0x
/**
diff --git a/drivers/tee/amdtee/call.c b/drivers/tee/amdtee/call.c
index 096dd4d92d39..06abaa518921 100644
--- a/drivers/tee/amdtee/call.c
+++ b/drivers/tee/amdtee/call.c
@@ -121,15 +121,69 @@ static int amd_params_to_tee_params(struct tee_param
*tee, u32 count,
return ret;
}
+static DEFINE_MUTEX(ta_refcount_mutex);
+static struct list_head ta_list = LIST_HEAD_INIT(ta_list);
+
+static u32 get_ta_refcount(u32 ta_handle)
+{
+ struct amdtee_ta_data *ta_data;
+ u32 count = 0;
+
+ /* Caller must hold a mutex */
+ list_for_each_entry(ta_data, _list, list_node)
+ if (ta_data->ta_handle == ta_handle)
+ return ++ta_data->refcount;
+
+ ta_data = kzalloc(sizeof(*ta_data), GFP_KERNEL);
+ if (ta_data) {
+ ta_data->ta_handle = ta_handle;
+ ta_data->refcount = 1;
+ count = ta_data->refcount;
+ list_add(_data->list_node, _list);
+ }
+
+ return count;
+}
+
+static u32 put_ta_refcount(u32 ta_handle)
+{
+ struct amdtee_ta_data *ta_data;
+ u32 count = 0;
+
+ /* Caller must hold a mutex */
+ list_for_each_entry(ta_data, _list, list_node)
+ if (ta_data->ta_handle == ta_handle) {
+ count = --ta_data->refcount;
+ if (count == 0) {
+ list_del(_data->list_node);
+ kfree(ta_data);
+ break;
+ }
+ }
+
+ return count;
+}
+
int handle_unload_ta(u32 ta_handle)
{
struct tee_cmd_unload_ta cmd = {0};
- u32 status;
+ u32 status, count;
int ret;
if (!ta_handle)
return -EINVAL;
+ mutex_lock(_refcount_mutex);
+
+ count = put_ta_refcount(ta_handle);
+
+ if (count) {
+ pr_debug("unload ta: not unloading %u count %u\n",
+ta_handle, count);
+ ret = -EBUSY;
+ goto unlock;
+ }
+
cmd.ta_handle = ta_handle;
ret = psp_tee_process_cmd(TEE_CMD_ID_UNLOAD_TA, (void *),
@@ -137,8 +191,12 @@ int handle_unload_ta(u32 ta_handle)
if (!ret && status != 0) {
pr_err("unload ta: status = 0x%x\n", status);
ret = -EBUSY;
+ } else {
+ pr_debug("unloaded ta handle %u\n", ta_handle);
}
+unlock:
+ mutex_unlock(_refcount_mutex);
return ret;
}
@@ -340,7 +398,8 @@ int handle_open_session(struct tee_ioctl_open_session_arg
*arg, u32 *info,
int handle_load_ta(void *data, u32 size, struct tee_ioctl_open_session_arg
*arg)
{
- struct tee_cmd_load_ta cmd = {0};
+ struct tee_cmd_unload_ta unload_cmd = {0};
+ struct tee_cmd_load_ta load_cmd = {0};
phys_addr_t blob;
int ret;
@@ -353,21 +412,36 @@ int handle_load_ta(void *data, u32 size, struct
tee_ioctl_open_session_arg *arg)
return -EINVAL;
}
- cmd.hi_addr = upper_32_bits(blob);
- cmd.low_addr = lower_32_bits(blob);
- cmd.size = size;
+ load_cmd.hi_addr = upper_32_bits(blob);
+ load_cmd.low_addr = lower_32_bits(blob);
+ load_cmd.size = size;
- ret = psp_tee_process_cmd(TEE_CMD_ID_LOAD_TA, (void *),
-
