[Linaro-mm-sig] Re: [PATCH 6/9] dma-buf: heaps: mtk_sec_heap: Add tee service call for buffer allocating/freeing

On Mon, Sep 11, 2023 at 10:30:35AM +0800, Yong Wu wrote:

Add TEE service call for secure memory allocating/freeing.

Signed-off-by: Anan Sun anan.sun@mediatek.com Signed-off-by: Yong Wu yong.wu@mediatek.com

---

drivers/dma-buf/heaps/mtk_secure_heap.c | 69 ++++++++++++++++++++++++- 1 file changed, 68 insertions(+), 1 deletion(-)

diff --git a/drivers/dma-buf/heaps/mtk_secure_heap.c b/drivers/dma-buf/heaps/mtk_secure_heap.c index e3da33a3d083..14c2a16a7164 100644 --- a/drivers/dma-buf/heaps/mtk_secure_heap.c +++ b/drivers/dma-buf/heaps/mtk_secure_heap.c @@ -17,6 +17,9 @@ #define MTK_TEE_PARAM_NUM 4 +#define TZCMD_MEM_SECURECM_UNREF 7 +#define TZCMD_MEM_SECURECM_ZALLOC 15

This is related to the discussion around UUID as well. These numbers here are specific to the MediaTek TA. If we could make things more generic, then these should probably be 0 and 1.

I also find the naming a bit heavy, I think I'd suggest something like: # define TEE_CMD_SECURE_HEAP_ZALLOC ... and so on.

/*

• MediaTek secure (chunk) memory type

@@ -29,6 +32,8 @@ enum kree_mem_type {

The "kree" here, is that meant to indicate kernel REE? If yes, then I guess that could be dropped since we know we're already in the kernel context, perhaps instead name it something with "secure_heap_type"?

struct mtk_secure_heap_buffer { struct dma_heap *heap; size_t size;

• u32 sec_handle;

}; struct mtk_secure_heap { @@ -80,6 +85,63 @@ static int mtk_kree_secure_session_init(struct mtk_secure_heap *sec_heap) return ret; } +static int +mtk_sec_mem_tee_service_call(struct tee_context *tee_ctx, u32 session,

• 	     unsigned int command, struct tee_param *params)
  

+{

• struct tee_ioctl_invoke_arg arg = {0};
• int ret;
• arg.num_params = MTK_TEE_PARAM_NUM;
• arg.session = session;
• arg.func = command;
• ret = tee_client_invoke_func(tee_ctx, &arg, params);
• if (ret < 0 || arg.ret) {

• pr_err("%s: cmd %d ret %d:%x.\n", __func__, command, ret, arg.ret);
  

• ret = -EOPNOTSUPP;
  

• }
• return ret;

+}

Perhaps not relevant for this patch set, but since this function is just a pure TEE call, I'm inclined to suggest that this could even be moved out as a generic TEE function. I.e., something that could be used elsewhere in the Linux kernel.

+static int mtk_sec_mem_allocate(struct mtk_secure_heap *sec_heap,

• 		struct mtk_secure_heap_buffer *sec_buf)
  

+{

• struct tee_param params[MTK_TEE_PARAM_NUM] = {0};
• u32 mem_session = sec_heap->mem_session;

How about name it tee_session? Alternative ta_session? I think that would better explain what it actually is.

• int ret;
• params[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
• params[0].u.value.a = SZ_4K; /* alignment */
• params[0].u.value.b = sec_heap->mem_type; /* memory type */
• params[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
• params[1].u.value.a = sec_buf->size;
• params[2].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT;
• /* Always request zeroed buffer */
• ret = mtk_sec_mem_tee_service_call(sec_heap->tee_ctx, mem_session,

• 			   TZCMD_MEM_SECURECM_ZALLOC, params);
  

• if (ret)

• return -ENOMEM;
  

• sec_buf->sec_handle = params[2].u.value.a;
• return 0;

+}

+static void mtk_sec_mem_release(struct mtk_secure_heap *sec_heap,

• 		struct mtk_secure_heap_buffer *sec_buf)
  

+{

• struct tee_param params[MTK_TEE_PARAM_NUM] = {0};
• u32 mem_session = sec_heap->mem_session;
• params[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
• params[0].u.value.a = sec_buf->sec_handle;
• params[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT;

Perhaps worth having a comment for params[1] explain why we need the VALUE_OUTPUT here?