| Message ID | 20231111111559.8218-5-yong.wu@mediatek.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | dma-buf: heaps: Add secure heap | expand |
Hi Yong,
kernel test robot noticed the following build errors:
[auto build test ERROR on drm-misc/drm-misc-next]
[also build test ERROR on robh/for-next drm-tip/drm-tip linus/master v6.6 next-20231110]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Yong-Wu/dma-buf-heaps-Initialize-a-secure-heap/20231111-192115
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link: https://lore.kernel.org/r/20231111111559.8218-5-yong.wu%40mediatek.com
patch subject: [PATCH v2 4/8] dma-buf: heaps: secure_heap: Add tee memory service call
config: powerpc-allmodconfig (https://download.01.org/0day-ci/archive/20231112/202311120707.FDrzrRME-lkp@intel.com/config)
compiler: powerpc64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231112/202311120707.FDrzrRME-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311120707.FDrzrRME-lkp@intel.com/
All errors (new ones prefixed by >>):
powerpc64-linux-ld: warning: discarding dynamic section .glink
powerpc64-linux-ld: warning: discarding dynamic section .plt
powerpc64-linux-ld: linkage table error against `tee_client_open_session'
powerpc64-linux-ld: stubs don't match calculated size
powerpc64-linux-ld: can not build stubs: bad value
powerpc64-linux-ld: drivers/dma-buf/heaps/secure_heap.o: in function `secure_heap_tee_session_init':
secure_heap.c:(.text.secure_heap_tee_session_init+0xd0): undefined reference to `tee_client_open_context'
powerpc64-linux-ld: secure_heap.c:(.text.secure_heap_tee_session_init+0x2b4): undefined reference to `tee_client_open_session'
powerpc64-linux-ld: secure_heap.c:(.text.secure_heap_tee_session_init+0x458): undefined reference to `tee_client_close_context'
powerpc64-linux-ld: drivers/dma-buf/heaps/secure_heap.o: in function `secure_heap_tee_service_call.constprop.0':
>> secure_heap.c:(.text.secure_heap_tee_service_call.constprop.0+0xbc): undefined reference to `tee_client_invoke_func'
On Sat, Nov 11, 2023 at 3:17 AM Yong Wu <yong.wu@mediatek.com> wrote: > > Add TEE service call. In the case of MediaTek, secure memory management is > located within the TEE. The kernel just needs to tell TEE what size it > needs and the TEE will return a "security handle" to kernel. > > To be consistent with the cma heap later, we put the tee ops into the ops > of secure_the_memory. > > It seems that secure_heap_tee_service_call could be a more general > interface, but it could be a new topic. > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > --- > drivers/dma-buf/heaps/secure_heap.c | 97 +++++++++++++++++++++++++++++ > 1 file changed, 97 insertions(+) > > diff --git a/drivers/dma-buf/heaps/secure_heap.c b/drivers/dma-buf/heaps/secure_heap.c > index 2a037fc54004..05062c14e7c7 100644 > --- a/drivers/dma-buf/heaps/secure_heap.c > +++ b/drivers/dma-buf/heaps/secure_heap.c > @@ -17,6 +17,27 @@ > > #define TEE_PARAM_NUM 4 > > +enum secure_buffer_tee_cmd { /* PARAM NUM always is 4. */ > + /* > + * TZCMD_SECMEM_ZALLOC: Allocate the zeroed secure memory from TEE. > + * > + * [in] value[0].a: The buffer size. > + * value[0].b: alignment. > + * [in] value[1].a: enum secure_memory_type. > + * [out] value[3].a: The secure handle. > + */ > + TZCMD_SECMEM_ZALLOC = 0, > + > + /* > + * TZCMD_SECMEM_FREE: Free secure memory. > + * > + * [in] value[0].a: The secure handle of this buffer, It's value[3].a of > + * TZCMD_SECMEM_ZALLOC. > + * [out] value[1].a: return value, 0 means successful, otherwise fail. > + */ > + TZCMD_SECMEM_FREE = 1, > +}; > + This should go in the MTK specific implementation. > enum secure_memory_type { > /* > * MediaTek static chunk memory carved out for TrustZone. The memory > @@ -28,13 +49,25 @@ enum secure_memory_type { > struct secure_buffer { > struct dma_heap *heap; > size_t size; > + /* > + * The secure handle is a reference to a buffer within the TEE, this is > + * a value got from TEE. > + */ > + u32 sec_handle; > }; Change this to a u64 and rename it to 'secure_address', it's up to the specific implementation what that would actually mean. > > +#define TEE_MEM_COMMAND_ID_BASE_MTK 0x10000 > + Move this into the MTK specific implementation. > struct secure_heap; > > struct secure_heap_prv_data { > const char *uuid; > const int tee_impl_id; > + /* > + * Different TEEs may implement different commands, and this provides an opportunity > + * for TEEs to use the same enum secure_buffer_tee_cmd. > + */ > + const int tee_command_id_base; Remove this, it can be handled in the MTK specific implementation. > > int (*memory_alloc)(struct secure_heap *sec_heap, struct secure_buffer *sec_buf); > void (*memory_free)(struct secure_heap *sec_heap, struct secure_buffer *sec_buf); > @@ -98,10 +131,74 @@ static int secure_heap_tee_session_init(struct secure_heap *sec_heap) > return ret; > } > > +static int > +secure_heap_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 = 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; > +} > + > +static int secure_heap_tee_secure_memory(struct secure_heap *sec_heap, > + struct secure_buffer *sec_buf) > +{ > + const struct secure_heap_prv_data *data = sec_heap->data; > + struct tee_param params[TEE_PARAM_NUM] = {0}; > + int ret; > + > + params[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT; > + params[0].u.value.a = sec_buf->size; > + params[0].u.value.b = PAGE_SIZE; > + params[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT; > + params[1].u.value.a = sec_heap->mem_type; > + params[2].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT; > + > + params[3].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT; > + ret = secure_heap_tee_service_call(sec_heap->tee_ctx, sec_heap->tee_session, > + data->tee_command_id_base + TZCMD_SECMEM_ZALLOC, > + params); > + if (ret) > + return -ENOMEM; > + > + sec_buf->sec_handle = params[3].u.value.a; > + return 0; > +} > + > +static void secure_heap_tee_unsecure_memory(struct secure_heap *sec_heap, > + struct secure_buffer *sec_buf) > +{ > + struct tee_param params[TEE_PARAM_NUM] = {0}; > + > + 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; > + > + secure_heap_tee_service_call(sec_heap->tee_ctx, sec_heap->tee_session, > + sec_heap->data->tee_command_id_base + TZCMD_SECMEM_FREE, > + params); > + if (params[1].u.value.a) > + pr_err("%s, free buffer(0x%x) return fail(%lld) from TEE.\n", > + sec_heap->name, sec_buf->sec_handle, params[1].u.value.a); > +} > + These are entirely MTK specific, so move them into the MTK specific implementation. > /* The memory allocating is within the TEE. */ > const struct secure_heap_prv_data mtk_sec_mem_data = { > .uuid = TZ_TA_MEM_UUID_MTK, > .tee_impl_id = TEE_IMPL_ID_OPTEE, > + .tee_command_id_base = TEE_MEM_COMMAND_ID_BASE_MTK, > + .secure_the_memory = secure_heap_tee_secure_memory, > + .unsecure_the_memory = secure_heap_tee_unsecure_memory, > }; This should also go into the MTK specific implementation, and to be clear, that's where module_init should be as well. > > static int secure_heap_secure_memory_allocate(struct secure_heap *sec_heap, > -- > 2.25.1 >
diff --git a/drivers/dma-buf/heaps/secure_heap.c b/drivers/dma-buf/heaps/secure_heap.c index 2a037fc54004..05062c14e7c7 100644 --- a/drivers/dma-buf/heaps/secure_heap.c +++ b/drivers/dma-buf/heaps/secure_heap.c @@ -17,6 +17,27 @@ #define TEE_PARAM_NUM 4 +enum secure_buffer_tee_cmd { /* PARAM NUM always is 4. */ + /* + * TZCMD_SECMEM_ZALLOC: Allocate the zeroed secure memory from TEE. + * + * [in] value[0].a: The buffer size. + * value[0].b: alignment. + * [in] value[1].a: enum secure_memory_type. + * [out] value[3].a: The secure handle. + */ + TZCMD_SECMEM_ZALLOC = 0, + + /* + * TZCMD_SECMEM_FREE: Free secure memory. + * + * [in] value[0].a: The secure handle of this buffer, It's value[3].a of + * TZCMD_SECMEM_ZALLOC. + * [out] value[1].a: return value, 0 means successful, otherwise fail. + */ + TZCMD_SECMEM_FREE = 1, +}; + enum secure_memory_type { /* * MediaTek static chunk memory carved out for TrustZone. The memory @@ -28,13 +49,25 @@ enum secure_memory_type { struct secure_buffer { struct dma_heap *heap; size_t size; + /* + * The secure handle is a reference to a buffer within the TEE, this is + * a value got from TEE. + */ + u32 sec_handle; }; +#define TEE_MEM_COMMAND_ID_BASE_MTK 0x10000 + struct secure_heap; struct secure_heap_prv_data { const char *uuid; const int tee_impl_id; + /* + * Different TEEs may implement different commands, and this provides an opportunity + * for TEEs to use the same enum secure_buffer_tee_cmd. + */ + const int tee_command_id_base; int (*memory_alloc)(struct secure_heap *sec_heap, struct secure_buffer *sec_buf); void (*memory_free)(struct secure_heap *sec_heap, struct secure_buffer *sec_buf); @@ -98,10 +131,74 @@ static int secure_heap_tee_session_init(struct secure_heap *sec_heap) return ret; } +static int +secure_heap_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 = 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; +} + +static int secure_heap_tee_secure_memory(struct secure_heap *sec_heap, + struct secure_buffer *sec_buf) +{ + const struct secure_heap_prv_data *data = sec_heap->data; + struct tee_param params[TEE_PARAM_NUM] = {0}; + int ret; + + params[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT; + params[0].u.value.a = sec_buf->size; + params[0].u.value.b = PAGE_SIZE; + params[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT; + params[1].u.value.a = sec_heap->mem_type; + params[2].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT; + + params[3].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT; + ret = secure_heap_tee_service_call(sec_heap->tee_ctx, sec_heap->tee_session, + data->tee_command_id_base + TZCMD_SECMEM_ZALLOC, + params); + if (ret) + return -ENOMEM; + + sec_buf->sec_handle = params[3].u.value.a; + return 0; +} + +static void secure_heap_tee_unsecure_memory(struct secure_heap *sec_heap, + struct secure_buffer *sec_buf) +{ + struct tee_param params[TEE_PARAM_NUM] = {0}; + + 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; + + secure_heap_tee_service_call(sec_heap->tee_ctx, sec_heap->tee_session, + sec_heap->data->tee_command_id_base + TZCMD_SECMEM_FREE, + params); + if (params[1].u.value.a) + pr_err("%s, free buffer(0x%x) return fail(%lld) from TEE.\n", + sec_heap->name, sec_buf->sec_handle, params[1].u.value.a); +} + /* The memory allocating is within the TEE. */ const struct secure_heap_prv_data mtk_sec_mem_data = { .uuid = TZ_TA_MEM_UUID_MTK, .tee_impl_id = TEE_IMPL_ID_OPTEE, + .tee_command_id_base = TEE_MEM_COMMAND_ID_BASE_MTK, + .secure_the_memory = secure_heap_tee_secure_memory, + .unsecure_the_memory = secure_heap_tee_unsecure_memory, }; static int secure_heap_secure_memory_allocate(struct secure_heap *sec_heap,
Add TEE service call. In the case of MediaTek, secure memory management is located within the TEE. The kernel just needs to tell TEE what size it needs and the TEE will return a "security handle" to kernel. To be consistent with the cma heap later, we put the tee ops into the ops of secure_the_memory. It seems that secure_heap_tee_service_call could be a more general interface, but it could be a new topic. Signed-off-by: Yong Wu <yong.wu@mediatek.com> --- drivers/dma-buf/heaps/secure_heap.c | 97 +++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+)