[PATCH v5 0/8] tee: Improve support for kexec and kdump

[PATCH v5 0/8] tee: Improve support for kexec and kdump

[Tyler Hicks]

v5:
- Picked up Reviewed-by's from Jens.
- Added 'Cc: stable@vger.kernel.org' to all commits as this is intended
  to be a bug fix series. I'm happy to sort out backports with the
  stable team.
- Got rid of the bool is_mapped parameter of optee_disable_shm_cache()
  by abstracting out the function with two wrappers. One
  (optee_disable_shm_cache()) for normal case where the shm cache is
  fully mapped and another (optee_disable_unmapped_shm_cache()) for the
  unusual case of the shm cache having potentially invalid entries.
- Replaced my previous 'tee: Support kernel shm registration without
  dma-buf' patch with a cleaner implementation ('tee: Correct
  inappropriate usage of TEE_SHM_DMA_BUF flag') from Sumit Garg.
v4: https://lore.kernel.org/lkml/20210610210913.536081-1-tyhicks@linux.microsoft.com/
v3: https://lore.kernel.org/lkml/20210609002326.210024-1-tyhicks@linux.microsoft.com/
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 TEE shm, session, and context during kexec

Jens Wiklander (1):
  tee: add tee_shm_alloc_kernel_buf()

Sumit Garg (1):
  tee: Correct inappropriate usage of TEE_SHM_DMA_BUF flag

Tyler Hicks (4):
  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
  tpm_ftpm_tee: Free and unregister TEE shared memory during kexec

 drivers/char/tpm/tpm_ftpm_tee.c         |  8 ++---
 drivers/firmware/broadcom/tee_bnxt_fw.c | 14 ++++++--
 drivers/tee/optee/call.c                | 38 +++++++++++++++++++---
 drivers/tee/optee/core.c                | 43 ++++++++++++++++++++++++-
 drivers/tee/optee/optee_private.h       |  1 +
 drivers/tee/optee/rpc.c                 |  5 +--
 drivers/tee/optee/shm_pool.c            | 20 +++++++++---
 drivers/tee/tee_shm.c                   | 20 +++++++++++-
 include/linux/tee_drv.h                 |  2 ++
 9 files changed, 132 insertions(+), 19 deletions(-)

-- 
2.25.1

[PATCH v5 1/8] 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")
Cc: stable@vger.kernel.org
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 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 v5 2/8] 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.

Cc: stable@vger.kernel.org
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 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 v5 3/8] 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

Cc: stable@vger.kernel.org
Signed-off-by: Allen Pais <apais@linux.microsoft.com>
Reviewed-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 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 v5 4/8] 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.

Cc: stable@vger.kernel.org
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
 drivers/tee/optee/call.c          | 36 ++++++++++++++++++++++++++++---
 drivers/tee/optee/core.c          |  9 ++++++++
 drivers/tee/optee/optee_private.h |  1 +
 3 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
index 6e6eb836e9b6..387e94768182 100644
--- a/drivers/tee/optee/call.c
+++ b/drivers/tee/optee/call.c
@@ -416,11 +416,13 @@ void optee_enable_shm_cache(struct optee *optee)
 }
 
 /**
- * optee_disable_shm_cache() - Disables caching of some shared memory allocation
- *			      in OP-TEE
+ * __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)
+static 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);
@@ -449,6 +458,27 @@ void optee_disable_shm_cache(struct optee *optee)
 	optee_cq_wait_final(&optee->call_queue, &w);
 }
 
+/**
+ * optee_disable_shm_cache() - Disables caching of mapped shared memory
+ *                             allocations in OP-TEE
+ * @optee:	main service struct
+ */
+void optee_disable_shm_cache(struct optee *optee)
+{
+	return __optee_disable_shm_cache(optee, true);
+}
+
+/**
+ * optee_disable_unmapped_shm_cache() - Disables caching of shared memory
+ *                                      allocations in OP-TEE which are not
+ *                                      currently mapped
+ * @optee:	main service struct
+ */
+void optee_disable_unmapped_shm_cache(struct optee *optee)
+{
+	return __optee_disable_shm_cache(optee, false);
+}
+
 #define PAGELIST_ENTRIES_PER_PAGE				\
 	((OPTEE_MSG_NONCONTIG_PAGE_SIZE / sizeof(u64)) - 1)
 
diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index 0987074d7ed0..651d49b53d3b 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -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_unmapped_shm_cache(optee);
+
 	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..dbdd367be156 100644
--- a/drivers/tee/optee/optee_private.h
+++ b/drivers/tee/optee/optee_private.h
@@ -159,6 +159,7 @@ 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_unmapped_shm_cache(struct optee *optee);
 
 int optee_shm_register(struct tee_context *ctx, struct tee_shm *shm,
 		       struct page **pages, size_t num_pages,
-- 
2.25.1

Re: [PATCH v5 4/8] optee: Clear stale cache entries during initialization

[Jens Wiklander]

On Tue, Jun 15, 2021 at 12:33 AM Tyler Hicks
<tyhicks@linux.microsoft.com> wrote:
>
> 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.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> ---
>  drivers/tee/optee/call.c          | 36 ++++++++++++++++++++++++++++---
>  drivers/tee/optee/core.c          |  9 ++++++++
>  drivers/tee/optee/optee_private.h |  1 +
>  3 files changed, 43 insertions(+), 3 deletions(-)

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

[PATCH v5 5/8] tee: add tee_shm_alloc_kernel_buf()

[Tyler Hicks]

From: Jens Wiklander <jens.wiklander@linaro.org>

Adds a new function tee_shm_alloc_kernel_buf() to allocate shared memory
from a kernel driver. This function can later be made more lightweight
by unnecessary dma-buf export.

Cc: stable@vger.kernel.org
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
 drivers/tee/tee_shm.c   | 18 ++++++++++++++++++
 include/linux/tee_drv.h |  1 +
 2 files changed, 19 insertions(+)

diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
index 00472f5ce22e..c65e44707cd6 100644
--- a/drivers/tee/tee_shm.c
+++ b/drivers/tee/tee_shm.c
@@ -193,6 +193,24 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
 }
 EXPORT_SYMBOL_GPL(tee_shm_alloc);
 
+/**
+ * tee_shm_alloc_kernel_buf() - Allocate shared memory for kernel buffer
+ * @ctx:	Context that allocates the shared memory
+ * @size:	Requested size of shared memory
+ *
+ * The returned memory registered in secure world and is suitable to be
+ * passed as a memory buffer in parameter argument to
+ * tee_client_invoke_func(). The memory allocated is later freed with a
+ * call to tee_shm_free().
+ *
+ * @returns a pointer to 'struct tee_shm'
+ */
+struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size)
+{
+	return tee_shm_alloc(ctx, size, TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
+}
+EXPORT_SYMBOL_GPL(tee_shm_alloc_kernel_buf);
+
 struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr,
 				 size_t length, u32 flags)
 {
diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
index 54269e47ac9a..8990f7628387 100644
--- a/include/linux/tee_drv.h
+++ b/include/linux/tee_drv.h
@@ -332,6 +332,7 @@ void *tee_get_drvdata(struct tee_device *teedev);
  * @returns a pointer to 'struct tee_shm'
  */
 struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags);
+struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size);
 
 /**
  * tee_shm_register() - Register shared memory buffer
-- 
2.25.1

[PATCH v5 6/8] tee: Correct inappropriate usage of TEE_SHM_DMA_BUF flag

[Tyler Hicks]

From: Sumit Garg <sumit.garg@linaro.org>

Currently TEE_SHM_DMA_BUF flag has been inappropriately used to not
register shared memory allocated for private usage by underlying TEE
driver: OP-TEE in this case. So rather add a new flag as TEE_SHM_PRIV
that can be utilized by underlying TEE drivers for private allocation
and usage of shared memory.

With this corrected, allow tee_shm_alloc_kernel_buf() to allocate a
shared memory region without the backing of dma-buf.

Cc: stable@vger.kernel.org
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
Co-developed-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
 drivers/tee/optee/call.c     | 2 +-
 drivers/tee/optee/core.c     | 3 ++-
 drivers/tee/optee/rpc.c      | 5 +++--
 drivers/tee/optee/shm_pool.c | 8 ++++++--
 drivers/tee/tee_shm.c        | 4 ++--
 include/linux/tee_drv.h      | 1 +
 6 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
index 387e94768182..945f03da0223 100644
--- a/drivers/tee/optee/call.c
+++ b/drivers/tee/optee/call.c
@@ -184,7 +184,7 @@ static struct tee_shm *get_msg_arg(struct tee_context *ctx, size_t num_params,
 	struct optee_msg_arg *ma;
 
 	shm = tee_shm_alloc(ctx, OPTEE_MSG_GET_ARG_SIZE(num_params),
-			    TEE_SHM_MAPPED);
+			    TEE_SHM_MAPPED | TEE_SHM_PRIV);
 	if (IS_ERR(shm))
 		return shm;
 
diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index 651d49b53d3b..5ce13b099d7d 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -278,7 +278,8 @@ static void optee_release(struct tee_context *ctx)
 	if (!ctxdata)
 		return;
 
-	shm = tee_shm_alloc(ctx, sizeof(struct optee_msg_arg), TEE_SHM_MAPPED);
+	shm = tee_shm_alloc(ctx, sizeof(struct optee_msg_arg),
+			    TEE_SHM_MAPPED | TEE_SHM_PRIV);
 	if (!IS_ERR(shm)) {
 		arg = tee_shm_get_va(shm, 0);
 		/*
diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
index 1849180b0278..efbaff7ad7e5 100644
--- a/drivers/tee/optee/rpc.c
+++ b/drivers/tee/optee/rpc.c
@@ -314,7 +314,7 @@ static void handle_rpc_func_cmd_shm_alloc(struct tee_context *ctx,
 		shm = cmd_alloc_suppl(ctx, sz);
 		break;
 	case OPTEE_RPC_SHM_TYPE_KERNEL:
-		shm = tee_shm_alloc(ctx, sz, TEE_SHM_MAPPED);
+		shm = tee_shm_alloc(ctx, sz, TEE_SHM_MAPPED | TEE_SHM_PRIV);
 		break;
 	default:
 		arg->ret = TEEC_ERROR_BAD_PARAMETERS;
@@ -502,7 +502,8 @@ void optee_handle_rpc(struct tee_context *ctx, struct optee_rpc_param *param,
 
 	switch (OPTEE_SMC_RETURN_GET_RPC_FUNC(param->a0)) {
 	case OPTEE_SMC_RPC_FUNC_ALLOC:
-		shm = tee_shm_alloc(ctx, param->a1, TEE_SHM_MAPPED);
+		shm = tee_shm_alloc(ctx, param->a1,
+				    TEE_SHM_MAPPED | TEE_SHM_PRIV);
 		if (!IS_ERR(shm) && !tee_shm_get_pa(shm, 0, &pa)) {
 			reg_pair_from_64(&param->a1, &param->a2, pa);
 			reg_pair_from_64(&param->a4, &param->a5,
diff --git a/drivers/tee/optee/shm_pool.c b/drivers/tee/optee/shm_pool.c
index da06ce9b9313..c41a9a501a6e 100644
--- a/drivers/tee/optee/shm_pool.c
+++ b/drivers/tee/optee/shm_pool.c
@@ -27,7 +27,11 @@ 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) {
+	/*
+	 * Shared memory private to the OP-TEE driver doesn't need
+	 * to be registered with OP-TEE.
+	 */
+	if (!(shm->flags & TEE_SHM_PRIV)) {
 		unsigned int nr_pages = 1 << order, i;
 		struct page **pages;
 
@@ -60,7 +64,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_PRIV))
 		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 c65e44707cd6..8a9384a64f3e 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_PRIV))) {
 		dev_err(teedev->dev.parent, "invalid shm flags 0x%x", flags);
 		return ERR_PTR(-EINVAL);
 	}
