Add a binding for describing the dynamic restricted reserved memory range. The memory range also will be defined in the TEE firmware. It means the TEE will be configured with the same address/size that is being set in this DT node. Regarding to the detail TEE command, Please search MTK_TZCMD_SECMEM_ZALLOC and MTK_TZCMD_SECMEM_FREE.

Signed-off-by: Yong Wu yong.wu@mediatek.com --- .../mediatek,dynamic-restricted-region.yaml | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,dynamic-restricted-region.yaml

diff --git a/Documentation/devicetree/bindings/reserved-memory/mediatek,dynamic-restricted-region.yaml b/Documentation/devicetree/bindings/reserved-memory/mediatek,dynamic-restricted-region.yaml new file mode 100644 index 000000000000..5cbe3a5637fa --- /dev/null +++ b/Documentation/devicetree/bindings/reserved-memory/mediatek,dynamic-restricted-region.yaml @@ -0,0 +1,43 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/reserved-memory/mediatek,dynamic-restricted-re... +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: MediaTek Dynamic Reserved Region + +description: + A memory region that can dynamically transition as a whole between + secure and non-secure states. This memory will be protected by OP-TEE + when allocations are active and unprotected otherwise. + +maintainers: + - Yong Wu yong.wu@mediatek.com + +allOf: + - $ref: reserved-memory.yaml + +properties: + compatible: + const: mediatek,dynamic-restricted-region + +required: + - compatible + - reg + - reusable + +unevaluatedProperties: false + +examples: + - | + reserved-memory { + #address-cells = <1>; + #size-cells = <1>; + ranges; + + reserved-memory@80000000 { + compatible = "mediatek,dynamic-restricted-region"; + reg = <0x80000000 0x18000000>; + reusable; + }; + };

Add "struct restricted_heap_ops". For the restricted memory, totally there are two steps: a) memory_alloc: Allocate the buffer in kernel; b) memory_restrict: Restrict/Protect/Secure that buffer. The memory_alloc is mandatory while memory_restrict is optinal since it may be part of memory_alloc.

Signed-off-by: Yong Wu yong.wu@mediatek.com --- drivers/dma-buf/heaps/restricted_heap.c | 41 ++++++++++++++++++++++++- drivers/dma-buf/heaps/restricted_heap.h | 12 ++++++++ 2 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/dma-buf/heaps/restricted_heap.c b/drivers/dma-buf/heaps/restricted_heap.c index fd7c82abd42e..8c266a0f6192 100644 --- a/drivers/dma-buf/heaps/restricted_heap.c +++ b/drivers/dma-buf/heaps/restricted_heap.c @@ -12,10 +12,44 @@

#include "restricted_heap.h"

