[PATCH v3 0/7] tee: Improve support for kexec and kdump

[PATCH v3 0/7] tee: Improve support for kexec and kdump

[Tyler Hicks]

v3:
- Tyler inherited the original series from Allen Pais
- New patch to fix memory leaks in OP-TEE's pool_op_alloc()
  + Unrelated to kexec/kdump
- New patch to refuse to load the OP-TEE driver when booting the kdump
  kernel
- Minor comment typo cleanups (s/alter/alert/) in the "optee: fix tee
  out of memory failure seen during kexec reboot" patch, as mentioned in
  v2 feedback
- New patch to clear stale cache entries during initialization to avoid
  crashes when kexec'ing from a buggy kernel, that didn't disable the
  shm cache, to a fixed kernel
- Three new patches to allow drivers to allocate a multi-page dynamic
  shm that's not dma-buf backed but is still fully registered with the
  TEE, ensuring that all driver private shms are unregistered during
  kexec
v2: https://lore.kernel.org/lkml/20210225090610.242623-1-allen.lkml@gmail.com/
v1: https://lore.kernel.org/lkml/20210217092714.121297-1-allen.lkml@gmail.com/

This series fixes several bugs uncovered while exercising the OP-TEE
(Open Portable Trusted Execution Environment), ftpm (firmware TPM), and
tee_bnxt_fw (Broadcom BNXT firmware manager) drivers with kexec and
kdump (emergency kexec) based workflows.

The majority of the problems are caused by missing .shutdown hooks in
the drivers. The .shutdown hooks are used by the normal kexec code path
to let the drivers clean up prior to executing the target kernel. The
.remove hooks, which are already implemented in these drivers, are not
called as part of the kexec code path. This resulted in shared memory
regions, that were cached and/or registered with OP-TEE, not being
cleared/unregistered prior to kexec. The new kernel would then run into
problems when handling the previously cached virtual addresses or trying
to register newly allocated shared memory objects that overlapped with
the previously registered virtual addresses. The TEE didn't receive
notification that the old virtual addresses were no longer meaningful
and that a new kernel, with a new address space, would soon be running.

However, implementing .shutdown hooks was not enough for supporting
kexec. There was an additional problem caused by the TEE driver's
reliance on the dma-buf subsystem for multi-page shared memory objects
that were registered with the TEE. Shared memory objects backed by a
dma-buf use a different mechanism for reference counting. When the final
reference is released, work is scheduled to be executed to unregister
the shared memory with the TEE but that work is only completed prior to
the current task returning the userspace. In the case of a kexec
operation, the current task that's calling the driver .shutdown hooks
never returns to userspace prior to the kexec operation so the shared
memory was never unregistered. This eventually caused problems from
overlapping shared memory regions that were registered with the TEE
after several kexec operations. The large 4M contiguous region
allocated by the tee_bnxt_fw driver reliably ran into this issue on the
fourth kexec on a system with 8G of RAM.

The use of dma-buf makes sense for shared memory that's in use by
userspace but dma-buf's aren't needed for shared memory that will only
used by the driver. This series separates dma-buf backed shared memory
allocated by the kernel from multi-page shared memory that the kernel
simply needs registered with the TEE for private use.

One other noteworthy change in this series is to completely refuse to
load the OP-TEE driver in the kdump kernel. This is needed because the
secure world may have had all of its threads in suspended state when the
regular kernel crashed. The kdump kernel would then hang during boot
because the OP-TEE driver's .probe function would attempt to use a
secure world thread when they're all in suspended state. Another problem
is that shared memory allocations could fail under the kdump kernel
because the previously registered were not unregistered (the .shutdown
hook is not called when kexec'ing into the kdump kernel).

The first patch in the series fixes potential memory leaks that are not
directly related to kexec or kdump but were noticed during the
development of this series.

Tyler

Allen Pais (2):
  optee: fix tee out of memory failure seen during kexec reboot
  firmware: tee_bnxt: Release shm, session, and context during kexec

Tyler Hicks (5):
  optee: Fix memory leak when failing to register shm pages
  optee: Refuse to load the driver under the kdump kernel
  optee: Clear stale cache entries during initialization
  tee: Support shm registration without dma-buf backing
  tpm_ftpm_tee: Free and unregister dynamic shared memory during kexec

 drivers/char/tpm/tpm_ftpm_tee.c         |  2 +-
 drivers/firmware/broadcom/tee_bnxt_fw.c | 11 ++++++-
 drivers/tee/optee/call.c                | 11 ++++++-
 drivers/tee/optee/core.c                | 42 ++++++++++++++++++++++++-
 drivers/tee/optee/optee_private.h       |  2 +-
 drivers/tee/optee/shm_pool.c            | 17 +++++++---
 drivers/tee/tee_shm.c                   | 11 ++++++-
 7 files changed, 85 insertions(+), 11 deletions(-)

-- 
2.25.1

[PATCH v3 1/7] optee: Fix memory leak when failing to register shm pages

[Tyler Hicks]

Free the previously allocated pages when we encounter an error condition
while attempting to register the pages with the secure world.

Fixes: a249dd200d03 ("tee: optee: Fix dynamic shm pool allocations")
Fixes: 5a769f6ff439 ("optee: Fix multi page dynamic shm pool alloc")
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
 drivers/tee/optee/shm_pool.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/tee/optee/shm_pool.c b/drivers/tee/optee/shm_pool.c
index d767eebf30bd..da06ce9b9313 100644
--- a/drivers/tee/optee/shm_pool.c
+++ b/drivers/tee/optee/shm_pool.c
@@ -32,8 +32,10 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm,
 		struct page **pages;
 
 		pages = kcalloc(nr_pages, sizeof(pages), GFP_KERNEL);
-		if (!pages)
-			return -ENOMEM;
+		if (!pages) {
+			rc = -ENOMEM;
+			goto err;
+		}
 
 		for (i = 0; i < nr_pages; i++) {
 			pages[i] = page;
@@ -44,8 +46,14 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm,
 		rc = optee_shm_register(shm->ctx, shm, pages, nr_pages,
 					(unsigned long)shm->kaddr);
 		kfree(pages);
+		if (rc)
+			goto err;
 	}
 
+	return 0;
+
+err:
+	__free_pages(page, order);
 	return rc;
 }
 
-- 
2.25.1

[PATCH v3 2/7] optee: Refuse to load the driver under the kdump kernel

[Tyler Hicks]

Fix a hung task issue, seen when booting the kdump kernel, that is
caused by all of the secure world threads being in a permanent suspended
state:

 INFO: task swapper/0:1 blocked for more than 120 seconds.
       Not tainted 5.4.83 #1
 "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
 swapper/0       D    0     1      0 0x00000028
 Call trace:
  __switch_to+0xc8/0x118
  __schedule+0x2e0/0x700
  schedule+0x38/0xb8
  schedule_timeout+0x258/0x388
  wait_for_completion+0x16c/0x4b8
  optee_cq_wait_for_completion+0x28/0xa8
  optee_disable_shm_cache+0xb8/0xf8
  optee_probe+0x560/0x61c
  platform_drv_probe+0x58/0xa8
  really_probe+0xe0/0x338
  driver_probe_device+0x5c/0xf0
  device_driver_attach+0x74/0x80
  __driver_attach+0x64/0xe0
  bus_for_each_dev+0x84/0xd8
  driver_attach+0x30/0x40
  bus_add_driver+0x188/0x1e8
  driver_register+0x64/0x110
  __platform_driver_register+0x54/0x60
  optee_driver_init+0x20/0x28
  do_one_initcall+0x54/0x24c
  kernel_init_freeable+0x1e8/0x2c0
  kernel_init+0x18/0x118
  ret_from_fork+0x10/0x18

The invoke_fn hook returned OPTEE_SMC_RETURN_ETHREAD_LIMIT, indicating
that the secure world threads were all in a suspended state at the time
of the kernel crash. This intermittently prevented the kdump kernel from
booting, resulting in a failure to collect the kernel dump.

Make kernel dump collection more reliable on systems utilizing OP-TEE by
refusing to load the driver under the kdump kernel.

Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
 drivers/tee/optee/core.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index ddb8f9ecf307..5288cd767d82 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -6,6 +6,7 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/arm-smccc.h>
+#include <linux/crash_dump.h>
 #include <linux/errno.h>
 #include <linux/io.h>
 #include <linux/module.h>
@@ -612,6 +613,16 @@ static int optee_probe(struct platform_device *pdev)
 	u32 sec_caps;
 	int rc;
 
+	/*
+	 * The kernel may have crashed at the same time that all available
+	 * secure world threads were suspended and we cannot reschedule the
+	 * suspended threads without access to the crashed kernel's wait_queue.
+	 * Therefore, we cannot reliably initialize the OP-TEE driver in the
+	 * kdump kernel.
+	 */
+	if (is_kdump_kernel())
+		return -ENODEV;
+
 	invoke_fn = get_invoke_func(&pdev->dev);
 	if (IS_ERR(invoke_fn))
 		return PTR_ERR(invoke_fn);
-- 
2.25.1

[PATCH v3 3/7] optee: fix tee out of memory failure seen during kexec reboot

[Tyler Hicks]

From: Allen Pais <apais@linux.microsoft.com>

The following out of memory errors are seen on kexec reboot
from the optee core.

[    0.368428] tee_bnxt_fw optee-clnt0: tee_shm_alloc failed
[    0.368461] tee_bnxt_fw: probe of optee-clnt0 failed with error -22

tee_shm_release() is not invoked on dma shm buffer.

Implement .shutdown() method to handle the release of the buffers
correctly.

More info:
https://github.com/OP-TEE/optee_os/issues/3637

Signed-off-by: Allen Pais <apais@linux.microsoft.com>
Reviewed-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
 drivers/tee/optee/core.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index 5288cd767d82..0987074d7ed0 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -573,6 +573,13 @@ static optee_invoke_fn *get_invoke_func(struct device *dev)
 	return ERR_PTR(-EINVAL);
 }
 
+/* optee_remove - Device Removal Routine
+ * @pdev: platform device information struct
+ *
+ * optee_remove is called by platform subsystem to alert the driver
+ * that it should release the device
+ */
+
 static int optee_remove(struct platform_device *pdev)
 {
 	struct optee *optee = platform_get_drvdata(pdev);
@@ -603,6 +610,18 @@ static int optee_remove(struct platform_device *pdev)
 	return 0;
 }
 
