patch #1 ~ #2 prepare for backporting fix patch patch #3 fix CVE-2021-44733
Jens Wiklander (3): tee: remove linked list of struct tee_shm tee: don't assign shm id for private shms tee: handle lookup of shm with reference count 0
drivers/tee/tee_core.c | 1 - drivers/tee/tee_private.h | 3 +- drivers/tee/tee_shm.c | 196 ++++++++++++++------------------------ include/linux/tee_drv.h | 7 +- 4 files changed, 77 insertions(+), 130 deletions(-)
From: Jens Wiklander jens.wiklander@linaro.org
mainline inclusion from mainline-v5.7-rc1 commit 59a135f6fb669f4f79f43160c7b8c8d6bfb37f75 category: cleanup bugzilla: NA CVE: NA
Prepare for fixing CVE-2021-44733. --------------------------------
Removes list_shm from struct tee_context since the linked list isn't used any longer.
Signed-off-by: Jens Wiklander jens.wiklander@linaro.org Conflicts: drivers/tee/tee_core.c [yyl: adjust context] Signed-off-by: Yang Yingliang yangyingliang@huawei.com --- drivers/tee/tee_core.c | 1 - drivers/tee/tee_shm.c | 12 +----------- include/linux/tee_drv.h | 3 --- 3 files changed, 1 insertion(+), 15 deletions(-)
diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c index dd46b758852aa..0d252f151584b 100644 --- a/drivers/tee/tee_core.c +++ b/drivers/tee/tee_core.c @@ -56,7 +56,6 @@ static int tee_open(struct inode *inode, struct file *filp)
kref_init(&ctx->refcount); ctx->teedev = teedev; - INIT_LIST_HEAD(&ctx->list_shm); filp->private_data = ctx; rc = teedev->desc->ops->open(ctx); if (rc) diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index 0b9ab1d0dd45d..8984158483463 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -26,8 +26,6 @@ static void tee_shm_release(struct tee_shm *shm)
mutex_lock(&teedev->mutex); idr_remove(&teedev->idr, shm->id); - if (shm->ctx) - list_del(&shm->link); mutex_unlock(&teedev->mutex);
if (shm->flags & TEE_SHM_POOL) { @@ -183,12 +181,8 @@ static struct tee_shm *__tee_shm_alloc(struct tee_context *ctx, } }
- if (ctx) { + if (ctx) teedev_ctx_get(ctx); - mutex_lock(&teedev->mutex); - list_add_tail(&shm->link, &ctx->list_shm); - mutex_unlock(&teedev->mutex); - }
return shm; err_rem: @@ -315,10 +309,6 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr, } }
- mutex_lock(&teedev->mutex); - list_add_tail(&shm->link, &ctx->list_shm); - mutex_unlock(&teedev->mutex); - return shm; err: if (shm) { diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h index a2b3dfcee0b55..f3b5782b026e0 100644 --- a/include/linux/tee_drv.h +++ b/include/linux/tee_drv.h @@ -50,7 +50,6 @@ struct tee_shm_pool; */ struct tee_context { struct tee_device *teedev; - struct list_head list_shm; void *data; struct kref refcount; bool releasing; @@ -170,7 +169,6 @@ void tee_device_unregister(struct tee_device *teedev); * struct tee_shm - shared memory object * @teedev: device used to allocate the object * @ctx: context using the object, if NULL the context is gone - * @link link element * @paddr: physical address of the shared memory * @kaddr: virtual address of the shared memory * @size: size of shared memory @@ -187,7 +185,6 @@ void tee_device_unregister(struct tee_device *teedev); struct tee_shm { struct tee_device *teedev; struct tee_context *ctx; - struct list_head link; phys_addr_t paddr; void *kaddr; size_t size;
From: Jens Wiklander jens.wiklander@linaro.org
mainline inclusion from mainline-v5.7-rc1 commit f1bbacedb0af640a93e47799203e556be2825da3 category: cleanup bugzilla: NA CVE: NA
Prepare for fixing CVE-2021-44733. --------------------------------
Private shared memory object must not be referenced from user space. To guarantee that, don't assign an id to shared memory objects which are driver private.
Signed-off-by: Jens Wiklander jens.wiklander@linaro.org Signed-off-by: Yang Yingliang yangyingliang@huawei.com --- drivers/tee/tee_private.h | 3 ++- drivers/tee/tee_shm.c | 31 ++++++++++++++++++------------- 2 files changed, 20 insertions(+), 14 deletions(-)
diff --git a/drivers/tee/tee_private.h b/drivers/tee/tee_private.h index 85d99d621603d..e3d62a21cd53b 100644 --- a/drivers/tee/tee_private.h +++ b/drivers/tee/tee_private.h @@ -46,7 +46,8 @@ struct tee_shm_pool { * @num_users: number of active users of this device * @c_no_user: completion used when unregistering the device * @mutex: mutex protecting @num_users and @idr - * @idr: register of shared memory object allocated on this device + * @idr: register of user space shared memory objects allocated or + * registered on this device * @pool: shared memory pool */ struct tee_device { diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index 8984158483463..4010817765ef1 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -24,9 +24,11 @@ static void tee_shm_release(struct tee_shm *shm) { struct tee_device *teedev = shm->teedev;
- mutex_lock(&teedev->mutex); - idr_remove(&teedev->idr, shm->id); - mutex_unlock(&teedev->mutex); + 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; @@ -158,17 +160,18 @@ static struct tee_shm *__tee_shm_alloc(struct tee_context *ctx, goto err_kfree; }
- mutex_lock(&teedev->mutex); - shm->id = idr_alloc(&teedev->idr, shm, 1, 0, GFP_KERNEL); - mutex_unlock(&teedev->mutex); - if (shm->id < 0) { - ret = ERR_PTR(shm->id); - goto err_pool_free; - }
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); + if (shm->id < 0) { + 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; @@ -186,9 +189,11 @@ static struct tee_shm *__tee_shm_alloc(struct tee_context *ctx,
return shm; err_rem: - mutex_lock(&teedev->mutex); - idr_remove(&teedev->idr, shm->id); - mutex_unlock(&teedev->mutex); + 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:
From: Jens Wiklander jens.wiklander@linaro.org
mainline inclusion from mainline-v5.17 commit dfd0743f1d9ea76931510ed150334d571fbab49d category: bugfix bugzilla: NA CVE: CVE-2021-44733
--------------------------------
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@vger.kernel.org Reviewed-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Reviewed-by: Lars Persson larper@axis.com Reviewed-by: Sumit Garg sumit.garg@linaro.org Reported-by: Patrik Lantz patrik.lantz@axis.com Signed-off-by: Jens Wiklander jens.wiklander@linaro.org Conflicts: drivers/tee/tee_shm.c include/linux/tee_drv.h [yyl: adjust context] Signed-off-by: Yang Yingliang yangyingliang@huawei.com --- drivers/tee/tee_shm.c | 177 +++++++++++++++------------------------- include/linux/tee_drv.h | 4 +- 2 files changed, 68 insertions(+), 113 deletions(-)
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index 4010817765ef1..c591e7ba4435c 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015-2016, Linaro Limited + * Copyright (c) 2015-2017, 2019-2021 Linaro Limited * * This software is licensed under the terms of the GNU General Public * License version 2, as published by the Free Software Foundation, and @@ -11,25 +11,17 @@ * GNU General Public License for more details. * */ +#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 "tee_private.h"
-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->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;
@@ -61,51 +53,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 void *tee_shm_op_map(struct dma_buf *dmabuf, unsigned long pgnum) -{ - return NULL; -} - -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_REGISTER) - 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, - .map = tee_shm_op_map, - .mmap = tee_shm_op_mmap, -}; - static struct tee_shm *__tee_shm_alloc(struct tee_context *ctx, struct tee_device *teedev, size_t size, u32 flags) @@ -146,6 +93,7 @@ static struct tee_shm *__tee_shm_alloc(struct tee_context *ctx, goto err_dev_put; }
+ refcount_set(&shm->refcount, 1); shm->flags = flags | TEE_SHM_POOL; shm->teedev = teedev; shm->ctx = ctx; @@ -160,10 +108,7 @@ static struct tee_shm *__tee_shm_alloc(struct tee_context *ctx, 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); @@ -171,29 +116,12 @@ static struct tee_shm *__tee_shm_alloc(struct tee_context *ctx, 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; - } }
if (ctx) 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: @@ -258,6 +186,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->teedev = teedev; shm->ctx = ctx; @@ -298,22 +227,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) { @@ -337,6 +250,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 @@ -349,10 +291,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; }
@@ -362,17 +305,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);
@@ -479,10 +412,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; } @@ -494,7 +432,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 f3b5782b026e0..a9ba89533284f 100644 --- a/include/linux/tee_drv.h +++ b/include/linux/tee_drv.h @@ -175,7 +175,7 @@ void tee_device_unregister(struct tee_device *teedev); * @offset: offset of buffer in user space * @pages: locked pages from userspace * @num_pages: number of locked pages - * @dmabuf: dmabuf used to for exporting to user space + * @refcount: reference counter * @flags: defined by TEE_SHM_* in tee_drv.h * @id: unique id of a shared memory object on this device * @@ -191,7 +191,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; };
-
Yang Yingliang