2023-12-12 02:46:56

by Yong Wu (吴勇)

[permalink] [raw]
Subject: [PATCH v3 0/7] dma-buf: heaps: Add secure heap

This patchset is for secure video playback and enables other potential
uses in the future. The 'secure dma-heap' will be used to allocate dma_buf
objects that reference memory in the secure world that is inaccessible/
unmappable by the non-secure (i.e.kernel/userspace) world. That memory
will be used by the secure world to store secure information (i.e.
decrypted media content). The dma_bufs allocated from the kernel will be
passed to V4L2 for video decoding (as input and output). They will also be
used by the drm system for rendering of the content.

This patchset adds two secure heaps and they will be used v4l2[1] and drm[2].
1) secure_mtk_cm: secure chunk memory for MediaTek SVP (Secure Video Path).
The buffer is reserved for the secure world after bootup and it is used
for vcodec's ES/working buffer;
2) secure_mtk_cma: secure CMA memory for MediaTek SVP. This buffer is
dynamically reserved for the secure world and will be got when we start
playing secure videos, Once the security video playing is complete, the
CMA will be released. This heap is used for the vcodec's frame buffer.

[1] https://lore.kernel.org/linux-mediatek/[email protected]/
[2] https://lore.kernel.org/linux-mediatek/[email protected]/

Change note:
v3: Base on v6.7-rc1.
1) Separate the secure heap into a common file(secure_heap.c) and a mtk special file
(secure_heap_mtk.c), and put all tee related code into our special file.
2) About dt-binding,
a) Add "mediatek," prefix since this is Mediatek TEE firmware definition.
b) Mute dt-binding check waring.
3) Remove the normal CMA heap which is a draft for qcom.

v2: https://lore.kernel.org/linux-mediatek/[email protected]/
1) Move John's patches into the vcodec patchset since they use the new
dma heap interface directly.
https://lore.kernel.org/linux-mediatek/[email protected]/
2) Reword the dt-binding description.
3) Rename the heap name from mtk_svp to secure_mtk_cm.
This means the current vcodec/DRM upstream code doesn't match this.
4) Add a normal CMA heap. currently it should be a draft version.
5) Regarding the UUID, I still use hard code, but put it in a private
data which allow the others could set their own UUID. What's more, UUID
is necessary for the session with TEE. If we don't have it, we can't
communicate with the TEE, including the get_uuid interface, which tries
to make uuid more generic, not working. If there is other way to make
UUID more general, please free to tell me.

v1: https://lore.kernel.org/linux-mediatek/[email protected]/
Base on v6.6-rc1.

Yong Wu (7):
dt-bindings: reserved-memory: Add mediatek,dynamic-secure-region
dma-buf: heaps: Initialize a secure heap
dma-buf: heaps: secure_heap: Add private heap ops
dma-buf: heaps: secure_heap: Add dma_ops
dma-buf: heaps: secure_heap: Add MediaTek secure heap and heap_init
dma-buf: heaps: secure_heap_mtk: Add tee memory service call
dma_buf: heaps: secure_heap_mtk: Add a new CMA heap

.../mediatek,dynamic-secure-region.yaml | 43 +++
drivers/dma-buf/heaps/Kconfig | 13 +
drivers/dma-buf/heaps/Makefile | 2 +
drivers/dma-buf/heaps/secure_heap.c | 234 +++++++++++++
drivers/dma-buf/heaps/secure_heap.h | 43 +++
drivers/dma-buf/heaps/secure_heap_mtk.c | 321 ++++++++++++++++++
6 files changed, 656 insertions(+)
create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,dynamic-secure-region.yaml
create mode 100644 drivers/dma-buf/heaps/secure_heap.c
create mode 100644 drivers/dma-buf/heaps/secure_heap.h
create mode 100644 drivers/dma-buf/heaps/secure_heap_mtk.c

--
2.18.0



2023-12-12 02:47:02

by Yong Wu (吴勇)

[permalink] [raw]
Subject: [PATCH v3 1/7] dt-bindings: reserved-memory: Add mediatek,dynamic-secure-region

Add a binding for describing the dynamic secure 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 <[email protected]>
---
.../mediatek,dynamic-secure-region.yaml | 43 +++++++++++++++++++
1 file changed, 43 insertions(+)
create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,dynamic-secure-region.yaml

diff --git a/Documentation/devicetree/bindings/reserved-memory/mediatek,dynamic-secure-region.yaml b/Documentation/devicetree/bindings/reserved-memory/mediatek,dynamic-secure-region.yaml
new file mode 100644
index 000000000000..4a735aeafc62
--- /dev/null
+++ b/Documentation/devicetree/bindings/reserved-memory/mediatek,dynamic-secure-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-secure-region.yaml#
+$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 <[email protected]>
+
+allOf:
+ - $ref: reserved-memory.yaml
+
+properties:
+ compatible:
+ const: mediatek,dynamic-secure-region
+
+required:
+ - compatible
+ - reg
+ - reusable
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ reserved-memory {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ reserved-memory@80000000 {
+ compatible = "mediatek,dynamic-secure-region";
+ reg = <0x80000000 0x18000000>;
+ reusable;
+ };
+ };
--
2.25.1

2023-12-12 02:47:23

by Yong Wu (吴勇)

[permalink] [raw]
Subject: [PATCH v3 3/7] dma-buf: heaps: secure_heap: Add private heap ops

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

Signed-off-by: Yong Wu <[email protected]>
---
drivers/dma-buf/heaps/secure_heap.c | 39 ++++++++++++++++++++++++++++-
drivers/dma-buf/heaps/secure_heap.h | 11 ++++++++
2 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/drivers/dma-buf/heaps/secure_heap.c b/drivers/dma-buf/heaps/secure_heap.c
index e087da5638aa..925cf8e1c7ce 100644
--- a/drivers/dma-buf/heaps/secure_heap.c
+++ b/drivers/dma-buf/heaps/secure_heap.c
@@ -12,10 +12,42 @@

#include "secure_heap.h"