+/* optee_shutdown - Device Removal Routine
+ * @pdev: platform device information struct
+ *
+ * platform_shutdown is called by the platform subsystem to alert
+ * the driver that a shutdown, reboot, or kexec is happening and
+ * device must be disabled.
+ */
+static void optee_shutdown(struct platform_device *pdev)
+{
+	optee_disable_shm_cache(platform_get_drvdata(pdev));
+}
+
 static int optee_probe(struct platform_device *pdev)
 {
 	optee_invoke_fn *invoke_fn;
@@ -739,6 +758,7 @@ MODULE_DEVICE_TABLE(of, optee_dt_match);
 static struct platform_driver optee_driver = {
 	.probe  = optee_probe,
 	.remove = optee_remove,
+	.shutdown = optee_shutdown,
 	.driver = {
 		.name = "optee",
 		.of_match_table = optee_dt_match,
-- 
2.25.1

[PATCH v3 4/7] optee: Clear stale cache entries during initialization

[Tyler Hicks]

The shm cache could contain invalid addresses if
optee_disable_shm_cache() was not called from the .shutdown hook of the
previous kernel before a kexec. These addresses could be unmapped or
they could point to mapped but unintended locations in memory.

Clear the shared memory cache, while being careful to not translate the
addresses returned from OPTEE_SMC_DISABLE_SHM_CACHE, during driver
initialization. Once all pre-cache shm objects are removed, proceed with
enabling the cache so that we know that we can handle cached shm objects
with confidence later in the .shutdown hook.

Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
 drivers/tee/optee/call.c          | 11 ++++++++++-
 drivers/tee/optee/core.c          | 13 +++++++++++--
 drivers/tee/optee/optee_private.h |  2 +-
 3 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
index 6e6eb836e9b6..5dcba6105ed7 100644
--- a/drivers/tee/optee/call.c
+++ b/drivers/tee/optee/call.c
@@ -419,8 +419,10 @@ void optee_enable_shm_cache(struct optee *optee)
  * optee_disable_shm_cache() - Disables caching of some shared memory allocation
  *			      in OP-TEE
  * @optee:	main service struct
+ * @is_mapped:	true if the cached shared memory addresses were mapped by this
+ *		kernel, are safe to dereference, and should be freed
  */
-void optee_disable_shm_cache(struct optee *optee)
+void optee_disable_shm_cache(struct optee *optee, bool is_mapped)
 {
 	struct optee_call_waiter w;
 
@@ -439,6 +441,13 @@ void optee_disable_shm_cache(struct optee *optee)
 		if (res.result.status == OPTEE_SMC_RETURN_OK) {
 			struct tee_shm *shm;
 
+			/*
+			 * Shared memory references that were not mapped by
+			 * this kernel must be ignored to prevent a crash.
+			 */
+			if (!is_mapped)
+				continue;
+
 			shm = reg_pair_to_ptr(res.result.shm_upper32,
 					      res.result.shm_lower32);
 			tee_shm_free(shm);
diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index 0987074d7ed0..6974e1104bd4 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -589,7 +589,7 @@ static int optee_remove(struct platform_device *pdev)
 	 * reference counters and also avoid wild pointers in secure world
 	 * into the old shared memory range.
 	 */
-	optee_disable_shm_cache(optee);
+	optee_disable_shm_cache(optee, true);
 
 	/*
 	 * The two devices have to be unregistered before we can free the
@@ -619,7 +619,7 @@ static int optee_remove(struct platform_device *pdev)
  */
 static void optee_shutdown(struct platform_device *pdev)
 {
-	optee_disable_shm_cache(platform_get_drvdata(pdev));
+	optee_disable_shm_cache(platform_get_drvdata(pdev), true);
 }
 
 static int optee_probe(struct platform_device *pdev)
@@ -716,6 +716,15 @@ static int optee_probe(struct platform_device *pdev)
 	optee->memremaped_shm = memremaped_shm;
 	optee->pool = pool;
 
+	/*
+	 * Ensure that there are no pre-existing shm objects before enabling
+	 * the shm cache so that there's no chance of receiving an invalid
+	 * address during shutdown. This could occur, for example, if we're
+	 * kexec booting from an older kernel that did not properly cleanup the
+	 * shm cache.
+	 */
+	optee_disable_shm_cache(optee, false);
+
 	optee_enable_shm_cache(optee);
 
 	if (optee->sec_caps & OPTEE_SMC_SEC_CAP_DYNAMIC_SHM)
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
index e25b216a14ef..16d8c82213e7 100644
--- a/drivers/tee/optee/optee_private.h
+++ b/drivers/tee/optee/optee_private.h
@@ -158,7 +158,7 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg,
 int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session);
 
 void optee_enable_shm_cache(struct optee *optee);
-void optee_disable_shm_cache(struct optee *optee);
+void optee_disable_shm_cache(struct optee *optee, bool is_mapped);
 
 int optee_shm_register(struct tee_context *ctx, struct tee_shm *shm,
 		       struct page **pages, size_t num_pages,
-- 
2.25.1

[PATCH v3 5/7] tee: Support shm registration without dma-buf backing

[Tyler Hicks]

Uncouple the registration of dynamic shared memory buffers from the
TEE_SHM_DMA_BUF flag. Drivers may wish to allocate dynamic shared memory
regions but do not need them to be backed by a dma-buf when the memory
region is private to the driver.

Allow callers of tee_shm_alloc() to specify the TEE_SHM_REGISTER flag to
request registration. If the TEE implementation does not require dynamic
shared memory to be registered, clear the flag prior to calling the
corresponding pool alloc function. Update the OP-TEE driver to respect
TEE_SHM_REGISTER, rather than TEE_SHM_DMA_BUF, when deciding whether to
(un)register on alloc/free operations. The AMD-TEE driver continues to
ignore the TEE_SHM_REGISTER flag.

Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
 drivers/tee/optee/shm_pool.c |  5 ++---
 drivers/tee/tee_shm.c        | 11 ++++++++++-
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/tee/optee/shm_pool.c b/drivers/tee/optee/shm_pool.c
index da06ce9b9313..6054343a29fb 100644
--- a/drivers/tee/optee/shm_pool.c
+++ b/drivers/tee/optee/shm_pool.c
@@ -27,7 +27,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm,
 	shm->paddr = page_to_phys(page);
 	shm->size = PAGE_SIZE << order;
 
-	if (shm->flags & TEE_SHM_DMA_BUF) {
+	if (shm->flags & TEE_SHM_REGISTER) {
 		unsigned int nr_pages = 1 << order, i;
 		struct page **pages;
 
@@ -42,7 +42,6 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm,
 			page++;
 		}
 
-		shm->flags |= TEE_SHM_REGISTER;
 		rc = optee_shm_register(shm->ctx, shm, pages, nr_pages,
 					(unsigned long)shm->kaddr);
 		kfree(pages);
@@ -60,7 +59,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm,
 static void pool_op_free(struct tee_shm_pool_mgr *poolm,
 			 struct tee_shm *shm)
 {
-	if (shm->flags & TEE_SHM_DMA_BUF)
+	if (shm->flags & TEE_SHM_REGISTER)
 		optee_shm_unregister(shm->ctx, shm);
 
 	free_pages((unsigned long)shm->kaddr, get_order(shm->size));
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
index 00472f5ce22e..1c0176550b9c 100644
--- a/drivers/tee/tee_shm.c
+++ b/drivers/tee/tee_shm.c
@@ -117,7 +117,7 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
 		return ERR_PTR(-EINVAL);
 	}
 
-	if ((flags & ~(TEE_SHM_MAPPED | TEE_SHM_DMA_BUF))) {
+	if ((flags & ~(TEE_SHM_MAPPED | TEE_SHM_DMA_BUF | TEE_SHM_REGISTER))) {
 		dev_err(teedev->dev.parent, "invalid shm flags 0x%x", flags);
 		return ERR_PTR(-EINVAL);
 	}
@@ -137,6 +137,15 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
 		goto err_dev_put;
 	}
 
+	if (!teedev->desc->ops->shm_register ||
+	    !teedev->desc->ops->shm_unregister) {
+		/* registration is not required by the TEE implementation */
+		flags &= ~TEE_SHM_REGISTER;
+	} else if (flags & TEE_SHM_DMA_BUF) {
+		/* all dma-buf backed shm allocations are registered */
+		flags |= TEE_SHM_REGISTER;
+	}
+
 	shm->flags = flags | TEE_SHM_POOL;
 	shm->ctx = ctx;
 	if (flags & TEE_SHM_DMA_BUF)
-- 
2.25.1

[PATCH v3 6/7] tpm_ftpm_tee: Free and unregister dynamic shared memory during kexec

[Tyler Hicks]

dma-buf backed shared memory cannot be reliably freed and unregistered
during a kexec operation even when tee_shm_free() is called on the shm
from a .shutdown hook. The problem occurs because dma_buf_put() calls
fput() which then uses task_work_add(), with the TWA_RESUME parameter,
to queue tee_shm_release() to be called before the current task returns
to user mode. However, the current task never returns to user mode
before the kexec completes so the memory is never freed nor
unregistered.

Don't use dma-buf backed shared memory for a multi-page dynamic shm
that's private to the driver. Not using a dma-buf backed shm will allow
tee_shm_free() to directly call tee_shm_release() so that the shared
memory can be freed and unregistered during a kexec operation.

Continue to register the multi-page dynamic shm with the TEE so that all
4K chunks can be used.

Fixes: 09e574831b27 ("tpm/tpm_ftpm_tee: A driver for firmware TPM running inside TEE")
Fixes: 1760eb689ed6 ("tpm/tpm_ftpm_tee: add shutdown call back")
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
 drivers/char/tpm/tpm_ftpm_tee.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
index 2ccdf8ac6994..8f1155227506 100644
--- a/drivers/char/tpm/tpm_ftpm_tee.c
+++ b/drivers/char/tpm/tpm_ftpm_tee.c
@@ -256,7 +256,7 @@ static int ftpm_tee_probe(struct device *dev)
 	/* Allocate dynamic shared memory with fTPM TA */
 	pvt_data->shm = tee_shm_alloc(pvt_data->ctx,
 				      MAX_COMMAND_SIZE + MAX_RESPONSE_SIZE,
-				      TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
+				      TEE_SHM_MAPPED | TEE_SHM_REGISTER);
 	if (IS_ERR(pvt_data->shm)) {
 		dev_err(dev, "%s: tee_shm_alloc failed\n", __func__);
 		rc = -ENOMEM;
-- 
2.25.1

[PATCH v3 7/7] firmware: tee_bnxt: Release shm, session, and context during kexec

[Tyler Hicks]

From: Allen Pais <apais@linux.microsoft.com>

Implement a .shutdown hook that will be called during a kexec operation
so that the TEE shared memory, session, and context that were set up
during .probe can be properly freed/closed.

Additionally, don't use dma-buf backed shared memory for the multi-page
dynamic shm that's private to the driver. dma-buf backed shared memory
cannot be reliably freed and unregistered during a kexec operation even
when tee_shm_free() is called on the shm from a .shutdown hook. The
problem occurs because dma_buf_put() calls fput() which then uses
task_work_add(), with the TWA_RESUME parameter, to queue
tee_shm_release() to be called before the current task returns to user
mode. However, the current task never returns to user mode before the
kexec completes so the memory is never freed nor unregistered.

Continue to register the multi-page dynamic shm with the TEE so that all
4K chunks can be used.

Fixes: 246880958ac9 ("firmware: broadcom: add OP-TEE based BNXT f/w manager")
Signed-off-by: Allen Pais <apais@linux.microsoft.com>
Co-developed-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
 drivers/firmware/broadcom/tee_bnxt_fw.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/broadcom/tee_bnxt_fw.c b/drivers/firmware/broadcom/tee_bnxt_fw.c
index ed10da5313e8..c496723e9e8d 100644
--- a/drivers/firmware/broadcom/tee_bnxt_fw.c
+++ b/drivers/firmware/broadcom/tee_bnxt_fw.c
@@ -213,7 +213,7 @@ static int tee_bnxt_fw_probe(struct device *dev)
 	pvt_data.dev = dev;
 
 	fw_shm_pool = tee_shm_alloc(pvt_data.ctx, MAX_SHM_MEM_SZ,
-				    TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
+				    TEE_SHM_MAPPED | TEE_SHM_REGISTER);
 	if (IS_ERR(fw_shm_pool)) {
 		dev_err(pvt_data.dev, "tee_shm_alloc failed\n");
 		err = PTR_ERR(fw_shm_pool);
@@ -242,6 +242,14 @@ static int tee_bnxt_fw_remove(struct device *dev)
 	return 0;
 }
 
+static void tee_bnxt_fw_shutdown(struct device *dev)
+{
+	tee_shm_free(pvt_data.fw_shm_pool);
+	tee_client_close_session(pvt_data.ctx, pvt_data.session_id);
+	tee_client_close_context(pvt_data.ctx);
+	pvt_data.ctx = NULL;
+}
+
 static const struct tee_client_device_id tee_bnxt_fw_id_table[] = {
 	{UUID_INIT(0x6272636D, 0x2019, 0x0716,
 		    0x42, 0x43, 0x4D, 0x5F, 0x53, 0x43, 0x48, 0x49)},
@@ -257,6 +265,7 @@ static struct tee_client_driver tee_bnxt_fw_driver = {
 		.bus		= &tee_bus_type,
 		.probe		= tee_bnxt_fw_probe,
 		.remove		= tee_bnxt_fw_remove,
+		.shutdown	= tee_bnxt_fw_shutdown,
 	},
 };
 
-- 
2.25.1

Re: [PATCH v3 5/7] tee: Support shm registration without dma-buf backing

[Sumit Garg]

Hi Tyler,

On Wed, 9 Jun 2021 at 05:55, Tyler Hicks <tyhicks@linux.microsoft.com> wrote:
>
> Uncouple the registration of dynamic shared memory buffers from the
> TEE_SHM_DMA_BUF flag. Drivers may wish to allocate dynamic shared memory
> regions but do not need them to be backed by a dma-buf when the memory
> region is private to the driver.

In this case drivers should use tee_shm_register() instead where the
memory allocated is actually private to the driver. However, you need
to remove TEE_SHM_DMA_BUF as a mandatory flag for tee_shm_register().
Have a look at an example here [1]. So modifying tee_shm_alloc() for
this purpose doesn't look appropriate to me.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/security/keys/trusted-keys/trusted_tee.c#n73

-Sumit

>
> Allow callers of tee_shm_alloc() to specify the TEE_SHM_REGISTER flag to
> request registration. If the TEE implementation does not require dynamic
> shared memory to be registered, clear the flag prior to calling the
> corresponding pool alloc function. Update the OP-TEE driver to respect
> TEE_SHM_REGISTER, rather than TEE_SHM_DMA_BUF, when deciding whether to
> (un)register on alloc/free operations. The AMD-TEE driver continues to
> ignore the TEE_SHM_REGISTER flag.
>
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> ---
>  drivers/tee/optee/shm_pool.c |  5 ++---
>  drivers/tee/tee_shm.c        | 11 ++++++++++-
>  2 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tee/optee/shm_pool.c b/drivers/tee/optee/shm_pool.c
> index da06ce9b9313..6054343a29fb 100644
> --- a/drivers/tee/optee/shm_pool.c
> +++ b/drivers/tee/optee/shm_pool.c
> @@ -27,7 +27,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm,
>         shm->paddr = page_to_phys(page);
>         shm->size = PAGE_SIZE << order;
>
> -       if (shm->flags & TEE_SHM_DMA_BUF) {
> +       if (shm->flags & TEE_SHM_REGISTER) {
>                 unsigned int nr_pages = 1 << order, i;
>                 struct page **pages;
>
> @@ -42,7 +42,6 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm,
>                         page++;
>                 }
>
> -               shm->flags |= TEE_SHM_REGISTER;
>                 rc = optee_shm_register(shm->ctx, shm, pages, nr_pages,
>                                         (unsigned long)shm->kaddr);
>                 kfree(pages);
> @@ -60,7 +59,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm,
>  static void pool_op_free(struct tee_shm_pool_mgr *poolm,
>                          struct tee_shm *shm)
>  {
> -       if (shm->flags & TEE_SHM_DMA_BUF)
> +       if (shm->flags & TEE_SHM_REGISTER)
>                 optee_shm_unregister(shm->ctx, shm);
>
>         free_pages((unsigned long)shm->kaddr, get_order(shm->size));
> diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> index 00472f5ce22e..1c0176550b9c 100644
> --- a/drivers/tee/tee_shm.c
> +++ b/drivers/tee/tee_shm.c
> @@ -117,7 +117,7 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
>                 return ERR_PTR(-EINVAL);
>         }
>
> -       if ((flags & ~(TEE_SHM_MAPPED | TEE_SHM_DMA_BUF))) {
> +       if ((flags & ~(TEE_SHM_MAPPED | TEE_SHM_DMA_BUF | TEE_SHM_REGISTER))) {
>                 dev_err(teedev->dev.parent, "invalid shm flags 0x%x", flags);
>                 return ERR_PTR(-EINVAL);
>         }
> @@ -137,6 +137,15 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
>                 goto err_dev_put;
>         }
>
> +       if (!teedev->desc->ops->shm_register ||
> +           !teedev->desc->ops->shm_unregister) {
> +               /* registration is not required by the TEE implementation */
> +               flags &= ~TEE_SHM_REGISTER;
> +       } else if (flags & TEE_SHM_DMA_BUF) {
> +               /* all dma-buf backed shm allocations are registered */
> +               flags |= TEE_SHM_REGISTER;
> +       }
> +
>         shm->flags = flags | TEE_SHM_POOL;
>         shm->ctx = ctx;
>         if (flags & TEE_SHM_DMA_BUF)
> --
> 2.25.1
>

Re: [PATCH v3 5/7] tee: Support shm registration without dma-buf backing

[Tyler Hicks]

On 2021-06-09 09:59:04, Sumit Garg wrote:
> Hi Tyler,

Hey Sumit - Thanks for the review. 

> 
> On Wed, 9 Jun 2021 at 05:55, Tyler Hicks <tyhicks@linux.microsoft.com> wrote:
> >
> > Uncouple the registration of dynamic shared memory buffers from the
> > TEE_SHM_DMA_BUF flag. Drivers may wish to allocate dynamic shared memory
> > regions but do not need them to be backed by a dma-buf when the memory
> > region is private to the driver.
> 
> In this case drivers should use tee_shm_register() instead where the
> memory allocated is actually private to the driver. However, you need
> to remove TEE_SHM_DMA_BUF as a mandatory flag for tee_shm_register().
> Have a look at an example here [1]. So modifying tee_shm_alloc() for
> this purpose doesn't look appropriate to me.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/security/keys/trusted-keys/trusted_tee.c#n73

I noticed what you did in commit 2a6ba3f794e8 ("tee: enable support to
register kernel memory") and considered moving ftpm and tee_bnxt_fw over
to tee_shm_register(). I think that's likely the right long term
approach but I decided against it since this series is a minimal set of
bug fixes that will hopefully go to stable (I'm affected by these bugs
in 5.4). Here are my reasons for feeling like moving to
tee_shm_register() isn't minimal in terms of a stable-focused fix:

- tee_shm_alloc() looks like it should work fine with AMD-TEE today.
  tee_shm_register() definitely does not since AMD-TEE doesn't provide a
  .shm_register or .shm_unregister hook. This may break existing users
  of AMD-TEE?
- tee_shm_register() has not historically been used for kernel
  allocations and is not fixed wrt the bug that Jens fixed in commit
  f1bbacedb0af ("tee: don't assign shm id for private shms").
- tee_shm_alloc() performs allocations using contiguous pages
  from alloc_pages() while tee_shm_register() performs non-contiguous
  allocations with kcalloc(). I suspect this would be fine but I don't
  know the secure world side of these things well enough to assess the
  risk involved with such a change on the kernel side.

I should have mentioned this in the cover letter but my hope was that
these minimal changes would be accepted and then additional work could
be done to merge tee_shm_alloc() and tee_shm_register() in a way that
would allow the caller to request contiguous or non-contiguous pages,
fix up the additional issues mentioned above, and then adjust the
call sites in ftpm and tee_bnxt_fw as appropriate.

I think that's a bigger set of changes because there are several things
that still confuse/concern me:

- Why does tee_shm_alloc() use TEE_SHM_MAPPED while tee_shm_register()
  uses TEE_SHM_KERNEL_MAPPED or TEE_SHM_USER_MAPPED? Why do all three
  exist?
- Why does tee_shm_register() unconditionally use non-contiguous
  allocations without ever taking into account whether or not
  OPTEE_SMC_SEC_CAP_DYNAMIC_SHM was set? It sounds like that's required
  from my reading of https://optee.readthedocs.io/en/latest/architecture/core.html#noncontiguous-shared-buffers.
- Why is TEE_SHM_REGISTER implemented at the TEE driver level when it is
  specific to OP-TEE? How to better abstract that away?

Let me know if you agree with the more minimal approach that I took for
these bug fix series or still feel like tee_shm_register() should be
fixed up so that it is usable. Thanks!

Tyler

> 
> -Sumit
> 
> >
> > Allow callers of tee_shm_alloc() to specify the TEE_SHM_REGISTER flag to
> > request registration. If the TEE implementation does not require dynamic
> > shared memory to be registered, clear the flag prior to calling the
> > corresponding pool alloc function. Update the OP-TEE driver to respect
> > TEE_SHM_REGISTER, rather than TEE_SHM_DMA_BUF, when deciding whether to
> > (un)register on alloc/free operations. The AMD-TEE driver continues to
> > ignore the TEE_SHM_REGISTER flag.
> >
> > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > ---
> >  drivers/tee/optee/shm_pool.c |  5 ++---
> >  drivers/tee/tee_shm.c        | 11 ++++++++++-
> >  2 files changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/tee/optee/shm_pool.c b/drivers/tee/optee/shm_pool.c
> > index da06ce9b9313..6054343a29fb 100644
> > --- a/drivers/tee/optee/shm_pool.c
> > +++ b/drivers/tee/optee/shm_pool.c
> > @@ -27,7 +27,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm,
> >         shm->paddr = page_to_phys(page);
> >         shm->size = PAGE_SIZE << order;
> >
> > -       if (shm->flags & TEE_SHM_DMA_BUF) {
> > +       if (shm->flags & TEE_SHM_REGISTER) {
> >                 unsigned int nr_pages = 1 << order, i;
> >                 struct page **pages;
> >
> > @@ -42,7 +42,6 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm,
> >                         page++;
> >                 }
> >
> > -               shm->flags |= TEE_SHM_REGISTER;
> >                 rc = optee_shm_register(shm->ctx, shm, pages, nr_pages,
> >                                         (unsigned long)shm->kaddr);
> >                 kfree(pages);
> > @@ -60,7 +59,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm,
> >  static void pool_op_free(struct tee_shm_pool_mgr *poolm,
> >                          struct tee_shm *shm)
> >  {
> > -       if (shm->flags & TEE_SHM_DMA_BUF)
> > +       if (shm->flags & TEE_SHM_REGISTER)
> >                 optee_shm_unregister(shm->ctx, shm);
> >
> >         free_pages((unsigned long)shm->kaddr, get_order(shm->size));
> > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> > index 00472f5ce22e..1c0176550b9c 100644
> > --- a/drivers/tee/tee_shm.c
> > +++ b/drivers/tee/tee_shm.c
> > @@ -117,7 +117,7 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
> >                 return ERR_PTR(-EINVAL);
> >         }
> >
> > -       if ((flags & ~(TEE_SHM_MAPPED | TEE_SHM_DMA_BUF))) {
> > +       if ((flags & ~(TEE_SHM_MAPPED | TEE_SHM_DMA_BUF | TEE_SHM_REGISTER))) {
> >                 dev_err(teedev->dev.parent, "invalid shm flags 0x%x", flags);
> >                 return ERR_PTR(-EINVAL);
> >         }
> > @@ -137,6 +137,15 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
> >                 goto err_dev_put;
> >         }
> >
> > +       if (!teedev->desc->ops->shm_register ||
> > +           !teedev->desc->ops->shm_unregister) {
> > +               /* registration is not required by the TEE implementation */
> > +               flags &= ~TEE_SHM_REGISTER;
> > +       } else if (flags & TEE_SHM_DMA_BUF) {
> > +               /* all dma-buf backed shm allocations are registered */
> > +               flags |= TEE_SHM_REGISTER;
> > +       }
> > +
> >         shm->flags = flags | TEE_SHM_POOL;
> >         shm->ctx = ctx;
> >         if (flags & TEE_SHM_DMA_BUF)
> > --
> > 2.25.1
> >
> 

Re: [PATCH v3 5/7] tee: Support shm registration without dma-buf backing

[Sumit Garg]

+ Rijo

On Wed, 9 Jun 2021 at 11:16, Tyler Hicks <tyhicks@linux.microsoft.com> wrote:
>
> On 2021-06-09 09:59:04, Sumit Garg wrote:
> > Hi Tyler,
>
> Hey Sumit - Thanks for the review.
>
> >
> > On Wed, 9 Jun 2021 at 05:55, Tyler Hicks <tyhicks@linux.microsoft.com> wrote:
> > >
> > > Uncouple the registration of dynamic shared memory buffers from the
> > > TEE_SHM_DMA_BUF flag. Drivers may wish to allocate dynamic shared memory
> > > regions but do not need them to be backed by a dma-buf when the memory
> > > region is private to the driver.
> >
> > In this case drivers should use tee_shm_register() instead where the
> > memory allocated is actually private to the driver. However, you need
> > to remove TEE_SHM_DMA_BUF as a mandatory flag for tee_shm_register().
> > Have a look at an example here [1]. So modifying tee_shm_alloc() for
> > this purpose doesn't look appropriate to me.
> >
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/security/keys/trusted-keys/trusted_tee.c#n73
>
> I noticed what you did in commit 2a6ba3f794e8 ("tee: enable support to
> register kernel memory") and considered moving ftpm and tee_bnxt_fw over
> to tee_shm_register(). I think that's likely the right long term
> approach but I decided against it since this series is a minimal set of
> bug fixes that will hopefully go to stable (I'm affected by these bugs
> in 5.4). Here are my reasons for feeling like moving to
> tee_shm_register() isn't minimal in terms of a stable-focused fix:
>
> - tee_shm_alloc() looks like it should work fine with AMD-TEE today.
>   tee_shm_register() definitely does not since AMD-TEE doesn't provide a
>   .shm_register or .shm_unregister hook. This may break existing users
>   of AMD-TEE?

AFAIK, ftpm and tee_bnxt_fw drivers only support OP-TEE at this point.
See ftpm_tee_match() and optee_ctx_match() APIs in corresponding
drivers.

> - tee_shm_register() has not historically been used for kernel
>   allocations and is not fixed wrt the bug that Jens fixed in commit
>   f1bbacedb0af ("tee: don't assign shm id for private shms").

Yes, that's what I meant earlier to make the TEE_SHM_DMA_BUF flag optional.

> - tee_shm_alloc() performs allocations using contiguous pages
>   from alloc_pages() while tee_shm_register() performs non-contiguous
>   allocations with kcalloc(). I suspect this would be fine but I don't
>   know the secure world side of these things well enough to assess the
>   risk involved with such a change on the kernel side.
>

I don't think that would make any difference.

> I should have mentioned this in the cover letter but my hope was that
> these minimal changes would be accepted and then additional work could
> be done to merge tee_shm_alloc() and tee_shm_register() in a way that
> would allow the caller to request contiguous or non-contiguous pages,
> fix up the additional issues mentioned above, and then adjust the
> call sites in ftpm and tee_bnxt_fw as appropriate.
>
> I think that's a bigger set of changes because there are several things
> that still confuse/concern me:
>
> - Why does tee_shm_alloc() use TEE_SHM_MAPPED while tee_shm_register()
>   uses TEE_SHM_KERNEL_MAPPED or TEE_SHM_USER_MAPPED? Why do all three
>   exist?

AFAIK, its due the the inherent nature of tee_shm_alloc() and
tee_shm_register() where tee_shm_alloc() doesn't need to know whether
its a kernel or user-space memory since it is the one that allocates
whereas tee_shm_register() need to know that since it has to register
pre-allocated client memory.

> - Why does tee_shm_register() unconditionally use non-contiguous
>   allocations without ever taking into account whether or not
>   OPTEE_SMC_SEC_CAP_DYNAMIC_SHM was set? It sounds like that's required
>   from my reading of https://optee.readthedocs.io/en/latest/architecture/core.html#noncontiguous-shared-buffers.

Yeah, but do we have platforms in OP-TEE that don't support dynamic
shared memory? I guess it has become the sane default which is a
mandatory requirement when it comes to OP-TEE driver in u-boot.

> - Why is TEE_SHM_REGISTER implemented at the TEE driver level when it is
>   specific to OP-TEE? How to better abstract that away?
>

I would like you to go through Section "3.2.4. Shared Memory" in TEE
Client API Specification. There are two standard ways for shared
memory approach with TEE:

1. A Shared Memory block can either be existing Client Application
memory (kernel driver in our case) which is subsequently registered
with the TEE Client API (using tee_shm_register() in our case).

2. Or memory which is allocated on behalf of the Client Application
using the TEE
Client API (using tee_shm_alloc() in our case).

> Let me know if you agree with the more minimal approach that I took for
> these bug fix series or still feel like tee_shm_register() should be
> fixed up so that it is usable. Thanks!

From drivers perspective I think the change should be:

tee_shm_alloc()

to

kcalloc()
tee_shm_register()

-Sumit

> > > Allow callers of tee_shm_alloc() to specify the TEE_SHM_REGISTER flag to.
> > > request registration. If the TEE implementation does not require dynamic
> > > shared memory to be registered, clear the flag prior to calling the
> > > corresponding pool alloc function. Update the OP-TEE driver to respect
> > > TEE_SHM_REGISTER, rather than TEE_SHM_DMA_BUF, when deciding whether to
> > > (un)register on alloc/free operations. The AMD-TEE driver continues to
> > > ignore the TEE_SHM_REGISTER flag.
> > >
> > > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > > ---
> > >  drivers/tee/optee/shm_pool.c |  5 ++---
> > >  drivers/tee/tee_shm.c        | 11 ++++++++++-
> > >  2 files changed, 12 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/tee/optee/shm_pool.c b/drivers/tee/optee/shm_pool.c
> > > index da06ce9b9313..6054343a29fb 100644
> > > --- a/drivers/tee/optee/shm_pool.c
> > > +++ b/drivers/tee/optee/shm_pool.c
> > > @@ -27,7 +27,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm,
> > >         shm->paddr = page_to_phys(page);
> > >         shm->size = PAGE_SIZE << order;
> > >
> > > -       if (shm->flags & TEE_SHM_DMA_BUF) {
> > > +       if (shm->flags & TEE_SHM_REGISTER) {
> > >                 unsigned int nr_pages = 1 << order, i;
> > >                 struct page **pages;
> > >
> > > @@ -42,7 +42,6 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm,
> > >                         page++;
> > >                 }
> > >
> > > -               shm->flags |= TEE_SHM_REGISTER;
> > >                 rc = optee_shm_register(shm->ctx, shm, pages, nr_pages,
> > >                                         (unsigned long)shm->kaddr);
> > >                 kfree(pages);
> > > @@ -60,7 +59,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm,
> > >  static void pool_op_free(struct tee_shm_pool_mgr *poolm,
> > >                          struct tee_shm *shm)
> > >  {
> > > -       if (shm->flags & TEE_SHM_DMA_BUF)
> > > +       if (shm->flags & TEE_SHM_REGISTER)
> > >                 optee_shm_unregister(shm->ctx, shm);
> > >
> > >         free_pages((unsigned long)shm->kaddr, get_order(shm->size));
> > > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> > > index 00472f5ce22e..1c0176550b9c 100644
> > > --- a/drivers/tee/tee_shm.c
> > > +++ b/drivers/tee/tee_shm.c
> > > @@ -117,7 +117,7 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
> > >                 return ERR_PTR(-EINVAL);
> > >         }
> > >
> > > -       if ((flags & ~(TEE_SHM_MAPPED | TEE_SHM_DMA_BUF))) {
> > > +       if ((flags & ~(TEE_SHM_MAPPED | TEE_SHM_DMA_BUF | TEE_SHM_REGISTER))) {
> > >                 dev_err(teedev->dev.parent, "invalid shm flags 0x%x", flags);
> > >                 return ERR_PTR(-EINVAL);
> > >         }
> > > @@ -137,6 +137,15 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
> > >                 goto err_dev_put;
> > >         }
> > >
> > > +       if (!teedev->desc->ops->shm_register ||
> > > +           !teedev->desc->ops->shm_unregister) {
> > > +               /* registration is not required by the TEE implementation */
> > > +               flags &= ~TEE_SHM_REGISTER;
> > > +       } else if (flags & TEE_SHM_DMA_BUF) {
> > > +               /* all dma-buf backed shm allocations are registered */
> > > +               flags |= TEE_SHM_REGISTER;
> > > +       }
> > > +
> > >         shm->flags = flags | TEE_SHM_POOL;
> > >         shm->ctx = ctx;
> > >         if (flags & TEE_SHM_DMA_BUF)
> > > --
> > > 2.25.1
> > >
> >

Re: [PATCH v3 5/7] tee: Support shm registration without dma-buf backing

[Jens Wiklander]

Hi,

On Wed, Jun 09, 2021 at 04:22:49PM +0530, Sumit Garg wrote:
> + Rijo
> 
> On Wed, 9 Jun 2021 at 11:16, Tyler Hicks <tyhicks@linux.microsoft.com> wrote:
> >
> > On 2021-06-09 09:59:04, Sumit Garg wrote:
> > > Hi Tyler,
> >
> > Hey Sumit - Thanks for the review.
> >
> > >
> > > On Wed, 9 Jun 2021 at 05:55, Tyler Hicks <tyhicks@linux.microsoft.com> wrote:
> > > >
> > > > Uncouple the registration of dynamic shared memory buffers from the
> > > > TEE_SHM_DMA_BUF flag. Drivers may wish to allocate dynamic shared memory
> > > > regions but do not need them to be backed by a dma-buf when the memory
> > > > region is private to the driver.
> > >
> > > In this case drivers should use tee_shm_register() instead where the
> > > memory allocated is actually private to the driver. However, you need
> > > to remove TEE_SHM_DMA_BUF as a mandatory flag for tee_shm_register().
> > > Have a look at an example here [1]. So modifying tee_shm_alloc() for
> > > this purpose doesn't look appropriate to me.
> > >
> > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/security/keys/trusted-keys/trusted_tee.c#n73
> >
> > I noticed what you did in commit 2a6ba3f794e8 ("tee: enable support to
> > register kernel memory") and considered moving ftpm and tee_bnxt_fw over
> > to tee_shm_register(). I think that's likely the right long term
> > approach but I decided against it since this series is a minimal set of
> > bug fixes that will hopefully go to stable (I'm affected by these bugs
> > in 5.4). Here are my reasons for feeling like moving to
> > tee_shm_register() isn't minimal in terms of a stable-focused fix:
> >
> > - tee_shm_alloc() looks like it should work fine with AMD-TEE today.
> >   tee_shm_register() definitely does not since AMD-TEE doesn't provide a
> >   .shm_register or .shm_unregister hook. This may break existing users
> >   of AMD-TEE?
> 
> AFAIK, ftpm and tee_bnxt_fw drivers only support OP-TEE at this point.
> See ftpm_tee_match() and optee_ctx_match() APIs in corresponding
> drivers.
> 
> > - tee_shm_register() has not historically been used for kernel
> >   allocations and is not fixed wrt the bug that Jens fixed in commit
> >   f1bbacedb0af ("tee: don't assign shm id for private shms").
> 
> Yes, that's what I meant earlier to make the TEE_SHM_DMA_BUF flag optional.
> 
> > - tee_shm_alloc() performs allocations using contiguous pages
> >   from alloc_pages() while tee_shm_register() performs non-contiguous
> >   allocations with kcalloc(). I suspect this would be fine but I don't
> >   know the secure world side of these things well enough to assess the
> >   risk involved with such a change on the kernel side.
> >
> 
> I don't think that would make any difference.
> 
> > I should have mentioned this in the cover letter but my hope was that
> > these minimal changes would be accepted and then additional work could
> > be done to merge tee_shm_alloc() and tee_shm_register() in a way that
> > would allow the caller to request contiguous or non-contiguous pages,
> > fix up the additional issues mentioned above, and then adjust the
> > call sites in ftpm and tee_bnxt_fw as appropriate.
> >
> > I think that's a bigger set of changes because there are several things
> > that still confuse/concern me:
> >
> > - Why does tee_shm_alloc() use TEE_SHM_MAPPED while tee_shm_register()
> >   uses TEE_SHM_KERNEL_MAPPED or TEE_SHM_USER_MAPPED? Why do all three
> >   exist?
> 
> AFAIK, its due the the inherent nature of tee_shm_alloc() and
> tee_shm_register() where tee_shm_alloc() doesn't need to know whether
> its a kernel or user-space memory since it is the one that allocates
> whereas tee_shm_register() need to know that since it has to register
> pre-allocated client memory.
> 
> > - Why does tee_shm_register() unconditionally use non-contiguous
> >   allocations without ever taking into account whether or not
> >   OPTEE_SMC_SEC_CAP_DYNAMIC_SHM was set? It sounds like that's required
> >   from my reading of https://optee.readthedocs.io/en/latest/architecture/core.html#noncontiguous-shared-buffers.
> 
> Yeah, but do we have platforms in OP-TEE that don't support dynamic
> shared memory? I guess it has become the sane default which is a
> mandatory requirement when it comes to OP-TEE driver in u-boot.
> 
> > - Why is TEE_SHM_REGISTER implemented at the TEE driver level when it is
> >   specific to OP-TEE? How to better abstract that away?
> >
> 
> I would like you to go through Section "3.2.4. Shared Memory" in TEE
> Client API Specification. There are two standard ways for shared
> memory approach with TEE:
> 
> 1. A Shared Memory block can either be existing Client Application
> memory (kernel driver in our case) which is subsequently registered
> with the TEE Client API (using tee_shm_register() in our case).
> 
> 2. Or memory which is allocated on behalf of the Client Application
> using the TEE
> Client API (using tee_shm_alloc() in our case).
> 
> > Let me know if you agree with the more minimal approach that I took for
> > these bug fix series or still feel like tee_shm_register() should be
> > fixed up so that it is usable. Thanks!
> 
> From drivers perspective I think the change should be:
> 
> tee_shm_alloc()
> 
> to
> 
> kcalloc()
> tee_shm_register()

I've just posted "[PATCH 0/7] tee: shared memory updates",
https://lore.kernel.org/lkml/20210609102324.2222332-1-jens.wiklander@linaro.org/

Where tee_shm_alloc() is replaced by among other functions
tee_shm_alloc_kernel_buf(). tee_shm_alloc_kernel_buf() takes care of the
problem with TEE_SHM_DMA_BUF.

Cheers,
Jens

Re: [PATCH v3 5/7] tee: Support shm registration without dma-buf backing

[Tyler Hicks]

On 2021-06-09 14:15:33, Jens Wiklander wrote:
> Hi,
> 
> On Wed, Jun 09, 2021 at 04:22:49PM +0530, Sumit Garg wrote:
> > + Rijo
> > 
> > On Wed, 9 Jun 2021 at 11:16, Tyler Hicks <tyhicks@linux.microsoft.com> wrote:
> > >
> > > On 2021-06-09 09:59:04, Sumit Garg wrote:
> > > > Hi Tyler,
> > >
> > > Hey Sumit - Thanks for the review.
> > >
> > > >
> > > > On Wed, 9 Jun 2021 at 05:55, Tyler Hicks <tyhicks@linux.microsoft.com> wrote:
> > > > >
> > > > > Uncouple the registration of dynamic shared memory buffers from the
> > > > > TEE_SHM_DMA_BUF flag. Drivers may wish to allocate dynamic shared memory
> > > > > regions but do not need them to be backed by a dma-buf when the memory
> > > > > region is private to the driver.
> > > >
> > > > In this case drivers should use tee_shm_register() instead where the
> > > > memory allocated is actually private to the driver. However, you need
> > > > to remove TEE_SHM_DMA_BUF as a mandatory flag for tee_shm_register().
> > > > Have a look at an example here [1]. So modifying tee_shm_alloc() for
> > > > this purpose doesn't look appropriate to me.
> > > >
> > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/security/keys/trusted-keys/trusted_tee.c#n73
> > >
> > > I noticed what you did in commit 2a6ba3f794e8 ("tee: enable support to
> > > register kernel memory") and considered moving ftpm and tee_bnxt_fw over
> > > to tee_shm_register(). I think that's likely the right long term
> > > approach but I decided against it since this series is a minimal set of
> > > bug fixes that will hopefully go to stable (I'm affected by these bugs
> > > in 5.4). Here are my reasons for feeling like moving to
> > > tee_shm_register() isn't minimal in terms of a stable-focused fix:
> > >
> > > - tee_shm_alloc() looks like it should work fine with AMD-TEE today.
> > >   tee_shm_register() definitely does not since AMD-TEE doesn't provide a
> > >   .shm_register or .shm_unregister hook. This may break existing users
> > >   of AMD-TEE?
> > 
> > AFAIK, ftpm and tee_bnxt_fw drivers only support OP-TEE at this point.
> > See ftpm_tee_match() and optee_ctx_match() APIs in corresponding
> > drivers.
> > 
> > > - tee_shm_register() has not historically been used for kernel
> > >   allocations and is not fixed wrt the bug that Jens fixed in commit
> > >   f1bbacedb0af ("tee: don't assign shm id for private shms").
> > 
> > Yes, that's what I meant earlier to make the TEE_SHM_DMA_BUF flag optional.
> > 
> > > - tee_shm_alloc() performs allocations using contiguous pages
> > >   from alloc_pages() while tee_shm_register() performs non-contiguous
> > >   allocations with kcalloc(). I suspect this would be fine but I don't
> > >   know the secure world side of these things well enough to assess the
> > >   risk involved with such a change on the kernel side.
> > >
> > 
> > I don't think that would make any difference.
> > 
> > > I should have mentioned this in the cover letter but my hope was that
> > > these minimal changes would be accepted and then additional work could
> > > be done to merge tee_shm_alloc() and tee_shm_register() in a way that
> > > would allow the caller to request contiguous or non-contiguous pages,
> > > fix up the additional issues mentioned above, and then adjust the
> > > call sites in ftpm and tee_bnxt_fw as appropriate.
> > >
> > > I think that's a bigger set of changes because there are several things
> > > that still confuse/concern me:
> > >
> > > - Why does tee_shm_alloc() use TEE_SHM_MAPPED while tee_shm_register()
> > >   uses TEE_SHM_KERNEL_MAPPED or TEE_SHM_USER_MAPPED? Why do all three
> > >   exist?
> > 
> > AFAIK, its due the the inherent nature of tee_shm_alloc() and
> > tee_shm_register() where tee_shm_alloc() doesn't need to know whether
> > its a kernel or user-space memory since it is the one that allocates
> > whereas tee_shm_register() need to know that since it has to register
> > pre-allocated client memory.
> > 
> > > - Why does tee_shm_register() unconditionally use non-contiguous
> > >   allocations without ever taking into account whether or not
> > >   OPTEE_SMC_SEC_CAP_DYNAMIC_SHM was set? It sounds like that's required
> > >   from my reading of https://optee.readthedocs.io/en/latest/architecture/core.html#noncontiguous-shared-buffers.
> > 
> > Yeah, but do we have platforms in OP-TEE that don't support dynamic
> > shared memory? I guess it has become the sane default which is a
> > mandatory requirement when it comes to OP-TEE driver in u-boot.
> > 
> > > - Why is TEE_SHM_REGISTER implemented at the TEE driver level when it is
> > >   specific to OP-TEE? How to better abstract that away?
> > >
> > 
> > I would like you to go through Section "3.2.4. Shared Memory" in TEE
> > Client API Specification. There are two standard ways for shared
> > memory approach with TEE:
> > 
> > 1. A Shared Memory block can either be existing Client Application
> > memory (kernel driver in our case) which is subsequently registered
> > with the TEE Client API (using tee_shm_register() in our case).
> > 
> > 2. Or memory which is allocated on behalf of the Client Application
> > using the TEE
> > Client API (using tee_shm_alloc() in our case).
> > 
> > > Let me know if you agree with the more minimal approach that I took for
> > > these bug fix series or still feel like tee_shm_register() should be
> > > fixed up so that it is usable. Thanks!
> > 
> > From drivers perspective I think the change should be:
> > 
> > tee_shm_alloc()
> > 
> > to
> > 
> > kcalloc()
> > tee_shm_register()
> 
> I've just posted "[PATCH 0/7] tee: shared memory updates",
> https://lore.kernel.org/lkml/20210609102324.2222332-1-jens.wiklander@linaro.org/
> 
> Where tee_shm_alloc() is replaced by among other functions
> tee_shm_alloc_kernel_buf(). tee_shm_alloc_kernel_buf() takes care of the
> problem with TEE_SHM_DMA_BUF.

Thanks! At first glance, that series would take care of the last three
patches in my kexec/kdump series.

I'm a bit worried that it is a rewrite of the shm allocator. Do you plan
to send all of that to stable? (I mentioned earlier in this thread that
I'm affected by these bugs in linux-5.4.y.)

Also, you and Sumit don't seem to have the same opinion on kernel
drivers making use of tee_shm_register() for allocations that are only
used internally. Can you comment on that?

I'm not clear on the next steps for fixing these kexec/kdump bugs in
older releases. I appreciate any guidance here.

Tyler

> 
> Cheers,
> Jens
> 

Re: [PATCH v3 5/7] tee: Support shm registration without dma-buf backing

[Tyler Hicks]

On 2021-06-09 08:42:28, Tyler Hicks wrote:
> On 2021-06-09 14:15:33, Jens Wiklander wrote:
> > Hi,
> > 
> > On Wed, Jun 09, 2021 at 04:22:49PM +0530, Sumit Garg wrote:
> > > + Rijo
> > > 
> > > On Wed, 9 Jun 2021 at 11:16, Tyler Hicks <tyhicks@linux.microsoft.com> wrote:
> > > >
> > > > On 2021-06-09 09:59:04, Sumit Garg wrote:
> > > > > Hi Tyler,
> > > >
> > > > Hey Sumit - Thanks for the review.
> > > >
> > > > >
> > > > > On Wed, 9 Jun 2021 at 05:55, Tyler Hicks <tyhicks@linux.microsoft.com> wrote:
> > > > > >
> > > > > > Uncouple the registration of dynamic shared memory buffers from the
> > > > > > TEE_SHM_DMA_BUF flag. Drivers may wish to allocate dynamic shared memory
> > > > > > regions but do not need them to be backed by a dma-buf when the memory
> > > > > > region is private to the driver.
> > > > >
> > > > > In this case drivers should use tee_shm_register() instead where the
> > > > > memory allocated is actually private to the driver. However, you need
> > > > > to remove TEE_SHM_DMA_BUF as a mandatory flag for tee_shm_register().
> > > > > Have a look at an example here [1]. So modifying tee_shm_alloc() for
> > > > > this purpose doesn't look appropriate to me.
> > > > >
> > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/security/keys/trusted-keys/trusted_tee.c#n73
> > > >
> > > > I noticed what you did in commit 2a6ba3f794e8 ("tee: enable support to
> > > > register kernel memory") and considered moving ftpm and tee_bnxt_fw over
> > > > to tee_shm_register(). I think that's likely the right long term
> > > > approach but I decided against it since this series is a minimal set of
> > > > bug fixes that will hopefully go to stable (I'm affected by these bugs
> > > > in 5.4). Here are my reasons for feeling like moving to
> > > > tee_shm_register() isn't minimal in terms of a stable-focused fix:
> > > >
> > > > - tee_shm_alloc() looks like it should work fine with AMD-TEE today.
> > > >   tee_shm_register() definitely does not since AMD-TEE doesn't provide a
> > > >   .shm_register or .shm_unregister hook. This may break existing users
> > > >   of AMD-TEE?
> > > 
> > > AFAIK, ftpm and tee_bnxt_fw drivers only support OP-TEE at this point.
> > > See ftpm_tee_match() and optee_ctx_match() APIs in corresponding
> > > drivers.
> > > 
> > > > - tee_shm_register() has not historically been used for kernel
> > > >   allocations and is not fixed wrt the bug that Jens fixed in commit
> > > >   f1bbacedb0af ("tee: don't assign shm id for private shms").
> > > 
> > > Yes, that's what I meant earlier to make the TEE_SHM_DMA_BUF flag optional.
> > > 
> > > > - tee_shm_alloc() performs allocations using contiguous pages
> > > >   from alloc_pages() while tee_shm_register() performs non-contiguous
> > > >   allocations with kcalloc(). I suspect this would be fine but I don't
> > > >   know the secure world side of these things well enough to assess the
> > > >   risk involved with such a change on the kernel side.
> > > >
> > > 
> > > I don't think that would make any difference.
> > > 
> > > > I should have mentioned this in the cover letter but my hope was that
> > > > these minimal changes would be accepted and then additional work could
> > > > be done to merge tee_shm_alloc() and tee_shm_register() in a way that
> > > > would allow the caller to request contiguous or non-contiguous pages,
> > > > fix up the additional issues mentioned above, and then adjust the
> > > > call sites in ftpm and tee_bnxt_fw as appropriate.
> > > >
> > > > I think that's a bigger set of changes because there are several things
> > > > that still confuse/concern me:
> > > >
> > > > - Why does tee_shm_alloc() use TEE_SHM_MAPPED while tee_shm_register()
> > > >   uses TEE_SHM_KERNEL_MAPPED or TEE_SHM_USER_MAPPED? Why do all three
> > > >   exist?
> > > 
> > > AFAIK, its due the the inherent nature of tee_shm_alloc() and
> > > tee_shm_register() where tee_shm_alloc() doesn't need to know whether
> > > its a kernel or user-space memory since it is the one that allocates
> > > whereas tee_shm_register() need to know that since it has to register
> > > pre-allocated client memory.
> > > 
> > > > - Why does tee_shm_register() unconditionally use non-contiguous
> > > >   allocations without ever taking into account whether or not
> > > >   OPTEE_SMC_SEC_CAP_DYNAMIC_SHM was set? It sounds like that's required
> > > >   from my reading of https://optee.readthedocs.io/en/latest/architecture/core.html#noncontiguous-shared-buffers.
> > > 
> > > Yeah, but do we have platforms in OP-TEE that don't support dynamic
> > > shared memory? I guess it has become the sane default which is a
> > > mandatory requirement when it comes to OP-TEE driver in u-boot.
> > > 
> > > > - Why is TEE_SHM_REGISTER implemented at the TEE driver level when it is
> > > >   specific to OP-TEE? How to better abstract that away?
> > > >
> > > 
> > > I would like you to go through Section "3.2.4. Shared Memory" in TEE
> > > Client API Specification. There are two standard ways for shared
> > > memory approach with TEE:
> > > 
> > > 1. A Shared Memory block can either be existing Client Application
> > > memory (kernel driver in our case) which is subsequently registered
> > > with the TEE Client API (using tee_shm_register() in our case).
> > > 
> > > 2. Or memory which is allocated on behalf of the Client Application
> > > using the TEE
> > > Client API (using tee_shm_alloc() in our case).
> > > 
> > > > Let me know if you agree with the more minimal approach that I took for
> > > > these bug fix series or still feel like tee_shm_register() should be
> > > > fixed up so that it is usable. Thanks!
> > > 
> > > From drivers perspective I think the change should be:
> > > 
> > > tee_shm_alloc()
> > > 
> > > to
> > > 
> > > kcalloc()
> > > tee_shm_register()
> > 
> > I've just posted "[PATCH 0/7] tee: shared memory updates",
> > https://lore.kernel.org/lkml/20210609102324.2222332-1-jens.wiklander@linaro.org/
> > 
> > Where tee_shm_alloc() is replaced by among other functions
> > tee_shm_alloc_kernel_buf(). tee_shm_alloc_kernel_buf() takes care of the
> > problem with TEE_SHM_DMA_BUF.
> 
> Thanks! At first glance, that series would take care of the last three
> patches in my kexec/kdump series.

Correction: Your series would not completely take care of the last three
patches in my kexec/kdump series because your series doesn't implement
the .shutdown() hook for tee_bnxt_fw.

Does it make sense to take my series first and then rebase your series
on top of it? That would allow my fixes to flow back to stable, then
your changes would greatly clean up the implementation in future
releases.

Tyler

> 
> I'm a bit worried that it is a rewrite of the shm allocator. Do you plan
> to send all of that to stable? (I mentioned earlier in this thread that
> I'm affected by these bugs in linux-5.4.y.)
> 
> Also, you and Sumit don't seem to have the same opinion on kernel
> drivers making use of tee_shm_register() for allocations that are only
> used internally. Can you comment on that?
> 
> I'm not clear on the next steps for fixing these kexec/kdump bugs in
> older releases. I appreciate any guidance here.
> 
> Tyler
> 
> > 
> > Cheers,
> > Jens
> > 

Re: [PATCH v3 5/7] tee: Support shm registration without dma-buf backing

[Jens Wiklander]

On Wed, Jun 09, 2021 at 04:22:49PM +0530, Sumit Garg wrote:
> + Rijo
> 
> On Wed, 9 Jun 2021 at 11:16, Tyler Hicks <tyhicks@linux.microsoft.com> wrote:
[snip]
> 
> > - tee_shm_alloc() performs allocations using contiguous pages
> >   from alloc_pages() while tee_shm_register() performs non-contiguous
> >   allocations with kcalloc(). I suspect this would be fine but I don't
> >   know the secure world side of these things well enough to assess the
> >   risk involved with such a change on the kernel side.
> >
> 
> I don't think that would make any difference.

Agree.

> 
> > I should have mentioned this in the cover letter but my hope was that
> > these minimal changes would be accepted and then additional work could
> > be done to merge tee_shm_alloc() and tee_shm_register() in a way that
> > would allow the caller to request contiguous or non-contiguous pages,
> > fix up the additional issues mentioned above, and then adjust the
> > call sites in ftpm and tee_bnxt_fw as appropriate.
> >
> > I think that's a bigger set of changes because there are several things
> > that still confuse/concern me:
> >
> > - Why does tee_shm_alloc() use TEE_SHM_MAPPED while tee_shm_register()
> >   uses TEE_SHM_KERNEL_MAPPED or TEE_SHM_USER_MAPPED? Why do all three
> >   exist?
> 
> AFAIK, its due the the inherent nature of tee_shm_alloc() and
> tee_shm_register() where tee_shm_alloc() doesn't need to know whether
> its a kernel or user-space memory since it is the one that allocates
> whereas tee_shm_register() need to know that since it has to register
> pre-allocated client memory.
> 
> > - Why does tee_shm_register() unconditionally use non-contiguous
> >   allocations without ever taking into account whether or not
> >   OPTEE_SMC_SEC_CAP_DYNAMIC_SHM was set? It sounds like that's required
> >   from my reading of https://optee.readthedocs.io/en/latest/architecture/core.html#noncontiguous-shared-buffers.
> 
> Yeah, but do we have platforms in OP-TEE that don't support dynamic
> shared memory? I guess it has become the sane default which is a
> mandatory requirement when it comes to OP-TEE driver in u-boot.
> 
> > - Why is TEE_SHM_REGISTER implemented at the TEE driver level when it is
> >   specific to OP-TEE? How to better abstract that away?
> >
> 
> I would like you to go through Section "3.2.4. Shared Memory" in TEE
> Client API Specification. There are two standard ways for shared
> memory approach with TEE:
> 
> 1. A Shared Memory block can either be existing Client Application
> memory (kernel driver in our case) which is subsequently registered
> with the TEE Client API (using tee_shm_register() in our case).
> 
> 2. Or memory which is allocated on behalf of the Client Application
> using the TEE
> Client API (using tee_shm_alloc() in our case).
> 
> > Let me know if you agree with the more minimal approach that I took for
> > these bug fix series or still feel like tee_shm_register() should be
> > fixed up so that it is usable. Thanks!
> 
> From drivers perspective I think the change should be:
> 
> tee_shm_alloc()
> 
> to
> 
> kcalloc()
> tee_shm_register()

I had another approach in mind in "[PATCH 0/7] tee: shared memory updates",
https://lore.kernel.org/lkml/20210609102324.2222332-1-jens.wiklander@linaro.org/

The flags needed by tee_shm_alloc() and tee_shm_register() aren't
very intuitive and in fact only accept quite few combinations. So my
idea was to hide those flags from callers outside of the TEE subsystem
with tee_shm_alloc_kernel_buf().

The approach with tee_shm_register() you suggest above has the drawback
that the TEE driver is forced to be able to handle any kernel memory.
This is OK with OP-TEE and dynamic shared memory enabled, but there are
platforms where dynamic shared memory isn't enabled. In those case must
the memory be allocated from a special pool.

Do you see any problem with instead replacing tee_shm_alloc()
with tee_shm_alloc_kernel_buf()?

Cheers,
Jens

Re: [PATCH v3 5/7] tee: Support shm registration without dma-buf backing

[Jens Wiklander]

On Wed, Jun 09, 2021 at 08:51:04AM -0500, Tyler Hicks wrote:
[snip]
> > > I've just posted "[PATCH 0/7] tee: shared memory updates",
> > > https://lore.kernel.org/lkml/20210609102324.2222332-1-jens.wiklander@linaro.org/
> > > 
> > > Where tee_shm_alloc() is replaced by among other functions
> > > tee_shm_alloc_kernel_buf(). tee_shm_alloc_kernel_buf() takes care of the
> > > problem with TEE_SHM_DMA_BUF.
> > 
> > Thanks! At first glance, that series would take care of the last three
> > patches in my kexec/kdump series.
> 
> Correction: Your series would not completely take care of the last three
> patches in my kexec/kdump series because your series doesn't implement
> the .shutdown() hook for tee_bnxt_fw.
> 
> Does it make sense to take my series first and then rebase your series
> on top of it? That would allow my fixes to flow back to stable, then
> your changes would greatly clean up the implementation in future
> releases.

Yes, we could try that. I'd like to see tee_shm_alloc_kernel_buf() being
used instead of tee_shm_alloc() in ftpm_tee_probe() and
tee_bnxt_fw_probe(). So it would be great if you could include "tee: add
tee_shm_alloc_kernel_buf()" in your patch set.

My patch set would then shrink a bit. By the way, thanks for reviewing
it.

Cheers,
Jens

Re: [PATCH v3 5/7] tee: Support shm registration without dma-buf backing

[Allen Pais]

>> 
>> AFAIK, its due the the inherent nature of tee_shm_alloc() and
>> tee_shm_register() where tee_shm_alloc() doesn't need to know whether
>> its a kernel or user-space memory since it is the one that allocates
>> whereas tee_shm_register() need to know that since it has to register
>> pre-allocated client memory.
>> 
>>> - Why does tee_shm_register() unconditionally use non-contiguous
>>>  allocations without ever taking into account whether or not
>>>  OPTEE_SMC_SEC_CAP_DYNAMIC_SHM was set? It sounds like that's required
>>>  from my reading of https://optee.readthedocs.io/en/latest/architecture/core.html#noncontiguous-shared-buffers.
>> 
>> Yeah, but do we have platforms in OP-TEE that don't support dynamic
>> shared memory? I guess it has become the sane default which is a
>> mandatory requirement when it comes to OP-TEE driver in u-boot.
>> 
>>> - Why is TEE_SHM_REGISTER implemented at the TEE driver level when it is
>>>  specific to OP-TEE? How to better abstract that away?
>>> 
>> 
>> I would like you to go through Section "3.2.4. Shared Memory" in TEE
>> Client API Specification. There are two standard ways for shared
>> memory approach with TEE:
>> 
>> 1. A Shared Memory block can either be existing Client Application
>> memory (kernel driver in our case) which is subsequently registered
>> with the TEE Client API (using tee_shm_register() in our case).
>> 
>> 2. Or memory which is allocated on behalf of the Client Application
>> using the TEE
>> Client API (using tee_shm_alloc() in our case).
>> 
>>> Let me know if you agree with the more minimal approach that I took for
>>> these bug fix series or still feel like tee_shm_register() should be
>>> fixed up so that it is usable. Thanks!
>> 
>> From drivers perspective I think the change should be:
>> 
>> tee_shm_alloc()
>> 
>> to
>> 
>> kcalloc()
>> tee_shm_register()
> 
> I've just posted "[PATCH 0/7] tee: shared memory updates",
> https://lore.kernel.org/lkml/20210609102324.2222332-1-jens.wiklander@linaro.org/
> 
> Where tee_shm_alloc() is replaced by among other functions
> tee_shm_alloc_kernel_buf(). tee_shm_alloc_kernel_buf() takes care of the
> problem with TEE_SHM_DMA_BUF.
> 

Thanks Jens. The series looks fine.  Tested too.

- Allen

Re: [PATCH v3 5/7] tee: Support shm registration without dma-buf backing

[Jens Wiklander]

On Wed, Jun 09, 2021 at 08:42:25AM -0500, Tyler Hicks wrote:
[snip]
> > I've just posted "[PATCH 0/7] tee: shared memory updates",
> > https://lore.kernel.org/lkml/20210609102324.2222332-1-jens.wiklander@linaro.org/
> > 
> > Where tee_shm_alloc() is replaced by among other functions
> > tee_shm_alloc_kernel_buf(). tee_shm_alloc_kernel_buf() takes care of the
> > problem with TEE_SHM_DMA_BUF.
> 
> Thanks! At first glance, that series would take care of the last three
> patches in my kexec/kdump series.
> 
> I'm a bit worried that it is a rewrite of the shm allocator. Do you plan
> to send all of that to stable? (I mentioned earlier in this thread that
> I'm affected by these bugs in linux-5.4.y.)

No, that might be a bit much.

> Also, you and Sumit don't seem to have the same opinion on kernel
> drivers making use of tee_shm_register() for allocations that are only
> used internally. Can you comment on that?
> 
> I'm not clear on the next steps for fixing these kexec/kdump bugs in
> older releases. I appreciate any guidance here.

Neither am I be honest. You're the only one that has brought up this
problem so perhaps it's enough to focus on the stable branch you need to
have fixed.

If I've understood it correctly it's best if it's possible to
cherry-pick the fixes from mainline to the stable branch in question.
So we must make sure to get your needed patches in before any rewrites
that would make cherry-picking impossible. The rewrite I'm proposing
isn't urgent so it can be held off for a while.

Cheers,
Jens

Re: [PATCH v3 5/7] tee: Support shm registration without dma-buf backing

[Sumit Garg]

Hi Jens,

On Thu, 10 Jun 2021 at 12:48, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> On Wed, Jun 09, 2021 at 04:22:49PM +0530, Sumit Garg wrote:
> > + Rijo
> >
> > On Wed, 9 Jun 2021 at 11:16, Tyler Hicks <tyhicks@linux.microsoft.com> wrote:
> [snip]
> >
> > > - tee_shm_alloc() performs allocations using contiguous pages
> > >   from alloc_pages() while tee_shm_register() performs non-contiguous
> > >   allocations with kcalloc(). I suspect this would be fine but I don't
> > >   know the secure world side of these things well enough to assess the
> > >   risk involved with such a change on the kernel side.
> > >
> >
> > I don't think that would make any difference.
>
> Agree.
>
> >
> > > I should have mentioned this in the cover letter but my hope was that
> > > these minimal changes would be accepted and then additional work could
> > > be done to merge tee_shm_alloc() and tee_shm_register() in a way that
> > > would allow the caller to request contiguous or non-contiguous pages,
> > > fix up the additional issues mentioned above, and then adjust the
> > > call sites in ftpm and tee_bnxt_fw as appropriate.
> > >
> > > I think that's a bigger set of changes because there are several things
> > > that still confuse/concern me:
> > >
> > > - Why does tee_shm_alloc() use TEE_SHM_MAPPED while tee_shm_register()
> > >   uses TEE_SHM_KERNEL_MAPPED or TEE_SHM_USER_MAPPED? Why do all three
> > >   exist?
> >
> > AFAIK, its due the the inherent nature of tee_shm_alloc() and
> > tee_shm_register() where tee_shm_alloc() doesn't need to know whether
> > its a kernel or user-space memory since it is the one that allocates
> > whereas tee_shm_register() need to know that since it has to register
> > pre-allocated client memory.
> >
> > > - Why does tee_shm_register() unconditionally use non-contiguous
> > >   allocations without ever taking into account whether or not
> > >   OPTEE_SMC_SEC_CAP_DYNAMIC_SHM was set? It sounds like that's required
> > >   from my reading of https://optee.readthedocs.io/en/latest/architecture/core.html#noncontiguous-shared-buffers.
> >
> > Yeah, but do we have platforms in OP-TEE that don't support dynamic
> > shared memory? I guess it has become the sane default which is a
> > mandatory requirement when it comes to OP-TEE driver in u-boot.
> >
> > > - Why is TEE_SHM_REGISTER implemented at the TEE driver level when it is
> > >   specific to OP-TEE? How to better abstract that away?
> > >
> >
> > I would like you to go through Section "3.2.4. Shared Memory" in TEE
> > Client API Specification. There are two standard ways for shared
> > memory approach with TEE:
> >
> > 1. A Shared Memory block can either be existing Client Application
> > memory (kernel driver in our case) which is subsequently registered
> > with the TEE Client API (using tee_shm_register() in our case).
> >
> > 2. Or memory which is allocated on behalf of the Client Application
> > using the TEE
> > Client API (using tee_shm_alloc() in our case).
> >
> > > Let me know if you agree with the more minimal approach that I took for
> > > these bug fix series or still feel like tee_shm_register() should be
> > > fixed up so that it is usable. Thanks!
> >
> > From drivers perspective I think the change should be:
> >
> > tee_shm_alloc()
> >
> > to
> >
> > kcalloc()
> > tee_shm_register()
>
> I had another approach in mind in "[PATCH 0/7] tee: shared memory updates",
> https://lore.kernel.org/lkml/20210609102324.2222332-1-jens.wiklander@linaro.org/
>
> The flags needed by tee_shm_alloc() and tee_shm_register() aren't
> very intuitive and in fact only accept quite few combinations. So my
> idea was to hide those flags from callers outside of the TEE subsystem
> with tee_shm_alloc_kernel_buf().
>

That looks like a good idea to hide flags from users. BTW, my only
objection earlier with Tyler's and your patch-set is the usage of
TEE_SHM_REGISTER flag in generic TEE methods: tee_shm_alloc*. AFAIU,
the only reason for such an additional flag is in case of OP-TEE only
because the OP-TEE driver could implement allocated shared memory via
re-using dynamic shared memory approach as well. And that additional
flag is only needed to differentiate that OP-TEE driver's private
memory shouldn't be registered with OP-TEE. If this understanding is
correct then we should introduce a separate flag as TEE_SHM_PRIV that
should only be set inside tee_shm_alloc_anon_kernel_buf().

As otherwise passing TEE_SHM_REGISTER flag for shared memory alloc API
for other TEEs like AMD-TEE etc. would be useless.

> The approach with tee_shm_register() you suggest above has the drawback
> that the TEE driver is forced to be able to handle any kernel memory.

That's the value-add in the problem that Tyler is trying to resolve
that driver should be able to free up the memory as needed as a
private buffer.

> This is OK with OP-TEE and dynamic shared memory enabled, but there are
> platforms where dynamic shared memory isn't enabled. In those case must
> the memory be allocated from a special pool.

Is there any limitation for those platforms to not support dynamic
shared memory in OP-TEE? If there isn't then we should able to handle
this via match for TEE_GEN_CAP_REG_MEM in the ftpm_tee_match() and
optee_ctx_match() APIs.

>
> Do you see any problem with instead replacing tee_shm_alloc()
> with tee_shm_alloc_kernel_buf()?

I don't see any problems apart from one mentioned above.

-Sumit

>
> Cheers,
> Jens

Re: [PATCH v3 5/7] tee: Support shm registration without dma-buf backing

[Tyler Hicks]

On 2021-06-10 09:34:24, Jens Wiklander wrote:
> On Wed, Jun 09, 2021 at 08:51:04AM -0500, Tyler Hicks wrote:
> [snip]
> > > > I've just posted "[PATCH 0/7] tee: shared memory updates",
> > > > https://lore.kernel.org/lkml/20210609102324.2222332-1-jens.wiklander@linaro.org/
> > > > 
> > > > Where tee_shm_alloc() is replaced by among other functions
> > > > tee_shm_alloc_kernel_buf(). tee_shm_alloc_kernel_buf() takes care of the
> > > > problem with TEE_SHM_DMA_BUF.
> > > 
> > > Thanks! At first glance, that series would take care of the last three
> > > patches in my kexec/kdump series.
> > 
> > Correction: Your series would not completely take care of the last three
> > patches in my kexec/kdump series because your series doesn't implement
> > the .shutdown() hook for tee_bnxt_fw.
> > 
> > Does it make sense to take my series first and then rebase your series
> > on top of it? That would allow my fixes to flow back to stable, then
> > your changes would greatly clean up the implementation in future
> > releases.
> 
> Yes, we could try that. I'd like to see tee_shm_alloc_kernel_buf() being
> used instead of tee_shm_alloc() in ftpm_tee_probe() and
> tee_bnxt_fw_probe(). So it would be great if you could include "tee: add
> tee_shm_alloc_kernel_buf()" in your patch set.

That would be no problem at all. I like that idea and I've prepared a v4
with that change. I'll send it out shortly once I've finished testing.

> My patch set would then shrink a bit. By the way, thanks for reviewing
> it.

No problem! I feel like I'm starting to understand the TEE subsystem and
OP-TEE driver a bit so I'm happy to help out.

Tyler

> 
> Cheers,
> Jens
> 

Re: [PATCH v3 5/7] tee: Support shm registration without dma-buf backing

[Tyler Hicks]

On 2021-06-10 09:49:48, Jens Wiklander wrote:
> On Wed, Jun 09, 2021 at 08:42:25AM -0500, Tyler Hicks wrote:
> [snip]
> > > I've just posted "[PATCH 0/7] tee: shared memory updates",
> > > https://lore.kernel.org/lkml/20210609102324.2222332-1-jens.wiklander@linaro.org/
> > > 
> > > Where tee_shm_alloc() is replaced by among other functions
> > > tee_shm_alloc_kernel_buf(). tee_shm_alloc_kernel_buf() takes care of the
> > > problem with TEE_SHM_DMA_BUF.
> > 
> > Thanks! At first glance, that series would take care of the last three
> > patches in my kexec/kdump series.
> > 
> > I'm a bit worried that it is a rewrite of the shm allocator. Do you plan
> > to send all of that to stable? (I mentioned earlier in this thread that
> > I'm affected by these bugs in linux-5.4.y.)
> 
> No, that might be a bit much.
> 
> > Also, you and Sumit don't seem to have the same opinion on kernel
> > drivers making use of tee_shm_register() for allocations that are only
> > used internally. Can you comment on that?
> > 
> > I'm not clear on the next steps for fixing these kexec/kdump bugs in
> > older releases. I appreciate any guidance here.
> 
> Neither am I be honest. You're the only one that has brought up this
> problem so perhaps it's enough to focus on the stable branch you need to
> have fixed.

I've already added Fixes tags to all of my patches. If you are
comfortable with them going to stable, you'd add
'Cc: stable@vger.kernel.org' to them if/when you merge them so that the
stable team will ensure that they're applied.

Note that I'm not the only person that brought up this bug:

 https://github.com/OP-TEE/optee_os/issues/3637

Once I started digging in, I realized that there were more kexec/kdump
bugs and the series grew.

> If I've understood it correctly it's best if it's possible to
> cherry-pick the fixes from mainline to the stable branch in question.
> So we must make sure to get your needed patches in before any rewrites
> that would make cherry-picking impossible. The rewrite I'm proposing
> isn't urgent so it can be held off for a while.

Thanks for holding off. I'll be quick on my revisions so that you don't
have to moth ball your series for too much longer.

Tyler

> 
> Cheers,
> Jens
>