@@ -207,7 +207,7 @@ EXPORT_SYMBOL_GPL(tee_shm_alloc);
  */
 struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size)
 {
-	return tee_shm_alloc(ctx, size, TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
+	return tee_shm_alloc(ctx, size, TEE_SHM_MAPPED);
 }
 EXPORT_SYMBOL_GPL(tee_shm_alloc_kernel_buf);
 
diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
index 8990f7628387..3ebfea0781f1 100644
--- a/include/linux/tee_drv.h
+++ b/include/linux/tee_drv.h
@@ -27,6 +27,7 @@
 #define TEE_SHM_USER_MAPPED	BIT(4)  /* Memory mapped in user space */
 #define TEE_SHM_POOL		BIT(5)  /* Memory allocated from pool */
 #define TEE_SHM_KERNEL_MAPPED	BIT(6)  /* Memory mapped in kernel space */
+#define TEE_SHM_PRIV		BIT(7)  /* Memory private to TEE driver */
 
 struct device;
 struct tee_device;
-- 
2.25.1

Re: [PATCH v5 6/8] tee: Correct inappropriate usage of TEE_SHM_DMA_BUF flag

[Jens Wiklander]

On Tue, Jun 15, 2021 at 12:33 AM Tyler Hicks
<tyhicks@linux.microsoft.com> wrote:
>
> From: Sumit Garg <sumit.garg@linaro.org>
>
> Currently TEE_SHM_DMA_BUF flag has been inappropriately used to not
> register shared memory allocated for private usage by underlying TEE
> driver: OP-TEE in this case. So rather add a new flag as TEE_SHM_PRIV
> that can be utilized by underlying TEE drivers for private allocation
> and usage of shared memory.
>
> With this corrected, allow tee_shm_alloc_kernel_buf() to allocate a
> shared memory region without the backing of dma-buf.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> Co-developed-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> ---
>  drivers/tee/optee/call.c     | 2 +-
>  drivers/tee/optee/core.c     | 3 ++-
>  drivers/tee/optee/rpc.c      | 5 +++--
>  drivers/tee/optee/shm_pool.c | 8 ++++++--
>  drivers/tee/tee_shm.c        | 4 ++--
>  include/linux/tee_drv.h      | 1 +
>  6 files changed, 15 insertions(+), 8 deletions(-)

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

[PATCH v5 7/8] tpm_ftpm_tee: Free and unregister TEE 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.

Use tee_shm_alloc_kernel_buf() to avoid dma-buf backed shared memory
allocation so that tee_shm_free() can directly call tee_shm_release().
This will ensure that the shm can be freed and unregistered during a
kexec operation.

Fixes: 09e574831b27 ("tpm/tpm_ftpm_tee: A driver for firmware TPM running inside TEE")
Fixes: 1760eb689ed6 ("tpm/tpm_ftpm_tee: add shutdown call back")
Cc: stable@vger.kernel.org
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
 drivers/char/tpm/tpm_ftpm_tee.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
index 2ccdf8ac6994..6e3235565a4d 100644
--- a/drivers/char/tpm/tpm_ftpm_tee.c
+++ b/drivers/char/tpm/tpm_ftpm_tee.c
@@ -254,11 +254,11 @@ static int ftpm_tee_probe(struct device *dev)
 	pvt_data->session = sess_arg.session;
 
 	/* 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);
+	pvt_data->shm = tee_shm_alloc_kernel_buf(pvt_data->ctx,
+						 MAX_COMMAND_SIZE +
+						 MAX_RESPONSE_SIZE);
 	if (IS_ERR(pvt_data->shm)) {
-		dev_err(dev, "%s: tee_shm_alloc failed\n", __func__);
+		dev_err(dev, "%s: tee_shm_alloc_kernel_buf failed\n", __func__);
 		rc = -ENOMEM;
 		goto out_shm_alloc;
 	}
-- 
2.25.1

[PATCH v5 8/8] firmware: tee_bnxt: Release TEE 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
fw_shm_pool. 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.

Use tee_shm_alloc_kernel_buf() to avoid dma-buf backed shared memory
allocation so that tee_shm_free() can directly call tee_shm_release().
This will ensure that the shm can be freed and unregistered during a
kexec operation.

Fixes: 246880958ac9 ("firmware: broadcom: add OP-TEE based BNXT f/w manager")
Cc: stable@vger.kernel.org
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 | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/broadcom/tee_bnxt_fw.c b/drivers/firmware/broadcom/tee_bnxt_fw.c
index ed10da5313e8..a5bf4c3f6dc7 100644
--- a/drivers/firmware/broadcom/tee_bnxt_fw.c
+++ b/drivers/firmware/broadcom/tee_bnxt_fw.c
@@ -212,10 +212,9 @@ 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);
+	fw_shm_pool = tee_shm_alloc_kernel_buf(pvt_data.ctx, MAX_SHM_MEM_SZ);
 	if (IS_ERR(fw_shm_pool)) {
-		dev_err(pvt_data.dev, "tee_shm_alloc failed\n");
+		dev_err(pvt_data.dev, "tee_shm_alloc_kernel_buf failed\n");
 		err = PTR_ERR(fw_shm_pool);
 		goto out_sess;
 	}
@@ -242,6 +241,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 +264,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 v5 0/8] tee: Improve support for kexec and kdump

[Sumit Garg]

Hi Tyler,

On Tue, 15 Jun 2021 at 04:03, Tyler Hicks <tyhicks@linux.microsoft.com> wrote:
>
> v5:
> - Picked up Reviewed-by's from Jens.
> - Added 'Cc: stable@vger.kernel.org' to all commits as this is intended
>   to be a bug fix series. I'm happy to sort out backports with the
>   stable team.
> - Got rid of the bool is_mapped parameter of optee_disable_shm_cache()
>   by abstracting out the function with two wrappers. One
>   (optee_disable_shm_cache()) for normal case where the shm cache is
>   fully mapped and another (optee_disable_unmapped_shm_cache()) for the
>   unusual case of the shm cache having potentially invalid entries.
> - Replaced my previous 'tee: Support kernel shm registration without
>   dma-buf' patch with a cleaner implementation ('tee: Correct
>   inappropriate usage of TEE_SHM_DMA_BUF flag') from Sumit Garg.
> v4: https://lore.kernel.org/lkml/20210610210913.536081-1-tyhicks@linux.microsoft.com/
> v3: https://lore.kernel.org/lkml/20210609002326.210024-1-tyhicks@linux.microsoft.com/
> 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 looks good to me. Feel free to add:

Reviewed-by: Sumit Garg <sumit.garg@linaro.org>

-Sumit

> 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 TEE shm, session, and context during kexec
>
> Jens Wiklander (1):
>   tee: add tee_shm_alloc_kernel_buf()
>
> Sumit Garg (1):
>   tee: Correct inappropriate usage of TEE_SHM_DMA_BUF flag
>
> Tyler Hicks (4):
>   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
>   tpm_ftpm_tee: Free and unregister TEE shared memory during kexec
>
>  drivers/char/tpm/tpm_ftpm_tee.c         |  8 ++---
>  drivers/firmware/broadcom/tee_bnxt_fw.c | 14 ++++++--
>  drivers/tee/optee/call.c                | 38 +++++++++++++++++++---
>  drivers/tee/optee/core.c                | 43 ++++++++++++++++++++++++-
>  drivers/tee/optee/optee_private.h       |  1 +
>  drivers/tee/optee/rpc.c                 |  5 +--
>  drivers/tee/optee/shm_pool.c            | 20 +++++++++---
>  drivers/tee/tee_shm.c                   | 20 +++++++++++-
>  include/linux/tee_drv.h                 |  2 ++
>  9 files changed, 132 insertions(+), 19 deletions(-)
>
> --
> 2.25.1
>

Re: [PATCH v5 0/8] tee: Improve support for kexec and kdump

[Tyler Hicks]

On 2021-06-15 10:02:39, Sumit Garg wrote:
> Hi Tyler,
> 
> On Tue, 15 Jun 2021 at 04:03, Tyler Hicks <tyhicks@linux.microsoft.com> wrote:
> >
> > v5:
> > - Picked up Reviewed-by's from Jens.
> > - Added 'Cc: stable@vger.kernel.org' to all commits as this is intended
> >   to be a bug fix series. I'm happy to sort out backports with the
> >   stable team.
> > - Got rid of the bool is_mapped parameter of optee_disable_shm_cache()
> >   by abstracting out the function with two wrappers. One
> >   (optee_disable_shm_cache()) for normal case where the shm cache is
> >   fully mapped and another (optee_disable_unmapped_shm_cache()) for the
> >   unusual case of the shm cache having potentially invalid entries.
> > - Replaced my previous 'tee: Support kernel shm registration without
> >   dma-buf' patch with a cleaner implementation ('tee: Correct
> >   inappropriate usage of TEE_SHM_DMA_BUF flag') from Sumit Garg.
> > v4: https://lore.kernel.org/lkml/20210610210913.536081-1-tyhicks@linux.microsoft.com/
> > v3: https://lore.kernel.org/lkml/20210609002326.210024-1-tyhicks@linux.microsoft.com/
> > 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 looks good to me. Feel free to add:
> 
> Reviewed-by: Sumit Garg <sumit.garg@linaro.org>

Thank you for all of your help and patience as I learn about the TEE
subsystem and begin to scratch the surface of how OP-TEE works. :)

Tyler

> 
> -Sumit
> 
> > 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 TEE shm, session, and context during kexec
> >
> > Jens Wiklander (1):
> >   tee: add tee_shm_alloc_kernel_buf()
> >
> > Sumit Garg (1):
> >   tee: Correct inappropriate usage of TEE_SHM_DMA_BUF flag
> >
> > Tyler Hicks (4):
> >   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
> >   tpm_ftpm_tee: Free and unregister TEE shared memory during kexec
> >
> >  drivers/char/tpm/tpm_ftpm_tee.c         |  8 ++---
> >  drivers/firmware/broadcom/tee_bnxt_fw.c | 14 ++++++--
> >  drivers/tee/optee/call.c                | 38 +++++++++++++++++++---
> >  drivers/tee/optee/core.c                | 43 ++++++++++++++++++++++++-
> >  drivers/tee/optee/optee_private.h       |  1 +
> >  drivers/tee/optee/rpc.c                 |  5 +--
> >  drivers/tee/optee/shm_pool.c            | 20 +++++++++---
> >  drivers/tee/tee_shm.c                   | 20 +++++++++++-
> >  include/linux/tee_drv.h                 |  2 ++
> >  9 files changed, 132 insertions(+), 19 deletions(-)
> >
> > --
> > 2.25.1
> >
> 

Re: [PATCH v5 0/8] tee: Improve support for kexec and kdump

[Jens Wiklander]

Hi Tyler,

On Tue, Jun 15, 2021 at 12:33 AM Tyler Hicks
<tyhicks@linux.microsoft.com> wrote:
>
> v5:
> - Picked up Reviewed-by's from Jens.
> - Added 'Cc: stable@vger.kernel.org' to all commits as this is intended
>   to be a bug fix series. I'm happy to sort out backports with the
>   stable team.
> - Got rid of the bool is_mapped parameter of optee_disable_shm_cache()
>   by abstracting out the function with two wrappers. One
>   (optee_disable_shm_cache()) for normal case where the shm cache is
>   fully mapped and another (optee_disable_unmapped_shm_cache()) for the
>   unusual case of the shm cache having potentially invalid entries.
> - Replaced my previous 'tee: Support kernel shm registration without
>   dma-buf' patch with a cleaner implementation ('tee: Correct
>   inappropriate usage of TEE_SHM_DMA_BUF flag') from Sumit Garg.
> v4: https://lore.kernel.org/lkml/20210610210913.536081-1-tyhicks@linux.microsoft.com/
> v3: https://lore.kernel.org/lkml/20210609002326.210024-1-tyhicks@linux.microsoft.com/
> 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 TEE shm, session, and context during kexec
>
> Jens Wiklander (1):
>   tee: add tee_shm_alloc_kernel_buf()
>
> Sumit Garg (1):
>   tee: Correct inappropriate usage of TEE_SHM_DMA_BUF flag
>
> Tyler Hicks (4):
>   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
>   tpm_ftpm_tee: Free and unregister TEE shared memory during kexec
>
>  drivers/char/tpm/tpm_ftpm_tee.c         |  8 ++---
>  drivers/firmware/broadcom/tee_bnxt_fw.c | 14 ++++++--
>  drivers/tee/optee/call.c                | 38 +++++++++++++++++++---
>  drivers/tee/optee/core.c                | 43 ++++++++++++++++++++++++-
>  drivers/tee/optee/optee_private.h       |  1 +
>  drivers/tee/optee/rpc.c                 |  5 +--
>  drivers/tee/optee/shm_pool.c            | 20 +++++++++---
>  drivers/tee/tee_shm.c                   | 20 +++++++++++-
>  include/linux/tee_drv.h                 |  2 ++
>  9 files changed, 132 insertions(+), 19 deletions(-)
>
> --
 > 2.25.1
>

It looks like we're almost done now. Thanks for your patience to see
this through.

I suppose it makes most sense to take this via my tree, but before I
can do that I'll need acks from the maintainers of
drivers/char/tpm/tpm_ftpm_tee.c ("tpm_ftpm_tee: Free and unregister
TEE shared memory during kexec") and
drivers/firmware/broadcom/tee_bnxt_fw.c ("firmware: tee_bnxt: Release
TEE shm, session, and context during kexec").

Cheers,
Jens

Re: [PATCH v5 0/8] tee: Improve support for kexec and kdump

[Tyler Hicks]

On 2021-06-15 09:23:25, Jens Wiklander wrote:
> It looks like we're almost done now. Thanks for your patience to see
> this through.
> 
> I suppose it makes most sense to take this via my tree, but before I
> can do that I'll need acks from the maintainers of
> drivers/char/tpm/tpm_ftpm_tee.c ("tpm_ftpm_tee: Free and unregister
> TEE shared memory during kexec") and
> drivers/firmware/broadcom/tee_bnxt_fw.c ("firmware: tee_bnxt: Release
> TEE shm, session, and context during kexec").

@Rafał Miłecki, we just need an ack from you for the tee_bnxt_fw.c
change:

 https://lore.kernel.org/lkml/20210614223317.999867-9-tyhicks@linux.microsoft.com/

Jarkko just gave us an ack for tpm_ftpm_tee.c on the v4 series:

 https://lore.kernel.org/lkml/20210615130411.hvpnaxnhimjloiz3@kernel.org/

The patch tpm_ftpm_tee.c patch didn't change from v4 to v5 and the
underlying concept of not using TEE_SHM_DMA_BUF remained the same so I
don't think we need a separate v5 ack.

Tyler

> 
> Cheers,
> Jens
> 

Re: [PATCH v5 0/8] tee: Improve support for kexec and kdump

[Florian Fainelli]

On 6/15/2021 6:37 AM, Tyler Hicks wrote:
> On 2021-06-15 09:23:25, Jens Wiklander wrote:
>> It looks like we're almost done now. Thanks for your patience to see
>> this through.
>>
>> I suppose it makes most sense to take this via my tree, but before I
>> can do that I'll need acks from the maintainers of
>> drivers/char/tpm/tpm_ftpm_tee.c ("tpm_ftpm_tee: Free and unregister
>> TEE shared memory during kexec") and
>> drivers/firmware/broadcom/tee_bnxt_fw.c ("firmware: tee_bnxt: Release
>> TEE shm, session, and context during kexec").
> 
> @Rafał Miłecki, we just need an ack from you for the tee_bnxt_fw.c
> change:
> 
>  https://lore.kernel.org/lkml/20210614223317.999867-9-tyhicks@linux.microsoft.com/

Rafal is listed as the maintainer of drivers/firmware/broadcom/ but he
did not author that file, Vikas did, adding him.
-- 
Florian

Re: [PATCH v5 0/8] tee: Improve support for kexec and kdump

[Jens Wiklander]

Hi Tyler,

With Florian's Ack on "PATCH v5 8/8] firmware: tee_bnxt: Release TEE
shm, session, and context during kexec" I think we have everything we
need for a pull request. I've pushed the patches to
https://git.linaro.org/people/jens.wiklander/linux-tee.git/log/?h=tyhicks_kexec_v5

Would you mind testing that branch to make sure it still works as
intended? I'd appreciate if you could double check the tags too. I'll
send a pull request for v5.14 later this week if everything is OK.

Thanks,
Jens

On Tue, Jun 15, 2021 at 4:16 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 6/15/2021 6:37 AM, Tyler Hicks wrote:
> > On 2021-06-15 09:23:25, Jens Wiklander wrote:
> >> It looks like we're almost done now. Thanks for your patience to see
> >> this through.
> >>
> >> I suppose it makes most sense to take this via my tree, but before I
> >> can do that I'll need acks from the maintainers of
> >> drivers/char/tpm/tpm_ftpm_tee.c ("tpm_ftpm_tee: Free and unregister
> >> TEE shared memory during kexec") and
> >> drivers/firmware/broadcom/tee_bnxt_fw.c ("firmware: tee_bnxt: Release
> >> TEE shm, session, and context during kexec").
> >
> > @Rafał Miłecki, we just need an ack from you for the tee_bnxt_fw.c
> > change:
> >
> >  https://lore.kernel.org/lkml/20210614223317.999867-9-tyhicks@linux.microsoft.com/
>
> Rafal is listed as the maintainer of drivers/firmware/broadcom/ but he
> did not author that file, Vikas did, adding him.
> --
> Florian