+static int secure_heap_memory_allocate(struct secure_heap *sec_heap, struct secure_buffer *sec_buf)
+{
+ const struct secure_heap_ops *ops = sec_heap->ops;
+ int ret;
+
+ ret = ops->memory_alloc(sec_heap, sec_buf);
+ if (ret)
+ return ret;
+
+ if (ops->secure_the_memory) {
+ ret = ops->secure_the_memory(sec_heap, sec_buf);
+ if (ret)
+ goto sec_memory_free;
+ }
+ return 0;
+
+sec_memory_free:
+ ops->memory_free(sec_heap, sec_buf);
+ return ret;
+}
+
+static void secure_heap_memory_free(struct secure_heap *sec_heap, struct secure_buffer *sec_buf)
+{
+ const struct secure_heap_ops *ops = sec_heap->ops;
+
+ if (ops->unsecure_the_memory)
+ ops->unsecure_the_memory(sec_heap, sec_buf);
+
+ ops->memory_free(sec_heap, sec_buf);
+}
+
static struct dma_buf *
secure_heap_allocate(struct dma_heap *heap, unsigned long size,
unsigned long fd_flags, unsigned long heap_flags)
{
+ struct secure_heap *sec_heap = dma_heap_get_drvdata(heap);
struct secure_buffer *sec_buf;
DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
struct dma_buf *dmabuf;
@@ -28,6 +60,9 @@ secure_heap_allocate(struct dma_heap *heap, unsigned long size,
sec_buf->size = ALIGN(size, PAGE_SIZE);
sec_buf->heap = heap;

+ ret = secure_heap_memory_allocate(sec_heap, sec_buf);
+ if (ret)
+ goto err_free_buf;
exp_info.exp_name = dma_heap_get_name(heap);
exp_info.size = sec_buf->size;
exp_info.flags = fd_flags;
@@ -36,11 +71,13 @@ secure_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_sec_mem;
}

return dmabuf;

+err_free_sec_mem:
+ secure_heap_memory_free(sec_heap, sec_buf);
err_free_buf:
kfree(sec_buf);
return ERR_PTR(ret);
diff --git a/drivers/dma-buf/heaps/secure_heap.h b/drivers/dma-buf/heaps/secure_heap.h
index 50733dc6d4db..ec5349cd28d0 100644
--- a/drivers/dma-buf/heaps/secure_heap.h
+++ b/drivers/dma-buf/heaps/secure_heap.h
@@ -15,6 +15,17 @@ struct secure_buffer {

struct secure_heap {
const char *name;
+
+ const struct secure_heap_ops *ops;
+};
+
+struct secure_heap_ops {
+ 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);
+
+ /* Protect/unprotect the memory */
+ int (*secure_the_memory)(struct secure_heap *sec_heap, struct secure_buffer *sec_buf);
+ void (*unsecure_the_memory)(struct secure_heap *sec_heap, struct secure_buffer *sec_buf);
};

int secure_heap_add(struct secure_heap *sec_heap);
--
2.25.1

2023-12-12 02:47:30

by Yong Wu (吴勇)

[permalink] [raw]
Subject: [PATCH v3 2/7] dma-buf: heaps: Initialize a secure heap

Initialize a secure heap. Currently just add a null heap, Prepare for
the later patches.

Signed-off-by: Yong Wu <[email protected]>
---
drivers/dma-buf/heaps/Kconfig | 6 +++
drivers/dma-buf/heaps/Makefile | 1 +
drivers/dma-buf/heaps/secure_heap.c | 67 +++++++++++++++++++++++++++++
drivers/dma-buf/heaps/secure_heap.h | 22 ++++++++++
4 files changed, 96 insertions(+)
create mode 100644 drivers/dma-buf/heaps/secure_heap.c
create mode 100644 drivers/dma-buf/heaps/secure_heap.h

diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
index a5eef06c4226..3a9943e94200 100644
--- a/drivers/dma-buf/heaps/Kconfig
+++ b/drivers/dma-buf/heaps/Kconfig
@@ -12,3 +12,9 @@ config DMABUF_HEAPS_CMA
Choose this option to enable dma-buf CMA heap. This heap is backed
by the Contiguous Memory Allocator (CMA). If your system has these
regions, you should say Y here.
+
+config DMABUF_HEAPS_SECURE
+ bool "DMA-BUF Secure Heap"
+ depends on DMABUF_HEAPS
+ help
+ Choose this option to enable dma-buf secure heap. If in doubt, say N.
diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile
index 974467791032..b1ad9d1f2fbe 100644
--- a/drivers/dma-buf/heaps/Makefile
+++ b/drivers/dma-buf/heaps/Makefile
@@ -1,3 +1,4 @@
# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_DMABUF_HEAPS_SECURE) += secure_heap.o
obj-$(CONFIG_DMABUF_HEAPS_SYSTEM) += system_heap.o
obj-$(CONFIG_DMABUF_HEAPS_CMA) += cma_heap.o
diff --git a/drivers/dma-buf/heaps/secure_heap.c b/drivers/dma-buf/heaps/secure_heap.c
new file mode 100644
index 000000000000..e087da5638aa
--- /dev/null
+++ b/drivers/dma-buf/heaps/secure_heap.c
@@ -0,0 +1,67 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * DMABUF secure heap exporter
+ *
+ * Copyright (C) 2023 MediaTek Inc.
+ */
+
+#include <linux/dma-buf.h>
+#include <linux/dma-heap.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+
+#include "secure_heap.h"
+
+static struct dma_buf *
+secure_heap_allocate(struct dma_heap *heap, unsigned long size,
+ unsigned long fd_flags, unsigned long heap_flags)
+{
+ struct secure_buffer *sec_buf;
+ DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
+ struct dma_buf *dmabuf;
+ int ret;
+
+ sec_buf = kzalloc(sizeof(*sec_buf), GFP_KERNEL);
+ if (!sec_buf)
+ return ERR_PTR(-ENOMEM);
+
+ sec_buf->size = ALIGN(size, PAGE_SIZE);
+ sec_buf->heap = heap;
+
+ exp_info.exp_name = dma_heap_get_name(heap);
+ exp_info.size = sec_buf->size;
+ exp_info.flags = fd_flags;
+ exp_info.priv = sec_buf;
+
+ dmabuf = dma_buf_export(&exp_info);
+ if (IS_ERR(dmabuf)) {
+ ret = PTR_ERR(dmabuf);
+ goto err_free_buf;
+ }
+
+ return dmabuf;
+
+err_free_buf:
+ kfree(sec_buf);
+ return ERR_PTR(ret);
+}
+
+static const struct dma_heap_ops sec_heap_ops = {
+ .allocate = secure_heap_allocate,
+};
+
+int secure_heap_add(struct secure_heap *sec_heap)
+{
+ struct dma_heap_export_info exp_info;
+ struct dma_heap *heap;
+
+ exp_info.name = sec_heap->name;
+ exp_info.ops = &sec_heap_ops;
+ exp_info.priv = (void *)sec_heap;
+
+ heap = dma_heap_add(&exp_info);
+ if (IS_ERR(heap))
+ return PTR_ERR(heap);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(secure_heap_add);
diff --git a/drivers/dma-buf/heaps/secure_heap.h b/drivers/dma-buf/heaps/secure_heap.h
new file mode 100644
index 000000000000..50733dc6d4db
--- /dev/null
+++ b/drivers/dma-buf/heaps/secure_heap.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Secure heap Header.
+ *
+ * Copyright (C) 2023 MediaTek, Inc.
+ */
+
+#ifndef _DMABUF_SECURE_HEAP_H_
+#define _DMABUF_SECURE_HEAP_H_
+
+struct secure_buffer {
+ struct dma_heap *heap;
+ size_t size;
+};
+
+struct secure_heap {
+ const char *name;
+};
+
+int secure_heap_add(struct secure_heap *sec_heap);
+
+#endif
--
2.25.1

2023-12-12 02:47:43

by Yong Wu (吴勇)

[permalink] [raw]
Subject: [PATCH v3 4/7] dma-buf: heaps: secure_heap: Add dma_ops

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

Signed-off-by: Yong Wu <[email protected]>
---
drivers/dma-buf/heaps/secure_heap.c | 103 ++++++++++++++++++++++++++++
1 file changed, 103 insertions(+)

diff --git a/drivers/dma-buf/heaps/secure_heap.c b/drivers/dma-buf/heaps/secure_heap.c
index 925cf8e1c7ce..7cb4db3e55c2 100644
--- a/drivers/dma-buf/heaps/secure_heap.c
+++ b/drivers/dma-buf/heaps/secure_heap.c
@@ -12,6 +12,10 @@

#include "secure_heap.h"

+struct secure_heap_attachment {
+ struct sg_table *table;
+};
+
static int secure_heap_memory_allocate(struct secure_heap *sec_heap, struct secure_buffer *sec_buf)
{
const struct secure_heap_ops *ops = sec_heap->ops;
@@ -43,6 +47,104 @@ static void secure_heap_memory_free(struct secure_heap *sec_heap, struct secure_
ops->memory_free(sec_heap, sec_buf);
}

+static int secure_heap_attach(struct dma_buf *dmabuf, struct dma_buf_attachment *attachment)
+{
+ struct secure_buffer *sec_buf = dmabuf->priv;
+ struct secure_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, sec_buf->size, 0);
+
+ a->table = table;
+ attachment->priv = a;
+
+ return 0;
+
+err_free_sgt:
+ kfree(table);
+err_free_attach:
+ kfree(a);
+ return ret;
+}
+
+static void secure_heap_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attachment)
+{
+ struct secure_heap_attachment *a = attachment->priv;
+
+ sg_free_table(a->table);
+ kfree(a->table);
+ kfree(a);
+}
+
+static struct sg_table *
+secure_heap_map_dma_buf(struct dma_buf_attachment *attachment, enum dma_data_direction direction)
+{
+ struct secure_heap_attachment *a = attachment->priv;
+ struct sg_table *table = a->table;
+
+ return table;
+}
+
+static void
+secure_heap_unmap_dma_buf(struct dma_buf_attachment *attachment, struct sg_table *table,
+ enum dma_data_direction direction)
+{
+ struct secure_heap_attachment *a = attachment->priv;
+
+ WARN_ON(a->table != table);
+}
+
+static int
+secure_heap_dma_buf_begin_cpu_access(struct dma_buf *dmabuf, enum dma_data_direction direction)
+{
+ return -EPERM;
+}
+
+static int
+secure_heap_dma_buf_end_cpu_access(struct dma_buf *dmabuf, enum dma_data_direction direction)
+{
+ return -EPERM;
+}
+
+static int secure_heap_dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
+{
+ return -EPERM;
+}
+
+static void secure_heap_free(struct dma_buf *dmabuf)
+{
+ struct secure_buffer *sec_buf = dmabuf->priv;
+ struct secure_heap *sec_heap = dma_heap_get_drvdata(sec_buf->heap);
+
+ secure_heap_memory_free(sec_heap, sec_buf);
+ kfree(sec_buf);
+}
+
+static const struct dma_buf_ops sec_heap_buf_ops = {
+ .attach = secure_heap_attach,
+ .detach = secure_heap_detach,
+ .map_dma_buf = secure_heap_map_dma_buf,
+ .unmap_dma_buf = secure_heap_unmap_dma_buf,
+ .begin_cpu_access = secure_heap_dma_buf_begin_cpu_access,
+ .end_cpu_access = secure_heap_dma_buf_end_cpu_access,
+ .mmap = secure_heap_dma_buf_mmap,
+ .release = secure_heap_free,
+};
+
static struct dma_buf *
secure_heap_allocate(struct dma_heap *heap, unsigned long size,
unsigned long fd_flags, unsigned long heap_flags)
@@ -64,6 +166,7 @@ secure_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 = &sec_heap_buf_ops;
exp_info.size = sec_buf->size;
exp_info.flags = fd_flags;
exp_info.priv = sec_buf;
--
2.25.1

2023-12-12 02:47:53

by Yong Wu (吴勇)

[permalink] [raw]
Subject: [PATCH v3 5/7] dma-buf: heaps: secure_heap: Add MediaTek secure heap and heap_init

Add a Mediatek secure heap which uses TEE service call to protect
buffer. Currently this secure heap is NULL, Prepare for the later patch.
Mainly there are two changes:
a) Add a heap_init ops since TEE probe late than secure 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 <[email protected]>
---
drivers/dma-buf/heaps/Kconfig | 7 ++
drivers/dma-buf/heaps/Makefile | 1 +
drivers/dma-buf/heaps/secure_heap.c | 11 +++
drivers/dma-buf/heaps/secure_heap.h | 4 +
drivers/dma-buf/heaps/secure_heap_mtk.c | 114 ++++++++++++++++++++++++
5 files changed, 137 insertions(+)
create mode 100644 drivers/dma-buf/heaps/secure_heap_mtk.c

diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
index 3a9943e94200..12962189878e 100644
--- a/drivers/dma-buf/heaps/Kconfig
+++ b/drivers/dma-buf/heaps/Kconfig
@@ -18,3 +18,10 @@ config DMABUF_HEAPS_SECURE
depends on DMABUF_HEAPS
help
Choose this option to enable dma-buf secure heap. If in doubt, say N.
+
+config DMABUF_HEAPS_SECURE_MTK
+ bool "MediaTek DMA-BUF Secure Heap"
+ depends on DMABUF_HEAPS_SECURE && TEE=y
+ help
+ Enable secure 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 b1ad9d1f2fbe..9751dea345df 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_SECURE) += secure_heap.o
+obj-$(CONFIG_DMABUF_HEAPS_SECURE_MTK) += secure_heap_mtk.o
obj-$(CONFIG_DMABUF_HEAPS_SYSTEM) += system_heap.o
obj-$(CONFIG_DMABUF_HEAPS_CMA) += cma_heap.o
diff --git a/drivers/dma-buf/heaps/secure_heap.c b/drivers/dma-buf/heaps/secure_heap.c
index 7cb4db3e55c2..ca4b433fb3f1 100644
--- a/drivers/dma-buf/heaps/secure_heap.c
+++ b/drivers/dma-buf/heaps/secure_heap.c
@@ -150,11 +150,22 @@ secure_heap_allocate(struct dma_heap *heap, unsigned long size,
unsigned long fd_flags, unsigned long heap_flags)
{
struct secure_heap *sec_heap = dma_heap_get_drvdata(heap);
+ const struct secure_heap_ops *ops = sec_heap->ops;
struct secure_buffer *sec_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(sec_heap);
+ if (ret)
+ return ERR_PTR(ret);
+ }
+
sec_buf = kzalloc(sizeof(*sec_buf), GFP_KERNEL);
if (!sec_buf)
return ERR_PTR(-ENOMEM);
diff --git a/drivers/dma-buf/heaps/secure_heap.h b/drivers/dma-buf/heaps/secure_heap.h
index ec5349cd28d0..1ce9c431d989 100644
--- a/drivers/dma-buf/heaps/secure_heap.h
+++ b/drivers/dma-buf/heaps/secure_heap.h
@@ -17,9 +17,13 @@ struct secure_heap {
const char *name;

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

struct secure_heap_ops {
+ int (*heap_init)(struct secure_heap *sec_heap);
+
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);

diff --git a/drivers/dma-buf/heaps/secure_heap_mtk.c b/drivers/dma-buf/heaps/secure_heap_mtk.c
new file mode 100644
index 000000000000..c7e609dd7bd3
--- /dev/null
+++ b/drivers/dma-buf/heaps/secure_heap_mtk.c
@@ -0,0 +1,114 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * DMABUF MediaTek secure heap exporter
+ *
+ * Copyright (C) 2023 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 "secure_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_secure_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_secure_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_secure_heap_init(struct secure_heap *sec_heap)
+{
+ struct mtk_secure_heap_data *data = sec_heap->priv_data;
+
+ if (!data->tee_ctx)
+ return mtk_tee_session_init(data);
+ return 0;
+}
+
+static const struct secure_heap_ops mtk_sec_mem_ops = {
+ .heap_init = mtk_secure_heap_init,
+};
+
+static struct mtk_secure_heap_data mtk_sec_heap_data = {
+ .mem_type = MTK_SECURE_MEMORY_TYPE_CM_TZ,
+};
+
+static struct secure_heap mtk_secure_heaps[] = {
+ {
+ .name = "secure_mtk_cm",
+ .ops = &mtk_sec_mem_ops,
+ .priv_data = &mtk_sec_heap_data,
+ },
+};
+
+static int mtk_sec_heap_init(void)
+{
+ struct secure_heap *sec_heap = mtk_secure_heaps;
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(mtk_secure_heaps); i++, sec_heap++)
+ secure_heap_add(sec_heap);
+ return 0;
+}
+
+module_init(mtk_sec_heap_init);
+MODULE_DESCRIPTION("MediaTek Secure Heap Driver");
+MODULE_LICENSE("GPL");
--
2.25.1

2023-12-12 02:47:59

by Yong Wu (吴勇)

[permalink] [raw]
Subject: [PATCH v3 6/7] dma-buf: heaps: secure_heap_mtk: Add tee memory service call

Add TEE service call for MediaTek heap. We have a limited number of
hardware entries to protect memory, therefore we cannot protect memory
arbitrarily, then our secure memory management is actually inside OPTEE.
The kernel just tells the TEE what size I want and the TEE will return a
"secure address". The secure_address is a reference to a secure buffer
within TEE. We put it in the sg_dma_address, please see the comment in
code.

Signed-off-by: Yong Wu <[email protected]>
---
drivers/dma-buf/heaps/secure_heap.c | 16 +++++
drivers/dma-buf/heaps/secure_heap.h | 2 +
drivers/dma-buf/heaps/secure_heap_mtk.c | 92 +++++++++++++++++++++++++
3 files changed, 110 insertions(+)

diff --git a/drivers/dma-buf/heaps/secure_heap.c b/drivers/dma-buf/heaps/secure_heap.c
index ca4b433fb3f1..e2b8b97ad4dc 100644
--- a/drivers/dma-buf/heaps/secure_heap.c
+++ b/drivers/dma-buf/heaps/secure_heap.c
@@ -94,8 +94,22 @@ static struct sg_table *
secure_heap_map_dma_buf(struct dma_buf_attachment *attachment, enum dma_data_direction direction)
{
struct secure_heap_attachment *a = attachment->priv;
+ struct dma_buf *dmabuf = attachment->dmabuf;
+ struct secure_buffer *sec_buf = dmabuf->priv;
struct sg_table *table = a->table;

+ /*
+ * Technically dma_address refers to the address used by HW, But for secure buffer
+ * we don't know its dma_address in kernel, Instead, we only know its "secure handle".
+ * Thus use this property to save the "secure handle", and the user will use it to
+ * obtain the real address in secure world.
+ *
+ * Note: CONFIG_DMA_API_DEBUG requires it to be aligned with PAGE_SIZE.
+ */
+ if (sec_buf->secure_address) {
+ sg_dma_address(table->sgl) = sec_buf->secure_address;
+ sg_dma_len(table->sgl) = sec_buf->size;
+ }
return table;
}

@@ -106,6 +120,8 @@ secure_heap_unmap_dma_buf(struct dma_buf_attachment *attachment, struct sg_table
struct secure_heap_attachment *a = attachment->priv;

WARN_ON(a->table != table);
+ sg_dma_address(table->sgl) = 0;
+ sg_dma_len(table->sgl) = 0;
}

static int
diff --git a/drivers/dma-buf/heaps/secure_heap.h b/drivers/dma-buf/heaps/secure_heap.h
index 1ce9c431d989..374fd276bdd7 100644
--- a/drivers/dma-buf/heaps/secure_heap.h
+++ b/drivers/dma-buf/heaps/secure_heap.h
@@ -11,6 +11,8 @@
struct secure_buffer {
struct dma_heap *heap;
size_t size;
+
+ u64 secure_address; /* A reference to the buffer in the secure world */
};

