OP-TEE December 2021

op-tee@lists.trustedfirmware.org

17 participants
15 discussions

[PATCH] tee: optee: Fix incorrect page free bug

by Sumit Garg

Pointer to the allocated pages (struct page *page) has already progressed towards the end of allocation. It is incorrect to perform __free_pages(page, order) using this pointer as we would free any arbitrary pages. Fix this by stop modifying the page pointer. Fixes: ec185dd3ab25 ("optee: Fix memory leak when failing to register shm pages") Reported-by: Patrik Lantz <patrik.lantz(a)axis.com> Signed-off-by: Sumit Garg <sumit.garg(a)linaro.org> --- drivers/tee/optee/core.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c index ab2edfcc6c70..2a66a5203d2f 100644 --- a/drivers/tee/optee/core.c +++ b/drivers/tee/optee/core.c @@ -48,10 +48,8 @@ int optee_pool_op_alloc_helper(struct tee_shm_pool_mgr *poolm, goto err; } - for (i = 0; i < nr_pages; i++) { - pages[i] = page; - page++; - } + for (i = 0; i < nr_pages; i++) + pages[i] = page + i; shm->flags |= TEE_SHM_REGISTER; rc = shm_register(shm->ctx, shm, pages, nr_pages, -- 2.25.1

2 years, 11 months

[PATCH] tee: handle lookup of shm with reference count 0

by Jens Wiklander

Since the tee subsystem does not keep a strong reference to its idle shared memory buffers, it races with other threads that try to destroy a shared memory through a close of its dma-buf fd or by unmapping the memory. In tee_shm_get_from_id() when a lookup in teedev->idr has been successful, it is possible that the tee_shm is in the dma-buf teardown path, but that path is blocked by the teedev mutex. Since we don't have an API to tell if the tee_shm is in the dma-buf teardown path or not we must find another way of detecting this condition. Fix this by doing the reference counting directly on the tee_shm using a new refcount_t refcount field. dma-buf is replaced by using anon_inode_getfd() instead, this separates the life-cycle of the underlying file from the tee_shm. tee_shm_put() is updated to hold the mutex when decreasing the refcount to 0 and then remove the tee_shm from teedev->idr before releasing the mutex. This means that the tee_shm can never be found unless it has a refcount larger than 0. Fixes: 967c9cca2cc5 ("tee: generic TEE subsystem") Cc: stable(a)vger.kernel.org Reviewed-by: Lars Persson <larper(a)axis.com> Reviewed-by: Sumit Garg <sumit.garg(a)linaro.org> Reported-by: Patrik Lantz <patrik.lantz(a)axis.com> Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> --- drivers/tee/tee_shm.c | 174 +++++++++++++++------------------------- include/linux/tee_drv.h | 2 +- 2 files changed, 67 insertions(+), 109 deletions(-) diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index 8a8deb95e918..0c82cf981c46 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -1,20 +1,17 @@ // SPDX-License-Identifier: GPL-2.0-only /* - * Copyright (c) 2015-2016, Linaro Limited + * Copyright (c) 2015-2021, Linaro Limited */ +#include <linux/anon_inodes.h> #include <linux/device.h> -#include <linux/dma-buf.h> -#include <linux/fdtable.h> #include <linux/idr.h> +#include <linux/mm.h> #include <linux/sched.h> #include <linux/slab.h> #include <linux/tee_drv.h> #include <linux/uio.h> -#include <linux/module.h> #include "tee_private.h" -MODULE_IMPORT_NS(DMA_BUF); - static void release_registered_pages(struct tee_shm *shm) { if (shm->pages) { @@ -31,16 +28,8 @@ static void release_registered_pages(struct tee_shm *shm) } } -static void tee_shm_release(struct tee_shm *shm) +static void tee_shm_release(struct tee_device *teedev, struct tee_shm *shm) { - struct tee_device *teedev = shm->ctx->teedev; - - if (shm->flags & TEE_SHM_DMA_BUF) { - mutex_lock(&teedev->mutex); - idr_remove(&teedev->idr, shm->id); - mutex_unlock(&teedev->mutex); - } - if (shm->flags & TEE_SHM_POOL) { struct tee_shm_pool_mgr *poolm; @@ -67,45 +56,6 @@ static void tee_shm_release(struct tee_shm *shm) tee_device_put(teedev); } -static struct sg_table *tee_shm_op_map_dma_buf(struct dma_buf_attachment - *attach, enum dma_data_direction dir) -{ - return NULL; -} - -static void tee_shm_op_unmap_dma_buf(struct dma_buf_attachment *attach, - struct sg_table *table, - enum dma_data_direction dir) -{ -} - -static void tee_shm_op_release(struct dma_buf *dmabuf) -{ - struct tee_shm *shm = dmabuf->priv; - - tee_shm_release(shm); -} - -static int tee_shm_op_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma) -{ - struct tee_shm *shm = dmabuf->priv; - size_t size = vma->vm_end - vma->vm_start; - - /* Refuse sharing shared memory provided by application */ - if (shm->flags & TEE_SHM_USER_MAPPED) - return -EINVAL; - - return remap_pfn_range(vma, vma->vm_start, shm->paddr >> PAGE_SHIFT, - size, vma->vm_page_prot); -} - -static const struct dma_buf_ops tee_shm_dma_buf_ops = { - .map_dma_buf = tee_shm_op_map_dma_buf, - .unmap_dma_buf = tee_shm_op_unmap_dma_buf, - .release = tee_shm_op_release, - .mmap = tee_shm_op_mmap, -}; - struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags) { struct tee_device *teedev = ctx->teedev; @@ -140,6 +90,7 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags) goto err_dev_put; } + refcount_set(&shm->refcount, 1); shm->flags = flags | TEE_SHM_POOL; shm->ctx = ctx; if (flags & TEE_SHM_DMA_BUF) @@ -153,10 +104,7 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags) goto err_kfree; } - if (flags & TEE_SHM_DMA_BUF) { - DEFINE_DMA_BUF_EXPORT_INFO(exp_info); - mutex_lock(&teedev->mutex); shm->id = idr_alloc(&teedev->idr, shm, 1, 0, GFP_KERNEL); mutex_unlock(&teedev->mutex); @@ -164,28 +112,11 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags) ret = ERR_PTR(shm->id); goto err_pool_free; } - - exp_info.ops = &tee_shm_dma_buf_ops; - exp_info.size = shm->size; - exp_info.flags = O_RDWR; - exp_info.priv = shm; - - shm->dmabuf = dma_buf_export(&exp_info); - if (IS_ERR(shm->dmabuf)) { - ret = ERR_CAST(shm->dmabuf); - goto err_rem; - } } teedev_ctx_get(ctx); return shm; -err_rem: - if (flags & TEE_SHM_DMA_BUF) { - mutex_lock(&teedev->mutex); - idr_remove(&teedev->idr, shm->id); - mutex_unlock(&teedev->mutex); - } err_pool_free: poolm->ops->free(poolm, shm); err_kfree: @@ -246,6 +177,7 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr, goto err; } + refcount_set(&shm->refcount, 1); shm->flags = flags | TEE_SHM_REGISTER; shm->ctx = ctx; shm->id = -1; @@ -306,22 +238,6 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr, goto err; } - if (flags & TEE_SHM_DMA_BUF) { - DEFINE_DMA_BUF_EXPORT_INFO(exp_info); - - exp_info.ops = &tee_shm_dma_buf_ops; - exp_info.size = shm->size; - exp_info.flags = O_RDWR; - exp_info.priv = shm; - - shm->dmabuf = dma_buf_export(&exp_info); - if (IS_ERR(shm->dmabuf)) { - ret = ERR_CAST(shm->dmabuf); - teedev->desc->ops->shm_unregister(ctx, shm); - goto err; - } - } - return shm; err: if (shm) { @@ -339,6 +255,35 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr, } EXPORT_SYMBOL_GPL(tee_shm_register); +static int tee_shm_fop_release(struct inode *inode, struct file *filp) +{ + tee_shm_put(filp->private_data); + return 0; +} + +static int tee_shm_fop_mmap(struct file *filp, struct vm_area_struct *vma) +{ + struct tee_shm *shm = filp->private_data; + size_t size = vma->vm_end - vma->vm_start; + + /* Refuse sharing shared memory provided by application */ + if (shm->flags & TEE_SHM_USER_MAPPED) + return -EINVAL; + + /* check for overflowing the buffer's size */ + if (vma->vm_pgoff + vma_pages(vma) > shm->size >> PAGE_SHIFT) + return -EINVAL; + + return remap_pfn_range(vma, vma->vm_start, shm->paddr >> PAGE_SHIFT, + size, vma->vm_page_prot); +} + +static const struct file_operations tee_shm_fops = { + .owner = THIS_MODULE, + .release = tee_shm_fop_release, + .mmap = tee_shm_fop_mmap, +}; + /** * tee_shm_get_fd() - Increase reference count and return file descriptor * @shm: Shared memory handle @@ -351,10 +296,11 @@ int tee_shm_get_fd(struct tee_shm *shm) if (!(shm->flags & TEE_SHM_DMA_BUF)) return -EINVAL; - get_dma_buf(shm->dmabuf); - fd = dma_buf_fd(shm->dmabuf, O_CLOEXEC); + /* matched by tee_shm_put() in tee_shm_op_release() */ + refcount_inc(&shm->refcount); + fd = anon_inode_getfd("tee_shm", &tee_shm_fops, shm, O_RDWR); if (fd < 0) - dma_buf_put(shm->dmabuf); + tee_shm_put(shm); return fd; } @@ -364,17 +310,7 @@ int tee_shm_get_fd(struct tee_shm *shm) */ void tee_shm_free(struct tee_shm *shm) { - /* - * dma_buf_put() decreases the dmabuf reference counter and will - * call tee_shm_release() when the last reference is gone. - * - * In the case of driver private memory we call tee_shm_release - * directly instead as it doesn't have a reference counter. - */ - if (shm->flags & TEE_SHM_DMA_BUF) - dma_buf_put(shm->dmabuf); - else - tee_shm_release(shm); + tee_shm_put(shm); } EXPORT_SYMBOL_GPL(tee_shm_free); @@ -481,10 +417,15 @@ struct tee_shm *tee_shm_get_from_id(struct tee_context *ctx, int id) teedev = ctx->teedev; mutex_lock(&teedev->mutex); shm = idr_find(&teedev->idr, id); + /* + * If the tee_shm was found in the IDR it must have a refcount + * larger than 0 due to the guarantee in tee_shm_put() below. So + * it's safe to use refcount_inc(). + */ if (!shm || shm->ctx != ctx) shm = ERR_PTR(-EINVAL); - else if (shm->flags & TEE_SHM_DMA_BUF) - get_dma_buf(shm->dmabuf); + else + refcount_inc(&shm->refcount); mutex_unlock(&teedev->mutex); return shm; } @@ -496,7 +437,24 @@ EXPORT_SYMBOL_GPL(tee_shm_get_from_id); */ void tee_shm_put(struct tee_shm *shm) { - if (shm->flags & TEE_SHM_DMA_BUF) - dma_buf_put(shm->dmabuf); + struct tee_device *teedev = shm->ctx->teedev; + bool do_release = false; + + mutex_lock(&teedev->mutex); + if (refcount_dec_and_test(&shm->refcount)) { + /* + * refcount has reached 0, we must now remove it from the + * IDR before releasing the mutex. This will guarantee that + * the refcount_inc() in tee_shm_get_from_id() never starts + * from 0. + */ + if (shm->flags & TEE_SHM_DMA_BUF) + idr_remove(&teedev->idr, shm->id); + do_release = true; + } + mutex_unlock(&teedev->mutex); + + if (do_release) + tee_shm_release(teedev, shm); } EXPORT_SYMBOL_GPL(tee_shm_put); diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h index a1f03461369b..e59130eb78a1 100644 --- a/include/linux/tee_drv.h +++ b/include/linux/tee_drv.h @@ -214,7 +214,7 @@ struct tee_shm { unsigned int offset; struct page **pages; size_t num_pages; - struct dma_buf *dmabuf; + refcount_t refcount; u32 flags; int id; u64 sec_world_id; -- 2.31.1

2 years, 11 months

[GIT PULL] OP-TEE add async notifications for v5.17

by Jens Wiklander

Hello arm-soc maintainers, Please pull these patches which adds support for asynchronous notifications from OP-TEE in secure world to the OP-TEE driver. An edge-triggered interrupt is used to notify the the driver. These patches has been in linux-next for a few weeks already. Thanks, Jens The following changes since commit fa55b7dcdc43c1aa1ba12bca9d2dd4318c2a0dbf: Linux 5.16-rc1 (2021-11-14 13:56:52 -0800) are available in the Git repository at: https://git.linaro.org/people/jens.wiklander/linux-tee.git tags/optee-async-notif-for-v5.17 for you to fetch changes up to b98aee466d194788bd651cb375b0e0f7e0e69865: optee: Fix NULL but dereferenced coccicheck error (2021-11-29 22:02:25 +0100) ---------------------------------------------------------------- OP-TEE Asynchronous notifications from secure world Adds support in the SMC based OP-TEE driver to receive asynchronous notifications from secure world using an edge-triggered interrupt as delivery mechanism. ---------------------------------------------------------------- Jens Wiklander (6): docs: staging/tee.rst: add a section on OP-TEE notifications dt-bindings: arm: optee: add interrupt property tee: fix put order in teedev_close_context() tee: export teedev_open() and teedev_close_context() optee: separate notification functions optee: add asynchronous notifications Yang Li (1): optee: Fix NULL but dereferenced coccicheck error .../bindings/arm/firmware/linaro,optee-tz.yaml | 8 + Documentation/staging/tee.rst | 30 +++ drivers/tee/optee/Makefile | 1 + drivers/tee/optee/core.c | 2 +- drivers/tee/optee/ffa_abi.c | 6 +- drivers/tee/optee/notif.c | 125 +++++++++++ drivers/tee/optee/optee_msg.h | 9 + drivers/tee/optee/optee_private.h | 28 ++- drivers/tee/optee/optee_rpc_cmd.h | 31 +-- drivers/tee/optee/optee_smc.h | 75 ++++++- drivers/tee/optee/rpc.c | 71 +----- drivers/tee/optee/smc_abi.c | 237 ++++++++++++++++++--- drivers/tee/tee_core.c | 10 +- include/linux/tee_drv.h | 14 ++ 14 files changed, 523 insertions(+), 124 deletions(-) create mode 100644 drivers/tee/optee/notif.c

2 years, 11 months

Re: [PATCH] optee: Suppress false positive kmemleak report in optee_handle_rpc()

by Wang, Xiaolei

-----Original Message----- From: Sumit Garg<sumit.garg(a)linaro.org> Sent: Thursday, December 9, 2021 7:41 PM To: Wang, Xiaolei<Xiaolei.Wang(a)windriver.com> Cc:jens.wiklander@linaro.org;op-tee@lists.trustedfirmware.org;linux-kernel@vger.kernel.org Subject: Re: [PATCH] optee: Suppress false positive kmemleak report in optee_handle_rpc() [Please note: This e-mail is from an EXTERNAL e-mail address] On Mon, 6 Dec 2021 at 17:35, Xiaolei Wang<xiaolei.wang(a)windriver.com> wrote: > We observed the following kmemleak report: > unreferenced object 0xffff000007904500 (size 128): > comm "swapper/0", pid 1, jiffies 4294892671 (age 44.036s) > hex dump (first 32 bytes): > 00 47 90 07 00 00 ff ff 60 00 c0 ff 00 00 00 00 .G......`....... > 60 00 80 13 00 80 ff ff a0 00 00 00 00 00 00 00 `............... > backtrace: > [<000000004c12b1c7>] kmem_cache_alloc+0x1ac/0x2f4 > [<000000005d23eb4f>] tee_shm_alloc+0x78/0x230 > [<00000000794dd22c>] optee_handle_rpc+0x60/0x6f0 > [<00000000d9f7c52d>] optee_do_call_with_arg+0x17c/0x1dc > [<00000000c35884da>] optee_open_session+0x128/0x1ec > [<000000001748f2ff>] tee_client_open_session+0x28/0x40 > [<00000000aecb5389>] optee_enumerate_devices+0x84/0x2a0 > [<000000003df18bf1>] optee_probe+0x674/0x6cc > [<000000003a4a534a>] platform_drv_probe+0x54/0xb0 > [<000000000c51ce7d>] really_probe+0xe4/0x4d0 > [<000000002f04c865>] driver_probe_device+0x58/0xc0 > [<00000000b485397d>] device_driver_attach+0xc0/0xd0 > [<00000000c835f0df>] __driver_attach+0x84/0x124 > [<000000008e5a429c>] bus_for_each_dev+0x70/0xc0 > [<000000001735e8a8>] driver_attach+0x24/0x30 > [<000000006d94b04f>] bus_add_driver+0x104/0x1ec > > This is not a memory leak because we pass the share memory pointer to > secure world and would get it from secure world before releasing it. > > IMO, we need to cross-check optee-os if it's responsible for leaking kernel memory. Hi Sumit You mean we need to check whether there is a real memory leak, if being secure world just allocates kernel memory via OPTEE_SMC_RPC_FUNC_ALLOC and until the end, there is no free it via OPTEE_SMC_RPC_FUNC_FREE, then we should judge it as a memory leak. thanks xiaolei > > > -Sumit > >> Signed-off-by: Xiaolei Wang<xiaolei.wang(a)windriver.com> >> --- >> drivers/tee/optee/smc_abi.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c >> index 6196d7c3888f..cf2e3293567d 100644 >> --- a/drivers/tee/optee/smc_abi.c >> +++ b/drivers/tee/optee/smc_abi.c >> @@ -23,6 +23,7 @@ >> #include "optee_private.h" >> #include "optee_smc.h" >> #include "optee_rpc_cmd.h" >> +#include <linux/kmemleak.h> >> #define CREATE_TRACE_POINTS >> #include "optee_trace.h" >> >> @@ -783,6 +784,7 @@ static void optee_handle_rpc(struct tee_context *ctx, >> param->a4 = 0; >> param->a5 = 0; >> } >> + kmemleak_not_leak(shm); >> break; >> case OPTEE_SMC_RPC_FUNC_FREE: >> shm = reg_pair_to_ptr(param->a1, param->a2); >> -- >> 2.25.1 >>

2 years, 11 months

Hardening OP-TEE kernel and trustlets with sanitizers

by Igor Zhbanov

Hello! Is it possible to use any compiler-based sanitizers to harden OP-TEE kernel and/or trustlets? I know, there is ASAN support in the OP-TEE kernel. But can it be used with TAs? Or some other sanitizers like UBSan? Thank you.

2 years, 11 months

2024

2023

2022

2021

2020

OP-TEE December 2021