+static int +restricted_heap_memory_allocate(struct restricted_heap *heap, struct restricted_buffer *buf) +{ + const struct restricted_heap_ops *ops = heap->ops; + int ret; + + ret = ops->memory_alloc(heap, buf); + if (ret) + return ret; + + if (ops->memory_restrict) { + ret = ops->memory_restrict(heap, buf); + if (ret) + goto memory_free; + } + return 0; + +memory_free: + ops->memory_free(heap, buf); + return ret; +} + +static void +restricted_heap_memory_free(struct restricted_heap *heap, struct restricted_buffer *buf) +{ + const struct restricted_heap_ops *ops = heap->ops; + + if (ops->memory_unrestrict) + ops->memory_unrestrict(heap, buf); + + ops->memory_free(heap, buf); +} + static struct dma_buf * restricted_heap_allocate(struct dma_heap *heap, unsigned long size, unsigned long fd_flags, unsigned long heap_flags) { + struct restricted_heap *restricted_heap = dma_heap_get_drvdata(heap); struct restricted_buffer *restricted_buf; DEFINE_DMA_BUF_EXPORT_INFO(exp_info); struct dma_buf *dmabuf; @@ -28,6 +62,9 @@ restricted_heap_allocate(struct dma_heap *heap, unsigned long size, restricted_buf->size = ALIGN(size, PAGE_SIZE); restricted_buf->heap = heap;

+ ret = restricted_heap_memory_allocate(restricted_heap, restricted_buf); + if (ret) + goto err_free_buf; exp_info.exp_name = dma_heap_get_name(heap); exp_info.size = restricted_buf->size; exp_info.flags = fd_flags; @@ -36,11 +73,13 @@ restricted_heap_allocate(struct dma_heap *heap, unsigned long size, dmabuf = dma_buf_export(&exp_info); if (IS_ERR(dmabuf)) { ret = PTR_ERR(dmabuf); - goto err_free_buf; + goto err_free_restricted_mem; }

return dmabuf;

+err_free_restricted_mem: + restricted_heap_memory_free(restricted_heap, restricted_buf); err_free_buf: kfree(restricted_buf); return ERR_PTR(ret); diff --git a/drivers/dma-buf/heaps/restricted_heap.h b/drivers/dma-buf/heaps/restricted_heap.h index 443028f6ba3b..ddeaf9805708 100644 --- a/drivers/dma-buf/heaps/restricted_heap.h +++ b/drivers/dma-buf/heaps/restricted_heap.h @@ -15,6 +15,18 @@ struct restricted_buffer {

struct restricted_heap { const char *name; + + const struct restricted_heap_ops *ops; +}; + +struct restricted_heap_ops { + int (*heap_init)(struct restricted_heap *heap); + + int (*memory_alloc)(struct restricted_heap *heap, struct restricted_buffer *buf); + void (*memory_free)(struct restricted_heap *heap, struct restricted_buffer *buf); + + int (*memory_restrict)(struct restricted_heap *heap, struct restricted_buffer *buf); + void (*memory_unrestrict)(struct restricted_heap *heap, struct restricted_buffer *buf); };

int restricted_heap_add(struct restricted_heap *rstrd_heap);

Add a Mediatek restricted heap which uses TEE service call to restrict buffer. Currently this restricted heap is NULL, Prepare for the later patch. Mainly there are two changes: a) Add a heap_init ops since TEE probe late than restricted heap, thus initialize the heap when we require the buffer the first time. b) Add a priv_data for each heap, like the special data used by MTK (such as "TEE session") can be placed in priv_data.

Currently our heap depends on CMA which could only be bool, thus depend on "TEE=y".

Signed-off-by: Yong Wu yong.wu@mediatek.com --- drivers/dma-buf/heaps/Kconfig | 7 ++ drivers/dma-buf/heaps/Makefile | 1 + drivers/dma-buf/heaps/restricted_heap.c | 11 ++ drivers/dma-buf/heaps/restricted_heap.h | 2 + drivers/dma-buf/heaps/restricted_heap_mtk.c | 113 ++++++++++++++++++++ 5 files changed, 134 insertions(+) create mode 100644 drivers/dma-buf/heaps/restricted_heap_mtk.c

diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig index e54506f480ea..84f748fb2856 100644 --- a/drivers/dma-buf/heaps/Kconfig +++ b/drivers/dma-buf/heaps/Kconfig @@ -21,3 +21,10 @@ config DMABUF_HEAPS_RESTRICTED heap is to manage buffers that are inaccessible to the kernel and user space. There may be several ways to restrict it, for example it may be encrypted or protected by a TEE or hypervisor. If in doubt, say N. + +config DMABUF_HEAPS_RESTRICTED_MTK + bool "MediaTek DMA-BUF Restricted Heap" + depends on DMABUF_HEAPS_RESTRICTED && TEE=y + help + Enable restricted dma-buf heaps for MediaTek platform. This heap is backed by + TEE client interfaces. If in doubt, say N. diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile index a2437c1817e2..0028aa9d875f 100644 --- a/drivers/dma-buf/heaps/Makefile +++ b/drivers/dma-buf/heaps/Makefile @@ -1,4 +1,5 @@ # SPDX-License-Identifier: GPL-2.0 obj-$(CONFIG_DMABUF_HEAPS_CMA) += cma_heap.o obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED) += restricted_heap.o +obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED_MTK) += restricted_heap_mtk.o obj-$(CONFIG_DMABUF_HEAPS_SYSTEM) += system_heap.o diff --git a/drivers/dma-buf/heaps/restricted_heap.c b/drivers/dma-buf/heaps/restricted_heap.c index ec4c63d2112d..4e9869ab4a85 100644 --- a/drivers/dma-buf/heaps/restricted_heap.c +++ b/drivers/dma-buf/heaps/restricted_heap.c @@ -152,11 +152,22 @@ restricted_heap_allocate(struct dma_heap *heap, unsigned long size, unsigned long fd_flags, unsigned long heap_flags) { struct restricted_heap *restricted_heap = dma_heap_get_drvdata(heap); + const struct restricted_heap_ops *ops = restricted_heap->ops; struct restricted_buffer *restricted_buf; DEFINE_DMA_BUF_EXPORT_INFO(exp_info); struct dma_buf *dmabuf; int ret;

+ /* + * In some implements, TEE is required to protect buffer. However TEE probe + * may be late, Thus heap_init is performed when the first buffer is requested. + */ + if (ops->heap_init) { + ret = ops->heap_init(restricted_heap); + if (ret) + return ERR_PTR(ret); + } + restricted_buf = kzalloc(sizeof(*restricted_buf), GFP_KERNEL); if (!restricted_buf) return ERR_PTR(-ENOMEM); diff --git a/drivers/dma-buf/heaps/restricted_heap.h b/drivers/dma-buf/heaps/restricted_heap.h index ddeaf9805708..cf5865f829fc 100644 --- a/drivers/dma-buf/heaps/restricted_heap.h +++ b/drivers/dma-buf/heaps/restricted_heap.h @@ -17,6 +17,8 @@ struct restricted_heap { const char *name;

const struct restricted_heap_ops *ops; + + void *priv_data; };

struct restricted_heap_ops { diff --git a/drivers/dma-buf/heaps/restricted_heap_mtk.c b/drivers/dma-buf/heaps/restricted_heap_mtk.c new file mode 100644 index 000000000000..a5f5365059cd --- /dev/null +++ b/drivers/dma-buf/heaps/restricted_heap_mtk.c @@ -0,0 +1,113 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * DMABUF restricted heap exporter for MediaTek + * + * Copyright (C) 2024 MediaTek Inc. + */ +#include <linux/dma-buf.h> +#include <linux/err.h> +#include <linux/module.h> +#include <linux/slab.h> +#include <linux/tee_drv.h> +#include <linux/uuid.h> + +#include "restricted_heap.h" + +#define TZ_TA_MEM_UUID_MTK "4477588a-8476-11e2-ad15-e41f1390d676" + +#define TEE_PARAM_NUM 4 + +enum mtk_secure_mem_type { + /* + * MediaTek static chunk memory carved out for TrustZone. The memory + * management is inside the TEE. + */ + MTK_SECURE_MEMORY_TYPE_CM_TZ = 1, +}; + +struct mtk_restricted_heap_data { + struct tee_context *tee_ctx; + u32 tee_session; + + const enum mtk_secure_mem_type mem_type; + +}; + +static int mtk_tee_ctx_match(struct tee_ioctl_version_data *ver, const void *data) +{ + return ver->impl_id == TEE_IMPL_ID_OPTEE; +} + +static int mtk_tee_session_init(struct mtk_restricted_heap_data *data) +{ + struct tee_param t_param[TEE_PARAM_NUM] = {0}; + struct tee_ioctl_open_session_arg arg = {0}; + uuid_t ta_mem_uuid; + int ret; + + data->tee_ctx = tee_client_open_context(NULL, mtk_tee_ctx_match, NULL, NULL); + if (IS_ERR(data->tee_ctx)) { + pr_err_once("%s: open context failed, ret=%ld\n", __func__, + PTR_ERR(data->tee_ctx)); + return -ENODEV; + } + + arg.num_params = TEE_PARAM_NUM; + arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC; + ret = uuid_parse(TZ_TA_MEM_UUID_MTK, &ta_mem_uuid); + if (ret) + goto close_context; + memcpy(&arg.uuid, &ta_mem_uuid.b, sizeof(ta_mem_uuid)); + + ret = tee_client_open_session(data->tee_ctx, &arg, t_param); + if (ret < 0 || arg.ret) { + pr_err_once("%s: open session failed, ret=%d:%d\n", + __func__, ret, arg.ret); + ret = -EINVAL; + goto close_context; + } + data->tee_session = arg.session; + return 0; + +close_context: + tee_client_close_context(data->tee_ctx); + return ret; +} + +static int mtk_restricted_heap_init(struct restricted_heap *heap) +{ + struct mtk_restricted_heap_data *data = heap->priv_data; + + if (!data->tee_ctx) + return mtk_tee_session_init(data); + return 0; +} + +static const struct restricted_heap_ops mtk_restricted_heap_ops = { + .heap_init = mtk_restricted_heap_init, +}; + +static struct mtk_restricted_heap_data mtk_restricted_heap_data = { + .mem_type = MTK_SECURE_MEMORY_TYPE_CM_TZ, +}; + +static struct restricted_heap mtk_restricted_heaps[] = { + { + .name = "restricted_mtk_cm", + .ops = &mtk_restricted_heap_ops, + .priv_data = &mtk_restricted_heap_data, + }, +}; + +static int mtk_restricted_heap_initialize(void) +{ + struct restricted_heap *rstrd_heap = mtk_restricted_heaps; + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(mtk_restricted_heaps); i++, rstrd_heap++) + restricted_heap_add(rstrd_heap); + return 0; +} +module_init(mtk_restricted_heap_initialize); +MODULE_DESCRIPTION("MediaTek Restricted Heap Driver"); +MODULE_LICENSE("GPL");

Create a new MediaTek CMA heap from the CMA reserved buffer.

In this heap, When the first allocating buffer, use cma_alloc to prepare whole the CMA range, then send its range to TEE to protect and manage. For the later allocating, we just adds the cma_used_size.

When SVP done, cma_release will release the buffer, then kernel may reuse it.

For the "CMA" restricted heap, "struct cma *cma" is a common property, not just for MediaTek, so put it into "struct restricted_heap" instead of our private data.

Signed-off-by: Yong Wu yong.wu@mediatek.com --- drivers/dma-buf/heaps/Kconfig | 2 +- drivers/dma-buf/heaps/restricted_heap.h | 4 + drivers/dma-buf/heaps/restricted_heap_mtk.c | 120 +++++++++++++++++++- 3 files changed, 123 insertions(+), 3 deletions(-)

diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig index 84f748fb2856..58903bc62ac8 100644 --- a/drivers/dma-buf/heaps/Kconfig +++ b/drivers/dma-buf/heaps/Kconfig @@ -24,7 +24,7 @@ config DMABUF_HEAPS_RESTRICTED

config DMABUF_HEAPS_RESTRICTED_MTK bool "MediaTek DMA-BUF Restricted Heap" - depends on DMABUF_HEAPS_RESTRICTED && TEE=y + depends on DMABUF_HEAPS_RESTRICTED && DMA_CMA && TEE=y help Enable restricted dma-buf heaps for MediaTek platform. This heap is backed by TEE client interfaces. If in doubt, say N. diff --git a/drivers/dma-buf/heaps/restricted_heap.h b/drivers/dma-buf/heaps/restricted_heap.h index 6c93f6d257dc..8d17c39b65de 100644 --- a/drivers/dma-buf/heaps/restricted_heap.h +++ b/drivers/dma-buf/heaps/restricted_heap.h @@ -21,6 +21,10 @@ struct restricted_heap {

const struct restricted_heap_ops *ops;

+ struct cma *cma; + unsigned long cma_paddr; + unsigned long cma_size; + void *priv_data; };

diff --git a/drivers/dma-buf/heaps/restricted_heap_mtk.c b/drivers/dma-buf/heaps/restricted_heap_mtk.c index 902add95bb7e..0180d04076e2 100644 --- a/drivers/dma-buf/heaps/restricted_heap_mtk.c +++ b/drivers/dma-buf/heaps/restricted_heap_mtk.c @@ -4,9 +4,11 @@ * * Copyright (C) 2024 MediaTek Inc. */ +#include <linux/cma.h> #include <linux/dma-buf.h> #include <linux/err.h> #include <linux/module.h> +#include <linux/of_reserved_mem.h> #include <linux/slab.h> #include <linux/tee_drv.h> #include <linux/uuid.h> @@ -23,6 +25,13 @@ enum mtk_secure_mem_type { * management is inside the TEE. */ MTK_SECURE_MEMORY_TYPE_CM_TZ = 1, + /* + * MediaTek dynamic chunk memory carved out from CMA. + * In normal case, the CMA could be used in kernel; When SVP start, we will + * allocate whole this CMA and pass whole the CMA PA and size into TEE to + * protect it, then the detail memory management also is inside the TEE. + */ + MTK_SECURE_MEMORY_TYPE_CM_CMA = 2, };

enum mtk_secure_buffer_tee_cmd { @@ -32,6 +41,8 @@ enum mtk_secure_buffer_tee_cmd { * [in] value[0].a: The buffer size. * value[0].b: alignment. * [in] value[1].a: enum mtk_secure_mem_type. + * [in] value[2].a: pa base in cma case. + * value[2].b: The buffer size in cma case. * [out] value[3].a: The secure handle. */ MTK_TZCMD_SECMEM_ZALLOC = 0x10000, /* MTK TEE Command ID Base */ @@ -52,6 +63,9 @@ struct mtk_restricted_heap_data {

const enum mtk_secure_mem_type mem_type;

+ struct page *cma_page; + unsigned long cma_used_size; + struct mutex lock; /* lock for cma_used_size */ };

static int mtk_tee_ctx_match(struct tee_ioctl_version_data *ver, const void *data) @@ -125,6 +139,10 @@ static int mtk_tee_restrict_memory(struct restricted_heap *heap, struct restrict params[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT; params[1].u.value.a = data->mem_type; params[2].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT; + if (heap->cma && data->mem_type == MTK_SECURE_MEMORY_TYPE_CM_CMA) { + params[2].u.value.a = heap->cma_paddr; + params[2].u.value.b = heap->cma_size; + } params[3].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT; ret = mtk_tee_service_call(data->tee_ctx, data->tee_session, MTK_TZCMD_SECMEM_ZALLOC, params); @@ -163,6 +181,48 @@ mtk_restricted_memory_free(struct restricted_heap *heap, struct restricted_buffe { }

+static int mtk_restricted_memory_cma_allocate(struct restricted_heap *heap, + struct restricted_buffer *buf) +{ + struct mtk_restricted_heap_data *data = heap->priv_data; + int ret = 0; + /* + * Allocate CMA only when allocating buffer for the first time, and just + * increase cma_used_size at the other time, Actually the memory + * allocating is within the TEE. + */ + mutex_lock(&data->lock); + if (!data->cma_used_size) { + data->cma_page = cma_alloc(heap->cma, heap->cma_size >> PAGE_SHIFT, + get_order(PAGE_SIZE), false); + if (!data->cma_page) { + ret = -ENOMEM; + goto out_unlock; + } + } else if (data->cma_used_size + buf->size > heap->cma_size) { + ret = -EINVAL; + goto out_unlock; + } + data->cma_used_size += buf->size; + +out_unlock: + mutex_unlock(&data->lock); + return ret; +} + +static void mtk_restricted_memory_cma_free(struct restricted_heap *heap, + struct restricted_buffer *buf) +{ + struct mtk_restricted_heap_data *data = heap->priv_data; + + mutex_lock(&data->lock); + data->cma_used_size -= buf->size; + if (!data->cma_used_size) + cma_release(heap->cma, data->cma_page, + heap->cma_size >> PAGE_SHIFT); + mutex_unlock(&data->lock); +} + static int mtk_restricted_heap_init(struct restricted_heap *heap) { struct mtk_restricted_heap_data *data = heap->priv_data; @@ -184,21 +244,77 @@ static struct mtk_restricted_heap_data mtk_restricted_heap_data = { .mem_type = MTK_SECURE_MEMORY_TYPE_CM_TZ, };

+static const struct restricted_heap_ops mtk_restricted_heap_ops_cma = { + .heap_init = mtk_restricted_heap_init, + .memory_alloc = mtk_restricted_memory_cma_allocate, + .memory_free = mtk_restricted_memory_cma_free, + .memory_restrict = mtk_tee_restrict_memory, + .memory_unrestrict = mtk_tee_unrestrict_memory, +}; + +static struct mtk_restricted_heap_data mtk_restricted_heap_data_cma = { + .mem_type = MTK_SECURE_MEMORY_TYPE_CM_CMA, +}; + static struct restricted_heap mtk_restricted_heaps[] = { { .name = "restricted_mtk_cm", .ops = &mtk_restricted_heap_ops, .priv_data = &mtk_restricted_heap_data, }, + { + .name = "restricted_mtk_cma", + .ops = &mtk_restricted_heap_ops_cma, + .priv_data = &mtk_restricted_heap_data_cma, + }, };

+static int __init mtk_restricted_cma_init(struct reserved_mem *rmem) +{ + struct restricted_heap *rstrd_heap = mtk_restricted_heaps, *rstrd_heap_cma = NULL; + struct mtk_restricted_heap_data *data; + struct cma *cma; + int ret, i; + + for (i = 0; i < ARRAY_SIZE(mtk_restricted_heaps); i++, rstrd_heap++) { + data = rstrd_heap->priv_data; + if (data->mem_type == MTK_SECURE_MEMORY_TYPE_CM_CMA) { + rstrd_heap_cma = rstrd_heap; + break; + } + } + if (!rstrd_heap_cma) + return -EINVAL; + + ret = cma_init_reserved_mem(rmem->base, rmem->size, 0, rmem->name, + &cma); + if (ret) { + pr_err("%s: %s set up CMA fail\n", __func__, rmem->name); + return ret; + } + + rstrd_heap_cma->cma = cma; + rstrd_heap_cma->cma_paddr = rmem->base; + rstrd_heap_cma->cma_size = rmem->size; + return 0; +} + +RESERVEDMEM_OF_DECLARE(restricted_cma, "mediatek,dynamic-restricted-region", + mtk_restricted_cma_init); + static int mtk_restricted_heap_initialize(void) { struct restricted_heap *rstrd_heap = mtk_restricted_heaps; + struct mtk_restricted_heap_data *data; unsigned int i;

- for (i = 0; i < ARRAY_SIZE(mtk_restricted_heaps); i++, rstrd_heap++) - restricted_heap_add(rstrd_heap); + for (i = 0; i < ARRAY_SIZE(mtk_restricted_heaps); i++, rstrd_heap++) { + data = rstrd_heap->priv_data; + if (data->mem_type == MTK_SECURE_MEMORY_TYPE_CM_CMA && !rstrd_heap->cma) + continue; + if (!restricted_heap_add(rstrd_heap)) + mutex_init(&data->lock); + } return 0; } module_init(mtk_restricted_heap_initialize);

On Fri, Jan 12, 2024 at 10:41:14AM +0100, Daniel Vetter wrote:

On Fri, Jan 12, 2024 at 05:20:11PM +0800, Yong Wu wrote:

...

Add the dma_ops for this restricted heap. For restricted buffer, cache_ops/mmap are not allowed, thus return EPERM for them.

Signed-off-by: Yong Wu yong.wu@mediatek.com

drivers/dma-buf/heaps/restricted_heap.c | 103 ++++++++++++++++++++++++ 1 file changed, 103 insertions(+)

diff --git a/drivers/dma-buf/heaps/restricted_heap.c b/drivers/dma-buf/heaps/restricted_heap.c index 8c266a0f6192..ec4c63d2112d 100644 --- a/drivers/dma-buf/heaps/restricted_heap.c +++ b/drivers/dma-buf/heaps/restricted_heap.c @@ -12,6 +12,10 @@ #include "restricted_heap.h" +struct restricted_heap_attachment {

• struct sg_table *table;

+};

static int restricted_heap_memory_allocate(struct restricted_heap *heap, struct restricted_buffer *buf) { @@ -45,6 +49,104 @@ restricted_heap_memory_free(struct restricted_heap *heap, struct restricted_buff ops->memory_free(heap, buf); } +static int restricted_heap_attach(struct dma_buf *dmabuf, struct dma_buf_attachment *attachment) +{

• struct restricted_buffer *restricted_buf = dmabuf->priv;
• struct restricted_heap_attachment *a;
• struct sg_table *table;
• int ret;
• a = kzalloc(sizeof(*a), GFP_KERNEL);
• if (!a)

• return -ENOMEM;
  

• table = kzalloc(sizeof(*table), GFP_KERNEL);
• if (!table) {

• ret = -ENOMEM;
  

• goto err_free_attach;
  

• }
• ret = sg_alloc_table(table, 1, GFP_KERNEL);
• if (ret)

• goto err_free_sgt;
  

• sg_set_page(table->sgl, NULL, restricted_buf->size, 0);

So this is definitely broken and violating the dma-buf api rules. You cannot let attach succed and supply a dummy/invalid sg table.

Two options:

• Reject ->attach for all this buffers with -EBUSY and provide instead a private api for these secure buffers, similar to how virtio_dma_buf has private virto-specific apis. This interface would need to be standardized across all arm TEE users, so that we don't have a disastrous proliferation of apis.

• Allow ->attach, but _only_ for drivers/devices which can access the secure buffer correctly, and only if you can put the right secure buffer address into the sg table directly. If dma to a secure buffer for a given struct device * will not work correctly (i.e. without data corruption), you _must_ reject the attach attempt with -EBUSY.

The 2nd approach would be my preferred one, if it's technically possible.

Also my understanding is that arm TEE is standardized, so I think we'll at least want some acks from other soc people whether this will work for them too.

Finally the usual drill:

• this also needs the driver side support, if there's any changes needed. Just the new heap isn't enough.

Ok I quickly scrolled through your drm patches and that confirms that the current dma-buf interface you're implementing is just completely breaking the api. And you need to paper over that will all kinds of very icky special-casing.

So definitely need to rethink the overall design between dma-buf heaps and drivers here. -Sima

• and for drm you need open userspace for this. Doesn't have to be the full content protection decode pipeline, the drivers in drm that landed secure buffer support thus far enabled it using the EGL_EXT_protected_content extension using gl, which side steps all the complications around content decryption keys and support

Cheers, Sima

...

• a->table = table;
• attachment->priv = a;
• return 0;

+err_free_sgt:

• kfree(table);

+err_free_attach:

• kfree(a);
• return ret;

+}

+static void restricted_heap_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attachment) +{

• struct restricted_heap_attachment *a = attachment->priv;
• sg_free_table(a->table);
• kfree(a->table);
• kfree(a);

+}

+static struct sg_table * +restricted_heap_map_dma_buf(struct dma_buf_attachment *attachment, enum dma_data_direction direct) +{

• struct restricted_heap_attachment *a = attachment->priv;
• struct sg_table *table = a->table;
• return table;

+}

+static void +restricted_heap_unmap_dma_buf(struct dma_buf_attachment *attachment, struct sg_table *table,

• 	      enum dma_data_direction direction)
  

+{

• struct restricted_heap_attachment *a = attachment->priv;
• WARN_ON(a->table != table);

+}

+static int +restricted_heap_dma_buf_begin_cpu_access(struct dma_buf *dmabuf, enum dma_data_direction direction) +{

• return -EPERM;

+}

+static int +restricted_heap_dma_buf_end_cpu_access(struct dma_buf *dmabuf, enum dma_data_direction direction) +{

• return -EPERM;

+}

+static int restricted_heap_dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma) +{

• return -EPERM;

+}

+static void restricted_heap_free(struct dma_buf *dmabuf) +{

• struct restricted_buffer *restricted_buf = dmabuf->priv;
• struct restricted_heap *heap = dma_heap_get_drvdata(restricted_buf->heap);
• restricted_heap_memory_free(heap, restricted_buf);
• kfree(restricted_buf);

+}

+static const struct dma_buf_ops restricted_heap_buf_ops = {

• .attach = restricted_heap_attach,
• .detach = restricted_heap_detach,
• .map_dma_buf = restricted_heap_map_dma_buf,
• .unmap_dma_buf = restricted_heap_unmap_dma_buf,
• .begin_cpu_access = restricted_heap_dma_buf_begin_cpu_access,
• .end_cpu_access = restricted_heap_dma_buf_end_cpu_access,
• .mmap = restricted_heap_dma_buf_mmap,
• .release = restricted_heap_free,

+};

static struct dma_buf * restricted_heap_allocate(struct dma_heap *heap, unsigned long size, unsigned long fd_flags, unsigned long heap_flags) @@ -66,6 +168,7 @@ restricted_heap_allocate(struct dma_heap *heap, unsigned long size, if (ret) goto err_free_buf; exp_info.exp_name = dma_heap_get_name(heap);

• exp_info.ops = &restricted_heap_buf_ops; exp_info.size = restricted_buf->size; exp_info.flags = fd_flags; exp_info.priv = restricted_buf;

-- 2.25.1

-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

On Fri, Jan 12, 2024 at 1:21 AM Yong Wu yong.wu@mediatek.com wrote:

Add "struct restricted_heap_ops". For the restricted memory, totally there are two steps: a) memory_alloc: Allocate the buffer in kernel; b) memory_restrict: Restrict/Protect/Secure that buffer. The memory_alloc is mandatory while memory_restrict is optinal since it may be part of memory_alloc.

Signed-off-by: Yong Wu yong.wu@mediatek.com

drivers/dma-buf/heaps/restricted_heap.c | 41 ++++++++++++++++++++++++- drivers/dma-buf/heaps/restricted_heap.h | 12 ++++++++ 2 files changed, 52 insertions(+), 1 deletion(-)

Thanks for sending this out! A thought below.

diff --git a/drivers/dma-buf/heaps/restricted_heap.h b/drivers/dma-buf/heaps/restricted_heap.h index 443028f6ba3b..ddeaf9805708 100644 --- a/drivers/dma-buf/heaps/restricted_heap.h +++ b/drivers/dma-buf/heaps/restricted_heap.h @@ -15,6 +15,18 @@ struct restricted_buffer {

struct restricted_heap { const char *name;

•   const struct restricted_heap_ops *ops;
  

+};

+struct restricted_heap_ops {

•   int     (*heap_init)(struct restricted_heap *heap);
  

•   int     (*memory_alloc)(struct restricted_heap *heap, struct restricted_buffer *buf);
  

•   void    (*memory_free)(struct restricted_heap *heap, struct restricted_buffer *buf);
  

•   int     (*memory_restrict)(struct restricted_heap *heap, struct restricted_buffer *buf);
  

•   void    (*memory_unrestrict)(struct restricted_heap *heap, struct restricted_buffer *buf);
  

};

int restricted_heap_add(struct restricted_heap *rstrd_heap);

So, I'm a little worried here, because you're basically turning the restricted_heap dma-buf heap driver into a framework itself. Where this patch is creating a subdriver framework.

Part of my hesitancy, is you're introducing this under the dma-buf heaps. For things like CMA, that's more of a core subsystem that has multiple users, and exporting cma buffers via dmabuf heaps is just an additional interface. What I like about that is the core kernel has to define the semantics for the memory type and then the dmabuf heap is just exporting that well understood type of buffer.

But with these restricted buffers, I'm not sure there's yet a well understood set of semantics nor a central abstraction for that which other drivers use directly.

I know part of this effort here is to start to centralize all these vendor specific restricted buffer implementations, which I think is great, but I just worry that in doing it in the dmabuf heap interface, it is a bit too user-facing. The likelihood of encoding a particular semantic to the singular "restricted_heap" name seems high.

Similarly we might run into systems with multiple types of restricted buffers (imagine a discrete gpu having one type along with TEE protected buffers also being used on the same system).

So the one question I have: Why not just have a mediatek specific restricted_heap dmabuf heap driver? Since there's already been some talk of slight semantic differences in various restricted buffer implementations, should we just start with separately named dmabuf heaps for each? Maybe consolidating to a common name as more drivers arrive and we gain a better understanding of the variations of semantics folks are using?

thanks -john

On Fri, Jan 12, 2024 at 3:27 PM Jeffrey Kardatzke jkardatzke@google.com wrote:

On Fri, Jan 12, 2024 at 2:52 PM John Stultz jstultz@google.com wrote:

...

On Fri, Jan 12, 2024 at 1:21 AM Yong Wu yong.wu@mediatek.com wrote:

...

diff --git a/drivers/dma-buf/heaps/restricted_heap.h b/drivers/dma-buf/heaps/restricted_heap.h index 443028f6ba3b..ddeaf9805708 100644 --- a/drivers/dma-buf/heaps/restricted_heap.h +++ b/drivers/dma-buf/heaps/restricted_heap.h @@ -15,6 +15,18 @@ struct restricted_buffer {

struct restricted_heap { const char *name;

•   const struct restricted_heap_ops *ops;
  

+};

+struct restricted_heap_ops {

•   int     (*heap_init)(struct restricted_heap *heap);
  

•   int     (*memory_alloc)(struct restricted_heap *heap, struct restricted_buffer *buf);
  

•   void    (*memory_free)(struct restricted_heap *heap, struct restricted_buffer *buf);
  

•   int     (*memory_restrict)(struct restricted_heap *heap, struct restricted_buffer *buf);
  

•   void    (*memory_unrestrict)(struct restricted_heap *heap, struct restricted_buffer *buf);
  

};

int restricted_heap_add(struct restricted_heap *rstrd_heap);

So, I'm a little worried here, because you're basically turning the restricted_heap dma-buf heap driver into a framework itself. Where this patch is creating a subdriver framework.

Part of my hesitancy, is you're introducing this under the dma-buf heaps. For things like CMA, that's more of a core subsystem that has multiple users, and exporting cma buffers via dmabuf heaps is just an additional interface. What I like about that is the core kernel has to define the semantics for the memory type and then the dmabuf heap is just exporting that well understood type of buffer.

But with these restricted buffers, I'm not sure there's yet a well understood set of semantics nor a central abstraction for that which other drivers use directly.

I know part of this effort here is to start to centralize all these vendor specific restricted buffer implementations, which I think is great, but I just worry that in doing it in the dmabuf heap interface, it is a bit too user-facing. The likelihood of encoding a particular semantic to the singular "restricted_heap" name seems high.

In patch #5 it has the actual driver implementation for the MTK heap that includes the heap name of "restricted_mtk_cm", so there shouldn't be a heap named "restricted_heap" that's actually getting exposed. The

Ah, I appreciate that clarification! Indeed, you're right the name is passed through. Apologies for missing that detail.

restricted_heap code is a framework, and not a driver itself. Unless I'm missing something in this patchset...but that's the way it's *supposed* to be.

So I guess I'm not sure I understand the benefit of the extra indirection. What then does the restricted_heap.c logic itself provide? The dmabuf heaps framework already provides a way to add heap implementations.

thanks -john

On Fri, Jan 12, 2024 at 05:20:10PM +0800, Yong Wu wrote:

Add "struct restricted_heap_ops". For the restricted memory, totally there are two steps: a) memory_alloc: Allocate the buffer in kernel; b) memory_restrict: Restrict/Protect/Secure that buffer. The memory_alloc is mandatory while memory_restrict is optinal since it may

s/optinal/optional/

be part of memory_alloc.

Signed-off-by: Yong Wu yong.wu@mediatek.com

drivers/dma-buf/heaps/restricted_heap.c | 41 ++++++++++++++++++++++++- drivers/dma-buf/heaps/restricted_heap.h | 12 ++++++++ 2 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/dma-buf/heaps/restricted_heap.c b/drivers/dma-buf/heaps/restricted_heap.c index fd7c82abd42e..8c266a0f6192 100644 --- a/drivers/dma-buf/heaps/restricted_heap.c +++ b/drivers/dma-buf/heaps/restricted_heap.c @@ -12,10 +12,44 @@ #include "restricted_heap.h" +static int +restricted_heap_memory_allocate(struct restricted_heap *heap, struct restricted_buffer *buf) +{

• const struct restricted_heap_ops *ops = heap->ops;
• int ret;
• ret = ops->memory_alloc(heap, buf);
• if (ret)

• return ret;
  

• if (ops->memory_restrict) {

• ret = ops->memory_restrict(heap, buf);
  

• if (ret)
  

• 	goto memory_free;
  

• }
• return 0;

+memory_free:

• ops->memory_free(heap, buf);
• return ret;

+}

+static void +restricted_heap_memory_free(struct restricted_heap *heap, struct restricted_buffer *buf) +{

• const struct restricted_heap_ops *ops = heap->ops;
• if (ops->memory_unrestrict)

• ops->memory_unrestrict(heap, buf);
  

• ops->memory_free(heap, buf);

+}

static struct dma_buf * restricted_heap_allocate(struct dma_heap *heap, unsigned long size, unsigned long fd_flags, unsigned long heap_flags) {

• struct restricted_heap *restricted_heap = dma_heap_get_drvdata(heap); struct restricted_buffer *restricted_buf; DEFINE_DMA_BUF_EXPORT_INFO(exp_info); struct dma_buf *dmabuf;

@@ -28,6 +62,9 @@ restricted_heap_allocate(struct dma_heap *heap, unsigned long size, restricted_buf->size = ALIGN(size, PAGE_SIZE); restricted_buf->heap = heap;

• ret = restricted_heap_memory_allocate(restricted_heap, restricted_buf);

Can we guarantee that "restricted_heap" here isn't NULL (i.e., heap->priv). If not perhaps we should consider adding a check for NULL in the restricted_heap_memory_allocate() function?

• if (ret)

• goto err_free_buf;
  

  exp_info.exp_name = dma_heap_get_name(heap); exp_info.size = restricted_buf->size; exp_info.flags = fd_flags;

@@ -36,11 +73,13 @@ restricted_heap_allocate(struct dma_heap *heap, unsigned long size, dmabuf = dma_buf_export(&exp_info); if (IS_ERR(dmabuf)) { ret = PTR_ERR(dmabuf);

• goto err_free_buf;
  

• goto err_free_restricted_mem;
  

  }

return dmabuf; +err_free_restricted_mem:

• restricted_heap_memory_free(restricted_heap, restricted_buf);

err_free_buf: kfree(restricted_buf); return ERR_PTR(ret); diff --git a/drivers/dma-buf/heaps/restricted_heap.h b/drivers/dma-buf/heaps/restricted_heap.h index 443028f6ba3b..ddeaf9805708 100644 --- a/drivers/dma-buf/heaps/restricted_heap.h +++ b/drivers/dma-buf/heaps/restricted_heap.h @@ -15,6 +15,18 @@ struct restricted_buffer { struct restricted_heap { const char *name;

• const struct restricted_heap_ops *ops;

+};

+struct restricted_heap_ops {

This have the same name as used for the dma_heap_ops in the file restricted_heap.c, this might be a little bit confusing, or?

• int (*heap_init)(struct restricted_heap *heap);
• int (*memory_alloc)(struct restricted_heap *heap, struct restricted_buffer *buf);
• void (*memory_free)(struct restricted_heap *heap, struct restricted_buffer *buf);
• int (*memory_restrict)(struct restricted_heap *heap, struct restricted_buffer *buf);
• void (*memory_unrestrict)(struct restricted_heap *heap, struct restricted_buffer *buf);

Is the prefix "memory_" superfluous here in these ops?

Also related to a comment on the prior patch. The name here is "heap" for restricted_heap, but below you use rstrd_heap. It's the same struct, so I would advise to use the same name to avoid confusion when reading the code. As mentioned before, I think the name "rheap" would be a good choice.

}; int restricted_heap_add(struct restricted_heap *rstrd_heap); -- 2.25.1

Hi Joakim,

Sorry for reply so late.

On Wed, 2024-01-31 at 14:53 +0100, Joakim Bech wrote:

External email : Please do not click links or open attachments until you have verified the sender or the content. On Fri, Jan 12, 2024 at 05:20:10PM +0800, Yong Wu wrote:

...

Add "struct restricted_heap_ops". For the restricted memory,

totally there

...

are two steps: a) memory_alloc: Allocate the buffer in kernel; b) memory_restrict: Restrict/Protect/Secure that buffer. The memory_alloc is mandatory while memory_restrict is optinal

since it may

...

s/optinal/optional/

Will Fix.

...

be part of memory_alloc.

Signed-off-by: Yong Wu yong.wu@mediatek.com

drivers/dma-buf/heaps/restricted_heap.c | 41

++++++++++++++++++++++++-

...

drivers/dma-buf/heaps/restricted_heap.h | 12 ++++++++ 2 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/dma-buf/heaps/restricted_heap.c b/drivers/dma-

buf/heaps/restricted_heap.c

...

index fd7c82abd42e..8c266a0f6192 100644 --- a/drivers/dma-buf/heaps/restricted_heap.c +++ b/drivers/dma-buf/heaps/restricted_heap.c @@ -12,10 +12,44 @@ #include "restricted_heap.h" +static int +restricted_heap_memory_allocate(struct restricted_heap *heap,

struct restricted_buffer *buf)

...

+{ +const struct restricted_heap_ops *ops = heap->ops; +int ret;

+ret = ops->memory_alloc(heap, buf); +if (ret) +return ret;

+if (ops->memory_restrict) { +ret = ops->memory_restrict(heap, buf); +if (ret) +goto memory_free; +} +return 0;

+memory_free: +ops->memory_free(heap, buf); +return ret; +}

+static void +restricted_heap_memory_free(struct restricted_heap *heap, struct

restricted_buffer *buf)

...

+{ +const struct restricted_heap_ops *ops = heap->ops;

+if (ops->memory_unrestrict) +ops->memory_unrestrict(heap, buf);

+ops->memory_free(heap, buf); +}

static struct dma_buf * restricted_heap_allocate(struct dma_heap *heap, unsigned long

size,

...

unsigned long fd_flags, unsigned long heap_flags) { +struct restricted_heap *restricted_heap =

dma_heap_get_drvdata(heap);

...

struct restricted_buffer *restricted_buf; DEFINE_DMA_BUF_EXPORT_INFO(exp_info); struct dma_buf *dmabuf; @@ -28,6 +62,9 @@ restricted_heap_allocate(struct dma_heap *heap,

unsigned long size,

...

restricted_buf->size = ALIGN(size, PAGE_SIZE); restricted_buf->heap = heap; +ret = restricted_heap_memory_allocate(restricted_heap,

restricted_buf);

...

Can we guarantee that "restricted_heap" here isn't NULL (i.e., heap-

...

priv). If

not perhaps we should consider adding a check for NULL in the restricted_heap_memory_allocate() function?

heap->priv always is set when dma_heap_add is called. Suppose heap-

priv is NULL, the KE would have already been occurred in

restricted_heap_add. I don't think it can be NULL. is it right?

...

+if (ret) +goto err_free_buf; exp_info.exp_name = dma_heap_get_name(heap); exp_info.size = restricted_buf->size; exp_info.flags = fd_flags; @@ -36,11 +73,13 @@ restricted_heap_allocate(struct dma_heap *heap,

unsigned long size,

...

dmabuf = dma_buf_export(&exp_info); if (IS_ERR(dmabuf)) { ret = PTR_ERR(dmabuf); -goto err_free_buf; +goto err_free_restricted_mem; } return dmabuf; +err_free_restricted_mem: +restricted_heap_memory_free(restricted_heap, restricted_buf); err_free_buf: kfree(restricted_buf); return ERR_PTR(ret); diff --git a/drivers/dma-buf/heaps/restricted_heap.h b/drivers/dma-

buf/heaps/restricted_heap.h

...

index 443028f6ba3b..ddeaf9805708 100644 --- a/drivers/dma-buf/heaps/restricted_heap.h +++ b/drivers/dma-buf/heaps/restricted_heap.h @@ -15,6 +15,18 @@ struct restricted_buffer { struct restricted_heap { const char*name;

+const struct restricted_heap_ops *ops; +};

+struct restricted_heap_ops {

This have the same name as used for the dma_heap_ops in the file restricted_heap.c, this might be a little bit confusing, or?

Thanks very much. I really didn't notice this. I will rename it.

...

+int(*heap_init)(struct restricted_heap *heap);

+int(*memory_alloc)(struct restricted_heap *heap, struct

restricted_buffer *buf);

...

+void(*memory_free)(struct restricted_heap *heap, struct

restricted_buffer *buf);

...

+int(*memory_restrict)(struct restricted_heap *heap, struct

restricted_buffer *buf);

...

+void(*memory_unrestrict)(struct restricted_heap *heap, struct

restricted_buffer *buf);

...

Is the prefix "memory_" superfluous here in these ops?

I will remove "memory_". But it looks like the "restrict" is a keyword, I can't use it or it will build fail. Therefore I plan to use alloc/free/restrict_buf/unrestrict_buf in next version.

Also related to a comment on the prior patch. The name here is "heap" for restricted_heap, but below you use rstrd_heap. It's the same struct, so I would advise to use the same name to avoid confusion when reading the code. As mentioned before, I think the name "rheap" would be a good choice.

I will use "rheap" to replace everywhere.

Thanks.

...

}; int restricted_heap_add(struct restricted_heap *rstrd_heap); -- 2.25.1

-- // Regards Joakim