struct secure_heap {
diff --git a/drivers/dma-buf/heaps/secure_heap_mtk.c b/drivers/dma-buf/heaps/secure_heap_mtk.c
index c7e609dd7bd3..2c6aaeaf469f 100644
--- a/drivers/dma-buf/heaps/secure_heap_mtk.c
+++ b/drivers/dma-buf/heaps/secure_heap_mtk.c
@@ -25,6 +25,27 @@ enum mtk_secure_mem_type {
MTK_SECURE_MEMORY_TYPE_CM_TZ = 1,
};

+enum mtk_secure_buffer_tee_cmd {
+ /*
+ * Allocate the zeroed secure memory from TEE.
+ *
+ * [in] value[0].a: The buffer size.
+ * value[0].b: alignment.
+ * [in] value[1].a: enum mtk_secure_mem_type.
+ * [out] value[3].a: The secure handle.
+ */
+ MTK_TZCMD_SECMEM_ZALLOC = 0x10000, /* MTK TEE Command ID Base */
+
+ /*
+ * Free secure memory.
+ *
+ * [in] value[0].a: The secure handle of this buffer, It's value[3].a of
+ * MTK_TZCMD_SECMEM_ZALLOC.
+ * [out] value[1].a: return value, 0 means successful, otherwise fail.
+ */
+ MTK_TZCMD_SECMEM_FREE = 0x10001,
+};
+
struct mtk_secure_heap_data {
struct tee_context *tee_ctx;
u32 tee_session;
@@ -74,6 +95,73 @@ static int mtk_tee_session_init(struct mtk_secure_heap_data *data)
return ret;
}

+static int
+mtk_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 mtk_tee_secure_memory(struct secure_heap *sec_heap, struct secure_buffer *sec_buf)
+{
+ struct mtk_secure_heap_data *data = sec_heap->priv_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 = data->mem_type;
+ params[2].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
+ 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);
+ if (ret)
+ return -ENOMEM;
+
+ sec_buf->secure_address = params[3].u.value.a;
+ return 0;
+}
+
+static void mtk_tee_unsecure_memory(struct secure_heap *sec_heap, struct secure_buffer *sec_buf)
+{
+ struct mtk_secure_heap_data *data = sec_heap->priv_data;
+ 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->secure_address;
+ params[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT;
+
+ mtk_tee_service_call(data->tee_ctx, data->tee_session,
+ MTK_TZCMD_SECMEM_FREE, params);
+ if (params[1].u.value.a)
+ pr_err("%s, unsecure buffer(0x%llx) fail(%lld) from TEE.\n",
+ sec_heap->name, sec_buf->secure_address, params[1].u.value.a);
+}
+
+static int mtk_secure_memory_allocate(struct secure_heap *sec_heap, struct secure_buffer *sec_buf)
+{
+ /* The memory allocating is within the TEE. */
+ return 0;
+}
+
+static void mtk_secure_memory_free(struct secure_heap *sec_heap, struct secure_buffer *sec_buf)
+{
+}
+
static int mtk_secure_heap_init(struct secure_heap *sec_heap)
{
struct mtk_secure_heap_data *data = sec_heap->priv_data;
@@ -85,6 +173,10 @@ static int mtk_secure_heap_init(struct secure_heap *sec_heap)

static const struct secure_heap_ops mtk_sec_mem_ops = {
.heap_init = mtk_secure_heap_init,
+ .memory_alloc = mtk_secure_memory_allocate,
+ .memory_free = mtk_secure_memory_free,
+ .secure_the_memory = mtk_tee_secure_memory,
+ .unsecure_the_memory = mtk_tee_unsecure_memory,
};

static struct mtk_secure_heap_data mtk_sec_heap_data = {
--
2.25.1

2023-12-12 02:48:12

by Yong Wu (吴勇)

[permalink] [raw]
Subject: [PATCH v3 7/7] dma_buf: heaps: secure_heap_mtk: Add a new CMA heap

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" secure heap, "struct cma *cma" is a common property, not just
for MediaTek, so put it into "struct secure_heap" instead of our private
data.

Signed-off-by: Yong Wu <[email protected]>
---
drivers/dma-buf/heaps/Kconfig | 2 +-
drivers/dma-buf/heaps/secure_heap.h | 4 +
drivers/dma-buf/heaps/secure_heap_mtk.c | 119 +++++++++++++++++++++++-
3 files changed, 122 insertions(+), 3 deletions(-)

diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
index 12962189878e..f117d91a0a9d 100644
--- a/drivers/dma-buf/heaps/Kconfig
+++ b/drivers/dma-buf/heaps/Kconfig
@@ -21,7 +21,7 @@ config DMABUF_HEAPS_SECURE

config DMABUF_HEAPS_SECURE_MTK
bool "MediaTek DMA-BUF Secure Heap"
- depends on DMABUF_HEAPS_SECURE && TEE=y
+ depends on DMABUF_HEAPS_SECURE && DMA_CMA && TEE=y
help
Enable secure 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/secure_heap.h b/drivers/dma-buf/heaps/secure_heap.h
index 374fd276bdd7..9f80edcd3e1f 100644
--- a/drivers/dma-buf/heaps/secure_heap.h
+++ b/drivers/dma-buf/heaps/secure_heap.h
@@ -20,6 +20,10 @@ struct secure_heap {

const struct secure_heap_ops *ops;

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

diff --git a/drivers/dma-buf/heaps/secure_heap_mtk.c b/drivers/dma-buf/heaps/secure_heap_mtk.c
index 2c6aaeaf469f..58148120f4ed 100644
--- a/drivers/dma-buf/heaps/secure_heap_mtk.c
+++ b/drivers/dma-buf/heaps/secure_heap_mtk.c
@@ -4,9 +4,11 @@
*
* Copyright (C) 2023 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_secure_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)
@@ -126,6 +140,10 @@ static int mtk_tee_secure_memory(struct secure_heap *sec_heap, struct secure_buf
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 (sec_heap->cma && data->mem_type == MTK_SECURE_MEMORY_TYPE_CM_CMA) {
+ params[2].u.value.a = sec_heap->cma_paddr;
+ params[2].u.value.b = sec_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);
@@ -162,6 +180,48 @@ static void mtk_secure_memory_free(struct secure_heap *sec_heap, struct secure_b
{
}

+static int mtk_secure_memory_cma_allocate(struct secure_heap *sec_heap,
+ struct secure_buffer *sec_buf)
+{
+ struct mtk_secure_heap_data *data = sec_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(sec_heap->cma, sec_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 + sec_buf->size > sec_heap->cma_size) {
+ ret = -EINVAL;
+ goto out_unlock;
+ }
+ data->cma_used_size += sec_buf->size;
+
+out_unlock:
+ mutex_unlock(&data->lock);
+ return ret;
+}
+
+static void mtk_secure_memory_cma_free(struct secure_heap *sec_heap,
+ struct secure_buffer *sec_buf)
+{
+ struct mtk_secure_heap_data *data = sec_heap->priv_data;
+
+ mutex_lock(&data->lock);
+ data->cma_used_size -= sec_buf->size;
+ if (!data->cma_used_size)
+ cma_release(sec_heap->cma, data->cma_page,
+ sec_heap->cma_size >> PAGE_SHIFT);
+ mutex_unlock(&data->lock);
+}
+
static int mtk_secure_heap_init(struct secure_heap *sec_heap)
{
struct mtk_secure_heap_data *data = sec_heap->priv_data;
@@ -183,21 +243,76 @@ static struct mtk_secure_heap_data mtk_sec_heap_data = {
.mem_type = MTK_SECURE_MEMORY_TYPE_CM_TZ,
};

+static const struct secure_heap_ops mtk_sec_mem_ops_cma = {
+ .heap_init = mtk_secure_heap_init,
+ .memory_alloc = mtk_secure_memory_cma_allocate,
+ .memory_free = mtk_secure_memory_cma_free,
+ .secure_the_memory = mtk_tee_secure_memory,
+ .unsecure_the_memory = mtk_tee_unsecure_memory,
+};
+
+static struct mtk_secure_heap_data mtk_sec_heap_data_cma = {
+ .mem_type = MTK_SECURE_MEMORY_TYPE_CM_CMA,
+};
+
static struct secure_heap mtk_secure_heaps[] = {
{
.name = "secure_mtk_cm",
.ops = &mtk_sec_mem_ops,
.priv_data = &mtk_sec_heap_data,
},
+ {
+ .name = "secure_mtk_cma",
+ .ops = &mtk_sec_mem_ops_cma,
+ .priv_data = &mtk_sec_heap_data_cma,
+ },
};

+static int __init mtk_secure_cma_init(struct reserved_mem *rmem)
+{
+ struct mtk_secure_heap_data *data;
+ struct secure_heap *sec_heap = mtk_secure_heaps, *sec_heap_cma = NULL;
+ struct cma *sec_cma;
+ int ret, i;
+
+ for (i = 0; i < ARRAY_SIZE(mtk_secure_heaps); i++, sec_heap++) {
+ data = sec_heap->priv_data;
+ if (data->mem_type == MTK_SECURE_MEMORY_TYPE_CM_CMA) {
+ sec_heap_cma = sec_heap;
+ break;
+ }
+ }
+ if (!sec_heap_cma)
+ return -EINVAL;
+
+ ret = cma_init_reserved_mem(rmem->base, rmem->size, 0, rmem->name,
+ &sec_cma);
+ if (ret) {
+ pr_err("%s: %s set up CMA fail\n", __func__, rmem->name);
+ return ret;
+ }
+
+ sec_heap_cma->cma = sec_cma;
+ sec_heap_cma->cma_paddr = rmem->base;
+ sec_heap_cma->cma_size = rmem->size;
+ return 0;
+}
+
+RESERVEDMEM_OF_DECLARE(secure_cma, "mediatek,dynamic-secure-region", mtk_secure_cma_init);
+
static int mtk_sec_heap_init(void)
{
struct secure_heap *sec_heap = mtk_secure_heaps;
+ struct mtk_secure_heap_data *data;
unsigned int i;

- for (i = 0; i < ARRAY_SIZE(mtk_secure_heaps); i++, sec_heap++)
- secure_heap_add(sec_heap);
+ for (i = 0; i < ARRAY_SIZE(mtk_secure_heaps); i++, sec_heap++) {
+ data = sec_heap->priv_data;
+ if (data->mem_type == MTK_SECURE_MEMORY_TYPE_CM_CMA && !sec_heap->cma)
+ continue;
+ if (!secure_heap_add(sec_heap))
+ mutex_init(&data->lock);
+ }
return 0;
}

--
2.25.1

2023-12-12 16:37:57

by Simon Ser

[permalink] [raw]
Subject: Re: [PATCH v3 0/7] dma-buf: heaps: Add secure heap

Is there a chance to pick a better name than "secure" here?

"Secure" is super overloaded, it's not clear at all what it means from
just the name. Something like "restricted" would be an improvement.

2023-12-13 09:06:06

by Pekka Paalanen

[permalink] [raw]
Subject: Re: [PATCH v3 0/7] dma-buf: heaps: Add secure heap

On Tue, 12 Dec 2023 16:36:35 +0000
Simon Ser <[email protected]> wrote:

> Is there a chance to pick a better name than "secure" here?
>
> "Secure" is super overloaded, it's not clear at all what it means from
> just the name. Something like "restricted" would be an improvement.
>

My thoughts exactly. Every time I see "secure" used for something that
either gives you garbage, refuses to work, or crashes your whole machine
*intentionally* when you try to do normal usual things to it in
userspace (like use it for GL texturing, or try to use KMS writeback), I
get an unscratchable itch.

There is nothing "secure" from security perspective there for end users
and developers. It's just inaccessible buffers.

I've been biting my lip until now, thinking it's too late.


Thanks,
pq


Attachments:
(No filename) (849.00 B)
OpenPGP digital signature

2023-12-13 10:16:12

by Joakim Bech

[permalink] [raw]
Subject: Re: [PATCH v3 0/7] dma-buf: heaps: Add secure heap

On Wed, Dec 13, 2023 at 11:05:17AM +0200, Pekka Paalanen wrote:
> On Tue, 12 Dec 2023 16:36:35 +0000
> Simon Ser <[email protected]> wrote:
>
> > Is there a chance to pick a better name than "secure" here?
> >
> > "Secure" is super overloaded, it's not clear at all what it means from
> > just the name. Something like "restricted" would be an improvement.
> >
>
> My thoughts exactly. Every time I see "secure" used for something that
> either gives you garbage, refuses to work, or crashes your whole machine
> *intentionally* when you try to do normal usual things to it in
> userspace (like use it for GL texturing, or try to use KMS writeback), I
> get an unscratchable itch.
>
> There is nothing "secure" from security perspective there for end users
> and developers. It's just inaccessible buffers.
>
> I've been biting my lip until now, thinking it's too late.
>
The characteristics we're looking for here is a buffer where the content
is inaccessible to the normal OS and user space, i.e., Non-secure EL0 to
EL2. I.e, the content of the buffer is meant to be used and accessible
primarily by the secure side and other devices that has been granted
access to it (for example decoders, display controllers if we're talking
about video use cases). However, since the use cases for this exercises
the whole stack, from non-secure user space (EL0) all the way to secure
user space (S-EL0), with various devices needing access to the buffer at
various times, it makes sense to let Linux manage the buffers, although
it still cannot access the content. That's the overall context.

As for the name, it's always difficult to find a name suitable precisely
describing what it is. "Secure" is perhaps vague, but it might still a
good choice, if you carefully describe what secure means for this
particular heap (in the source code and the documentation for it). For
example, the definition of "secure" for a secure heap as here could mean
that buffer content is inaccessible to the host OS and user space
running in normal world (using Arm nomenclature). I wouldn't have any
problems with calling it secure if, as said it's defined what we mean by
saying so. But I'm all ears for other suggestions as well.

Safe, protected, shielded, unreachable, isolated, inaccessible,
unaccessible, fortified, ... would any of these make more sense?

>
> Thanks,
> pq

--
// Regards
Joakim

2023-12-13 11:38:53

by Pekka Paalanen

[permalink] [raw]
Subject: Re: [PATCH v3 0/7] dma-buf: heaps: Add secure heap

On Wed, 13 Dec 2023 11:15:49 +0100
Joakim Bech <[email protected]> wrote:

> On Wed, Dec 13, 2023 at 11:05:17AM +0200, Pekka Paalanen wrote:
> > On Tue, 12 Dec 2023 16:36:35 +0000
> > Simon Ser <[email protected]> wrote:
> >
> > > Is there a chance to pick a better name than "secure" here?
> > >
> > > "Secure" is super overloaded, it's not clear at all what it means from
> > > just the name. Something like "restricted" would be an improvement.
> > >
> >
> > My thoughts exactly. Every time I see "secure" used for something that
> > either gives you garbage, refuses to work, or crashes your whole machine
> > *intentionally* when you try to do normal usual things to it in
> > userspace (like use it for GL texturing, or try to use KMS writeback), I
> > get an unscratchable itch.
> >
> > There is nothing "secure" from security perspective there for end users
> > and developers. It's just inaccessible buffers.
> >
> > I've been biting my lip until now, thinking it's too late.
> >
> The characteristics we're looking for here is a buffer where the content
> is inaccessible to the normal OS and user space, i.e., Non-secure EL0 to
> EL2. I.e, the content of the buffer is meant to be used and accessible
> primarily by the secure side and other devices that has been granted

s/secure side/proprietary side/

I presume nothing of the other side can ever be in any way open?

Maybe the other side is even less secure than the FOSS side...

> access to it (for example decoders, display controllers if we're talking
> about video use cases). However, since the use cases for this exercises
> the whole stack, from non-secure user space (EL0) all the way to secure
> user space (S-EL0), with various devices needing access to the buffer at
> various times, it makes sense to let Linux manage the buffers, although
> it still cannot access the content. That's the overall context.

Yes, we know all this (except for the exact meaning of EL0 etc.).

> As for the name, it's always difficult to find a name suitable precisely
> describing what it is. "Secure" is perhaps vague, but it might still a
> good choice, if you carefully describe what secure means for this
> particular heap (in the source code and the documentation for it). For

Carefully describe, as in, re-define.

> example, the definition of "secure" for a secure heap as here could mean
> that buffer content is inaccessible to the host OS and user space
> running in normal world (using Arm nomenclature). I wouldn't have any
> problems with calling it secure if, as said it's defined what we mean by
> saying so. But I'm all ears for other suggestions as well.
>
> Safe, protected, shielded, unreachable, isolated, inaccessible,
> unaccessible, fortified, ... would any of these make more sense?

"Restricted" sounds like a good compromise to me. The buffers' usage is
severely restricted.

It is the opposite of "safe", because accessing the contents the wrong
way can return garbage or intentionally crash the whole system,
depending on the hardware implementation. One example is attempting to
put such a buffer on a KMS plane while the connector HDCP state is not
high enough, or a writeback connector is connected to the CRTC. It is
really fragile. (Do KMS drivers fail an atomic commit that would
violate the heap rules? Somehow I doubt that, who'd even know what the
rules are.)

It is protected/shielded/fortified from all the kernel and userspace,
but a more familiar word to describe that is inaccessible.
"Inaccessible buffer" per se OTOH sounds like a useless concept.

It is not secure, because it does not involve security in any way. In
fact, given it's so fragile, I'd classify it as mildly opposite of
secure, as e.g. clients of a Wayland compositor can potentially DoS the
compositor with it by simply sending such a dmabuf. Or DoS the whole
system.

"Poisonous heap" would be fitting but politically inappropriate I
guess. After all, "poison" is data that is not meant to be read by
anything normal.


Thanks,
pq


Attachments:
(No filename) (849.00 B)
OpenPGP digital signature

2023-12-13 13:23:28

by Joakim Bech

[permalink] [raw]
Subject: Re: [PATCH v3 0/7] dma-buf: heaps: Add secure heap

On Wed, Dec 13, 2023 at 01:38:25PM +0200, Pekka Paalanen wrote:
> On Wed, 13 Dec 2023 11:15:49 +0100
> Joakim Bech <[email protected]> wrote:
>
> > On Wed, Dec 13, 2023 at 11:05:17AM +0200, Pekka Paalanen wrote:
> > > On Tue, 12 Dec 2023 16:36:35 +0000
> > > Simon Ser <[email protected]> wrote:
> > >
> > > > Is there a chance to pick a better name than "secure" here?
> > > >
> > > > "Secure" is super overloaded, it's not clear at all what it means from
> > > > just the name. Something like "restricted" would be an improvement.
> > > >
> > >
> > > My thoughts exactly. Every time I see "secure" used for something that
> > > either gives you garbage, refuses to work, or crashes your whole machine
> > > *intentionally* when you try to do normal usual things to it in
> > > userspace (like use it for GL texturing, or try to use KMS writeback), I
> > > get an unscratchable itch.
> > >
> > > There is nothing "secure" from security perspective there for end users
> > > and developers. It's just inaccessible buffers.
> > >
> > > I've been biting my lip until now, thinking it's too late.
> > >
> > The characteristics we're looking for here is a buffer where the content
> > is inaccessible to the normal OS and user space, i.e., Non-secure EL0 to
> > EL2. I.e, the content of the buffer is meant to be used and accessible
> > primarily by the secure side and other devices that has been granted
>
> s/secure side/proprietary side/
>
I'm using the nomenclature as written by Arm (other architectures have
other names for their secure execution states).

> I presume nothing of the other side can ever be in any way open?
>
I'm sure there are lots of examples of things running on the secure side
that are open. The OP-TEE project where I'm a maintainer has been fully
open source since 2014, to give one example that I'm familiar with
myself.

> Maybe the other side is even less secure than the FOSS side...
>
> > access to it (for example decoders, display controllers if we're talking
> > about video use cases). However, since the use cases for this exercises
> > the whole stack, from non-secure user space (EL0) all the way to secure
> > user space (S-EL0), with various devices needing access to the buffer at
> > various times, it makes sense to let Linux manage the buffers, although
> > it still cannot access the content. That's the overall context.
>
> Yes, we know all this (except for the exact meaning of EL0 etc.).
>
Great!

> > As for the name, it's always difficult to find a name suitable precisely
> > describing what it is. "Secure" is perhaps vague, but it might still a
> > good choice, if you carefully describe what secure means for this
> > particular heap (in the source code and the documentation for it). For
>
> Carefully describe, as in, re-define.
>
> > example, the definition of "secure" for a secure heap as here could mean
> > that buffer content is inaccessible to the host OS and user space
> > running in normal world (using Arm nomenclature). I wouldn't have any
> > problems with calling it secure if, as said it's defined what we mean by
> > saying so. But I'm all ears for other suggestions as well.
> >
> > Safe, protected, shielded, unreachable, isolated, inaccessible,
> > unaccessible, fortified, ... would any of these make more sense?
>
> "Restricted" sounds like a good compromise to me. The buffers' usage is
> severely restricted.
>
Yes, restricted isn't a bad choice. We would still need to describe what
we mean by saying it's restricted, i.e., what is it restricted from,
since I'd guess that "restricted" by itself also could be a bit open
ended for a lot of people.

> It is the opposite of "safe", because accessing the contents the wrong
> way can return garbage or intentionally crash the whole system,
> depending on the hardware implementation. One example is attempting to
> put such a buffer on a KMS plane while the connector HDCP state is not
> high enough, or a writeback connector is connected to the CRTC. It is
> really fragile. (Do KMS drivers fail an atomic commit that would
> violate the heap rules? Somehow I doubt that, who'd even know what the
> rules are.)
>
I believe one of the goals with reviewing the patches is to highlight
issues like this and try to figure out how to avoid ending up in
situations like what you described by suggesting alternative solutions
and ideas.

> It is protected/shielded/fortified from all the kernel and userspace,
> but a more familiar word to describe that is inaccessible.
> "Inaccessible buffer" per se OTOH sounds like a useless concept.
>
> It is not secure, because it does not involve security in any way. In
> fact, given it's so fragile, I'd classify it as mildly opposite of
> secure, as e.g. clients of a Wayland compositor can potentially DoS the
> compositor with it by simply sending such a dmabuf. Or DoS the whole
> system.
>
I hear what you are saying and DoS is a known problem and attack vector,
but regardless, we have use cases where we don't want to expose
information in the clear and where we also would like to have some
guarantees about correctness. That is where various secure elements and
more generally security is needed.

So, it sounds like we have two things here, the first is the naming and
the meaning behind it. I'm pretty sure the people following and
contributing to this thread can agree on a name that makes sense. Would
you personally be OK with "restricted" as the name? It sounds like that.

The other thing is the feature and functionality itself offered by this
patch series. My impression from reading your replies is that you think
this is the wrong approach. If my impression is correct, what would you
suggest as an alternative approach?

> "Poisonous heap" would be fitting but politically inappropriate I
> guess. After all, "poison" is data that is not meant to be read by
> anything normal.
>
>
> Thanks,
> pq

--
// Regards
Joakim

2023-12-13 14:01:42

by Christian König

[permalink] [raw]
Subject: Re: [PATCH v3 0/7] dma-buf: heaps: Add secure heap

Am 13.12.23 um 14:22 schrieb Joakim Bech:
> On Wed, Dec 13, 2023 at 01:38:25PM +0200, Pekka Paalanen wrote:
>> On Wed, 13 Dec 2023 11:15:49 +0100
>> Joakim Bech <[email protected]> wrote:
>>
>>> On Wed, Dec 13, 2023 at 11:05:17AM +0200, Pekka Paalanen wrote:
>>>> On Tue, 12 Dec 2023 16:36:35 +0000
>>>> Simon Ser <[email protected]> wrote:
>>>>
>>>>> Is there a chance to pick a better name than "secure" here?
>>>>>
>>>>> "Secure" is super overloaded, it's not clear at all what it means from
>>>>> just the name. Something like "restricted" would be an improvement.
>>>>>
>>>> My thoughts exactly. Every time I see "secure" used for something that
>>>> either gives you garbage, refuses to work, or crashes your whole machine
>>>> *intentionally* when you try to do normal usual things to it in
>>>> userspace (like use it for GL texturing, or try to use KMS writeback), I
>>>> get an unscratchable itch.
>>>>
>>>> There is nothing "secure" from security perspective there for end users
>>>> and developers. It's just inaccessible buffers.
>>>>
>>>> I've been biting my lip until now, thinking it's too late.
>>>>
>>> The characteristics we're looking for here is a buffer where the content
>>> is inaccessible to the normal OS and user space, i.e., Non-secure EL0 to
>>> EL2. I.e, the content of the buffer is meant to be used and accessible
>>> primarily by the secure side and other devices that has been granted
>> s/secure side/proprietary side/
>>
> I'm using the nomenclature as written by Arm (other architectures have
> other names for their secure execution states).

AMDs GPUs call that "trusted" which is only minimal better than secure I
think.

>
>> I presume nothing of the other side can ever be in any way open?
>>
> I'm sure there are lots of examples of things running on the secure side
> that are open. The OP-TEE project where I'm a maintainer has been fully
> open source since 2014, to give one example that I'm familiar with
> myself.

On AMDs GPUs you can actually write shaders which works with the trusted
data and can read and write.

What is prevented is that you copy the data outside of the trusted zone,
e.g. to the CPU. When you do this you only get garbage.

Only engines which have the proper decryption key can send out the data
(for example) to a display device which has authenticated itself using HDCP.

Just a few infos how this is done elsewhere.

Cheers,
Christian.

>
>> Maybe the other side is even less secure than the FOSS side...
>>
>>> access to it (for example decoders, display controllers if we're talking
>>> about video use cases). However, since the use cases for this exercises
>>> the whole stack, from non-secure user space (EL0) all the way to secure
>>> user space (S-EL0), with various devices needing access to the buffer at
>>> various times, it makes sense to let Linux manage the buffers, although
>>> it still cannot access the content. That's the overall context.
>> Yes, we know all this (except for the exact meaning of EL0 etc.).
>>
> Great!
>
>>> As for the name, it's always difficult to find a name suitable precisely
>>> describing what it is. "Secure" is perhaps vague, but it might still a
>>> good choice, if you carefully describe what secure means for this
>>> particular heap (in the source code and the documentation for it). For
>> Carefully describe, as in, re-define.
>>
>>> example, the definition of "secure" for a secure heap as here could mean
>>> that buffer content is inaccessible to the host OS and user space
>>> running in normal world (using Arm nomenclature). I wouldn't have any
>>> problems with calling it secure if, as said it's defined what we mean by
>>> saying so. But I'm all ears for other suggestions as well.
>>>
>>> Safe, protected, shielded, unreachable, isolated, inaccessible,
>>> unaccessible, fortified, ... would any of these make more sense?
>> "Restricted" sounds like a good compromise to me. The buffers' usage is
>> severely restricted.
>>
> Yes, restricted isn't a bad choice. We would still need to describe what
> we mean by saying it's restricted, i.e., what is it restricted from,
> since I'd guess that "restricted" by itself also could be a bit open
> ended for a lot of people.
>
>> It is the opposite of "safe", because accessing the contents the wrong
>> way can return garbage or intentionally crash the whole system,
>> depending on the hardware implementation. One example is attempting to
>> put such a buffer on a KMS plane while the connector HDCP state is not
>> high enough, or a writeback connector is connected to the CRTC. It is
>> really fragile. (Do KMS drivers fail an atomic commit that would
>> violate the heap rules? Somehow I doubt that, who'd even know what the
>> rules are.)
>>
> I believe one of the goals with reviewing the patches is to highlight
> issues like this and try to figure out how to avoid ending up in
> situations like what you described by suggesting alternative solutions
> and ideas.
>
>> It is protected/shielded/fortified from all the kernel and userspace,
>> but a more familiar word to describe that is inaccessible.
>> "Inaccessible buffer" per se OTOH sounds like a useless concept.
>>
>> It is not secure, because it does not involve security in any way. In
>> fact, given it's so fragile, I'd classify it as mildly opposite of
>> secure, as e.g. clients of a Wayland compositor can potentially DoS the
>> compositor with it by simply sending such a dmabuf. Or DoS the whole
>> system.
>>
> I hear what you are saying and DoS is a known problem and attack vector,
> but regardless, we have use cases where we don't want to expose
> information in the clear and where we also would like to have some
> guarantees about correctness. That is where various secure elements and
> more generally security is needed.
>
> So, it sounds like we have two things here, the first is the naming and
> the meaning behind it. I'm pretty sure the people following and
> contributing to this thread can agree on a name that makes sense. Would
> you personally be OK with "restricted" as the name? It sounds like that.
>
> The other thing is the feature and functionality itself offered by this
> patch series. My impression from reading your replies is that you think
> this is the wrong approach. If my impression is correct, what would you
> suggest as an alternative approach?
>
>> "Poisonous heap" would be fitting but politically inappropriate I
>> guess. After all, "poison" is data that is not meant to be read by
>> anything normal.
>>
>>
>> Thanks,
>> pq

2023-12-13 14:16:45

by Pekka Paalanen

[permalink] [raw]
Subject: Re: [PATCH v3 0/7] dma-buf: heaps: Add secure heap

On Wed, 13 Dec 2023 14:22:29 +0100
Joakim Bech <[email protected]> wrote:

> On Wed, Dec 13, 2023 at 01:38:25PM +0200, Pekka Paalanen wrote:
> > On Wed, 13 Dec 2023 11:15:49 +0100
> > Joakim Bech <[email protected]> wrote:
> >
> > > On Wed, Dec 13, 2023 at 11:05:17AM +0200, Pekka Paalanen wrote:
> > > > On Tue, 12 Dec 2023 16:36:35 +0000
> > > > Simon Ser <[email protected]> wrote:
> > > >
> > > > > Is there a chance to pick a better name than "secure" here?
> > > > >
> > > > > "Secure" is super overloaded, it's not clear at all what it means from
> > > > > just the name. Something like "restricted" would be an improvement.
> > > > >
> > > >
> > > > My thoughts exactly. Every time I see "secure" used for something that
> > > > either gives you garbage, refuses to work, or crashes your whole machine
> > > > *intentionally* when you try to do normal usual things to it in
> > > > userspace (like use it for GL texturing, or try to use KMS writeback), I
> > > > get an unscratchable itch.
> > > >
> > > > There is nothing "secure" from security perspective there for end users
> > > > and developers. It's just inaccessible buffers.
> > > >
> > > > I've been biting my lip until now, thinking it's too late.
> > > >
> > > The characteristics we're looking for here is a buffer where the content
> > > is inaccessible to the normal OS and user space, i.e., Non-secure EL0 to
> > > EL2. I.e, the content of the buffer is meant to be used and accessible
> > > primarily by the secure side and other devices that has been granted
> >
> > s/secure side/proprietary side/
> >
> I'm using the nomenclature as written by Arm (other architectures have
> other names for their secure execution states).
>
> > I presume nothing of the other side can ever be in any way open?
> >
> I'm sure there are lots of examples of things running on the secure side
> that are open. The OP-TEE project where I'm a maintainer has been fully
> open source since 2014, to give one example that I'm familiar with
> myself.

Oh, I didn't realise there were FOSS implementations of the other side
that tie in with this secure/restricted heap here. Sorry.

I think the patch series cover letter should point to those to give a
view of the other side to the reviewers, just like DRM requires
userspace to be open for new UAPI.

> > Maybe the other side is even less secure than the FOSS side...
> >
> > > access to it (for example decoders, display controllers if we're talking
> > > about video use cases). However, since the use cases for this exercises
> > > the whole stack, from non-secure user space (EL0) all the way to secure
> > > user space (S-EL0), with various devices needing access to the buffer at
> > > various times, it makes sense to let Linux manage the buffers, although
> > > it still cannot access the content. That's the overall context.
> >
> > Yes, we know all this (except for the exact meaning of EL0 etc.).
> >
> Great!
>
> > > As for the name, it's always difficult to find a name suitable precisely
> > > describing what it is. "Secure" is perhaps vague, but it might still a
> > > good choice, if you carefully describe what secure means for this
> > > particular heap (in the source code and the documentation for it). For
> >
> > Carefully describe, as in, re-define.
> >
> > > example, the definition of "secure" for a secure heap as here could mean
> > > that buffer content is inaccessible to the host OS and user space
> > > running in normal world (using Arm nomenclature). I wouldn't have any
> > > problems with calling it secure if, as said it's defined what we mean by
> > > saying so. But I'm all ears for other suggestions as well.
> > >
> > > Safe, protected, shielded, unreachable, isolated, inaccessible,
> > > unaccessible, fortified, ... would any of these make more sense?
> >
> > "Restricted" sounds like a good compromise to me. The buffers' usage is
> > severely restricted.
> >
> Yes, restricted isn't a bad choice. We would still need to describe what
> we mean by saying it's restricted, i.e., what is it restricted from,
> since I'd guess that "restricted" by itself also could be a bit open
> ended for a lot of people.

Yes, but "restricted" also does not give out an immediate wrong
impression. Label something as "secure", and it immediately raises the
questions of what kind of attacks it prevents and how can I make use of
it.

Is there any use of restricted buffers outside of Digital Rights
Management though? Could a private person somehow make use of it to
protect their own contents? Like a photographer sending drafts to a
customer while not wanting give out any digital copies?

If not, then restricted buffers are something people would generally
like to avoid, not embrace. "Secure" gives a positive impression,
"restricted" a negative impression.

> > It is the opposite of "safe", because accessing the contents the wrong
> > way can return garbage or intentionally crash the whole system,
> > depending on the hardware implementation. One example is attempting to
> > put such a buffer on a KMS plane while the connector HDCP state is not
> > high enough, or a writeback connector is connected to the CRTC. It is
> > really fragile. (Do KMS drivers fail an atomic commit that would
> > violate the heap rules? Somehow I doubt that, who'd even know what the
> > rules are.)
> >
> I believe one of the goals with reviewing the patches is to highlight
> issues like this and try to figure out how to avoid ending up in
> situations like what you described by suggesting alternative solutions
> and ideas.
>
> > It is protected/shielded/fortified from all the kernel and userspace,
> > but a more familiar word to describe that is inaccessible.
> > "Inaccessible buffer" per se OTOH sounds like a useless concept.
> >
> > It is not secure, because it does not involve security in any way. In
> > fact, given it's so fragile, I'd classify it as mildly opposite of
> > secure, as e.g. clients of a Wayland compositor can potentially DoS the
> > compositor with it by simply sending such a dmabuf. Or DoS the whole
> > system.
> >
> I hear what you are saying and DoS is a known problem and attack vector,
> but regardless, we have use cases where we don't want to expose
> information in the clear and where we also would like to have some
> guarantees about correctness. That is where various secure elements and
> more generally security is needed.
>
> So, it sounds like we have two things here, the first is the naming and
> the meaning behind it. I'm pretty sure the people following and
> contributing to this thread can agree on a name that makes sense. Would
> you personally be OK with "restricted" as the name? It sounds like that.

I would. I'm also just a by-stander, not a maintainer of kernel
anything. I have no power to accept nor reject anything here.

> The other thing is the feature and functionality itself offered by this
> patch series. My impression from reading your replies is that you think
> this is the wrong approach. If my impression is correct, what would you
> suggest as an alternative approach?

I just generally dislike locking people out of what their systems hold,
but I also understand why extremely big companies want this Digital
Rights Management technology for their business model. If Linux does
not support that business model, they and the whole broadcast industry
might use something else. At least it pays for kernel developers who
can hopefully do some genuinely useful work on the side as well,
benefiting the community.

Let's just be honest about what is what.


Thanks,
pq


Attachments:
(No filename) (849.00 B)
OpenPGP digital signature

2023-12-22 09:40:52

by Simon Ser

[permalink] [raw]
Subject: Re: [PATCH v3 0/7] dma-buf: heaps: Add secure heap

On Wednesday, December 13th, 2023 at 15:16, Pekka Paalanen <[email protected]> wrote:

> > > It is protected/shielded/fortified from all the kernel and userspace,
> > > but a more familiar word to describe that is inaccessible.
> > > "Inaccessible buffer" per se OTOH sounds like a useless concept.
> > >
> > > It is not secure, because it does not involve security in any way. In
> > > fact, given it's so fragile, I'd classify it as mildly opposite of
> > > secure, as e.g. clients of a Wayland compositor can potentially DoS the
> > > compositor with it by simply sending such a dmabuf. Or DoS the whole
> > > system.
> >
> > I hear what you are saying and DoS is a known problem and attack vector,
> > but regardless, we have use cases where we don't want to expose
> > information in the clear and where we also would like to have some
> > guarantees about correctness. That is where various secure elements and
> > more generally security is needed.
> >
> > So, it sounds like we have two things here, the first is the naming and
> > the meaning behind it. I'm pretty sure the people following and
> > contributing to this thread can agree on a name that makes sense. Would
> > you personally be OK with "restricted" as the name? It sounds like that.
>
> I would. I'm also just a by-stander, not a maintainer of kernel
> anything. I have no power to accept nor reject anything here.

I'd also personally be OK with "restricted", I think it's a lot better
than "secure".

In general I agree with everything Pekka said.

2024-01-04 19:51:18

by Jeffrey Kardatzke

[permalink] [raw]
Subject: Re: [PATCH v3 0/7] dma-buf: heaps: Add secure heap

Any feedback from maintainers on what their preference is? I'm fine
with 'restricted' as well, but the main reason we chose secure was
because of its use in ARM nomenclature and this is more for ARM usage
than x86.

The main difference with similar buffers on AMD/Intel is that with
AMD/Intel the buffers are mappable and readable by the CPU in the
kernel. The problem is their contents are encrypted so you get junk
back if you do that. On ARM, the buffers are completely inaccessible
by the kernel and the memory controller prevents access to them
completely from the kernel.

There are also other use cases for this where the hypervisor is what
is controlling access (second stage in the MMU is providing
isolation)....and in that case I do agree that 'secure' would not be
the right terminology for those types of buffers. So I do agree
something other than 'secure' is probably a better option overall.


On Fri, Dec 22, 2023 at 1:40 AM Simon Ser <[email protected]> wrote:
>
> On Wednesday, December 13th, 2023 at 15:16, Pekka Paalanen <[email protected]> wrote:
>
> > > > It is protected/shielded/fortified from all the kernel and userspace,
> > > > but a more familiar word to describe that is inaccessible.
> > > > "Inaccessible buffer" per se OTOH sounds like a useless concept.
> > > >
> > > > It is not secure, because it does not involve security in any way. In
> > > > fact, given it's so fragile, I'd classify it as mildly opposite of
> > > > secure, as e.g. clients of a Wayland compositor can potentially DoS the
> > > > compositor with it by simply sending such a dmabuf. Or DoS the whole
> > > > system.
> > >
> > > I hear what you are saying and DoS is a known problem and attack vector,
> > > but regardless, we have use cases where we don't want to expose
> > > information in the clear and where we also would like to have some
> > > guarantees about correctness. That is where various secure elements and
> > > more generally security is needed.
> > >
> > > So, it sounds like we have two things here, the first is the naming and
> > > the meaning behind it. I'm pretty sure the people following and
> > > contributing to this thread can agree on a name that makes sense. Would
> > > you personally be OK with "restricted" as the name? It sounds like that.
> >
> > I would. I'm also just a by-stander, not a maintainer of kernel
> > anything. I have no power to accept nor reject anything here.
>
> I'd also personally be OK with "restricted", I think it's a lot better
> than "secure".
>
> In general I agree with everything Pekka said.

2024-01-05 09:37:05

by Christian König

[permalink] [raw]
Subject: Re: [PATCH v3 0/7] dma-buf: heaps: Add secure heap

Am 04.01.24 um 20:50 schrieb Jeffrey Kardatzke:
> Any feedback from maintainers on what their preference is? I'm fine
> with 'restricted' as well, but the main reason we chose secure was
> because of its use in ARM nomenclature and this is more for ARM usage
> than x86.

Well AMD calls this "trusted", but I think that's just slightly better
than "secure".

+1 for using "restricted" cause that seems to match the technical
consequences.

Regards,
Christian.

>
> The main difference with similar buffers on AMD/Intel is that with
> AMD/Intel the buffers are mappable and readable by the CPU in the
> kernel. The problem is their contents are encrypted so you get junk
> back if you do that. On ARM, the buffers are completely inaccessible
> by the kernel and the memory controller prevents access to them
> completely from the kernel.
>
> There are also other use cases for this where the hypervisor is what
> is controlling access (second stage in the MMU is providing
> isolation)....and in that case I do agree that 'secure' would not be
> the right terminology for those types of buffers. So I do agree
> something other than 'secure' is probably a better option overall.
>
>
> On Fri, Dec 22, 2023 at 1:40 AM Simon Ser <[email protected]> wrote:
>> On Wednesday, December 13th, 2023 at 15:16, Pekka Paalanen <[email protected]> wrote:
>>
>>>>> It is protected/shielded/fortified from all the kernel and userspace,
>>>>> but a more familiar word to describe that is inaccessible.
>>>>> "Inaccessible buffer" per se OTOH sounds like a useless concept.
>>>>>
>>>>> It is not secure, because it does not involve security in any way. In
>>>>> fact, given it's so fragile, I'd classify it as mildly opposite of
>>>>> secure, as e.g. clients of a Wayland compositor can potentially DoS the
>>>>> compositor with it by simply sending such a dmabuf. Or DoS the whole
>>>>> system.
>>>> I hear what you are saying and DoS is a known problem and attack vector,
>>>> but regardless, we have use cases where we don't want to expose
>>>> information in the clear and where we also would like to have some
>>>> guarantees about correctness. That is where various secure elements and
>>>> more generally security is needed.
>>>>
>>>> So, it sounds like we have two things here, the first is the naming and
>>>> the meaning behind it. I'm pretty sure the people following and
>>>> contributing to this thread can agree on a name that makes sense. Would
>>>> you personally be OK with "restricted" as the name? It sounds like that.
>>> I would. I'm also just a by-stander, not a maintainer of kernel
>>> anything. I have no power to accept nor reject anything here.
>> I'd also personally be OK with "restricted", I think it's a lot better
>> than "secure".
>>
>> In general I agree with everything Pekka said.


2024-01-09 03:08:27

by Yong Wu (吴勇)

[permalink] [raw]
Subject: Re: [PATCH v3 0/7] dma-buf: heaps: Add secure heap

On Fri, 2024-01-05 at 10:35 +0100, Christian König wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> Am 04.01.24 um 20:50 schrieb Jeffrey Kardatzke:
> > Any feedback from maintainers on what their preference is? I'm
> fine
> > with 'restricted' as well, but the main reason we chose secure was
> > because of its use in ARM nomenclature and this is more for ARM
> usage
> > than x86.
>
> Well AMD calls this "trusted", but I think that's just slightly
> better
> than "secure".
>
> +1 for using "restricted" cause that seems to match the technical
> consequences.

Thanks you all for the discussion and the conclusion. I will send v4 in
this week with "restricted".

>
> Regards,
> Christian.
>
> >
> > The main difference with similar buffers on AMD/Intel is that with
> > AMD/Intel the buffers are mappable and readable by the CPU in the
> > kernel. The problem is their contents are encrypted so you get junk
> > back if you do that. On ARM, the buffers are completely
> inaccessible
> > by the kernel and the memory controller prevents access to them
> > completely from the kernel.
> >
> > There are also other use cases for this where the hypervisor is
> what
> > is controlling access (second stage in the MMU is providing
> > isolation)....and in that case I do agree that 'secure' would not
> be
> > the right terminology for those types of buffers. So I do agree
> > something other than 'secure' is probably a better option overall.
> >
> >
> > On Fri, Dec 22, 2023 at 1:40 AM Simon Ser <[email protected]>
> wrote:
> >> On Wednesday, December 13th, 2023 at 15:16, Pekka Paalanen <
> [email protected]> wrote:
> >>
> >>>>> It is protected/shielded/fortified from all the kernel and
> userspace,
> >>>>> but a more familiar word to describe that is inaccessible.
> >>>>> "Inaccessible buffer" per se OTOH sounds like a useless
> concept.
> >>>>>
> >>>>> It is not secure, because it does not involve security in any
> way. In
> >>>>> fact, given it's so fragile, I'd classify it as mildly opposite
> of
> >>>>> secure, as e.g. clients of a Wayland compositor can potentially
> DoS the
> >>>>> compositor with it by simply sending such a dmabuf. Or DoS the
> whole
> >>>>> system.
> >>>> I hear what you are saying and DoS is a known problem and attack
> vector,
> >>>> but regardless, we have use cases where we don't want to expose
> >>>> information in the clear and where we also would like to have
> some
> >>>> guarantees about correctness. That is where various secure
> elements and
> >>>> more generally security is needed.
> >>>>
> >>>> So, it sounds like we have two things here, the first is the
> naming and
> >>>> the meaning behind it. I'm pretty sure the people following and
> >>>> contributing to this thread can agree on a name that makes
> sense. Would
> >>>> you personally be OK with "restricted" as the name? It sounds
> like that.
> >>> I would. I'm also just a by-stander, not a maintainer of kernel
> >>> anything. I have no power to accept nor reject anything here.
> >> I'd also personally be OK with "restricted", I think it's a lot
> better
> >> than "secure".
> >>
> >> In general I agree with everything Pekka said.
>