OP-TEE December 2022

op-tee@lists.trustedfirmware.org

5 participants
5 discussions

[PATCH v2 0/1] tee: Add tee_shm_register_fd

by Olivier Masse

Add a new ioctl called TEE_IOC_SHM_REGISTER_FD to register a shared memory from a dmabuf file descriptor. This new ioctl will allow the Linux Kernel to register a buffer to be used by the Secure Data Path OPTEE OS feature. Please find more information here: https://static.linaro.org/connect/san19/presentations/san19-107.pdf Patch tested on Hikey 6220. Etienne Carriere (1): tee: new ioctl to a register tee_shm from a dmabuf file descriptor drivers/tee/tee_core.c | 38 +++++++++++++++ drivers/tee/tee_shm.c | 99 +++++++++++++++++++++++++++++++++++++++- include/linux/tee_drv.h | 11 +++++ include/uapi/linux/tee.h | 29 ++++++++++++ 4 files changed, 175 insertions(+), 2 deletions(-) -- 2.25.0

1 year, 9 months

[PATCH] tee: optee: ffa_abi: Fix use-after-free in optee_ffa_open

by Yoochan Lee

A race condition may occur if the user physically removes the ffa_abi device while calling open(). This is a race condition between optee_open() function and the optee_ffa_remove() function, which may lead to Use-After-Free. Therefore, add a refcount check to optee_ffa_remove() function to free the "optee" structure after the device is close()d. ---------------CPU 0--------------------CPU 1----------------- optee_open() | optee_ffa_remove() -------------------------------------------------------------- struct optee *optee = tee_get_| drvdata(teedev); — (1) | | struct optee *optee = ffa_dev_ | get_drvdata(ffa_dev); | ... | kfree(optee); — (2) if (teedev == optee->supp_ | teedev) { — (3) | Signed-off-by: Yoochan Lee <yoochan1026(a)gmail.com> --- drivers/tee/optee/ffa_abi.c | 49 +++++++++++++++++++++++++------ drivers/tee/optee/optee_private.h | 1 + 2 files changed, 41 insertions(+), 9 deletions(-) diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c index 0828240f27e6..ea76d7532419 100644 --- a/drivers/tee/optee/ffa_abi.c +++ b/drivers/tee/optee/ffa_abi.c @@ -726,15 +726,52 @@ static void optee_ffa_get_version(struct tee_device *teedev, *vers = v; } +static void optee_ffa_delete(struct kref *kref) +{ + struct optee *optee = container_of(kref, struct optee, refcnt); + + optee_remove_common(optee); + + mutex_destroy(&optee->ffa.mutex); + rhashtable_free_and_destroy(&optee->ffa.global_ids, rh_free_fn, NULL); + + kfree(optee); + +} + +static void optee_ffa_release(struct tee_context *ctx) +{ + struct optee *optee = tee_get_drvdata(teedev); + + optee_release_helper(ctx, optee_close_session_helper); + kref_put(&optee->refcnt, optee_ffa_delete); +} + +void optee_ffa_release_supp(struct tee_context *ctx) +{ + struct optee *optee = tee_get_drvdata(ctx->teedev); + + optee_release_helper(ctx, optee_close_session_helper); + if (optee->scan_bus_wq) { + destroy_workqueue(optee->scan_bus_wq); + optee->scan_bus_wq = NULL; + } + optee_supp_release(&optee->supp); + kref_put(&optee->refcnt, optee_ffa_delete); +} + static int optee_ffa_open(struct tee_context *ctx) { + struct optee *optee = tee_get_drvdata(teedev); + kref_get(&optee->refcnt); + return optee_open(ctx, true); } static const struct tee_driver_ops optee_ffa_clnt_ops = { .get_version = optee_ffa_get_version, .open = optee_ffa_open, - .release = optee_release, + .release = optee_ffa_release, .open_session = optee_open_session, .close_session = optee_close_session, .invoke_func = optee_invoke_func, @@ -752,7 +789,7 @@ static const struct tee_desc optee_ffa_clnt_desc = { static const struct tee_driver_ops optee_ffa_supp_ops = { .get_version = optee_ffa_get_version, .open = optee_ffa_open, - .release = optee_release_supp, + .release = optee_ffa_release_supp, .supp_recv = optee_supp_recv, .supp_send = optee_supp_send, .shm_register = optee_ffa_shm_register, /* same as for clnt ops */ @@ -775,13 +812,7 @@ static const struct optee_ops optee_ffa_ops = { static void optee_ffa_remove(struct ffa_device *ffa_dev) { struct optee *optee = ffa_dev_get_drvdata(ffa_dev); - - optee_remove_common(optee); - - mutex_destroy(&optee->ffa.mutex); - rhashtable_free_and_destroy(&optee->ffa.global_ids, rh_free_fn, NULL); - - kfree(optee); + kref_put(&optee->refcnt, optee_ffa_delete); } static int optee_ffa_probe(struct ffa_device *ffa_dev) diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h index 04ae58892608..f52b1cf20eab 100644 --- a/drivers/tee/optee/optee_private.h +++ b/drivers/tee/optee/optee_private.h @@ -175,6 +175,7 @@ struct optee { bool scan_bus_done; struct workqueue_struct *scan_bus_wq; struct work_struct scan_bus_work; + struct kref refcnt; }; struct optee_session { -- 2.39.0

1 year, 10 months

[PATCH] tee: optee: smc_abi: Fix use-after-free in optee_smc_open

by Yoochan Lee

A race condition may occur if the user physically removes the smc_abi device while calling open(). This is a race condition between optee_smc_open() function and the optee_smc_remove() function, which may lead to Use-After-Free. Therefore, add a refcount check to optee_smc_remove() function to free the "optee" structure after the device is close()d. ---------------CPU 0--------------------CPU 1----------------- optee_smc_open() | optee_smc_remove() -------------------------------------------------------------- struct optee *optee = tee_get_| drvdata(ctx->teedev); — (1) | | struct optee *optee = platform_ | get_drvdata(pdev); | ... | kfree(optee); — (2) u32 sec_caps = optee->smc.sec_| caps; — (3) Signed-off-by: Yoochan Lee <yoochan1026(a)gmail.com> --- drivers/tee/optee/optee_private.h | 1 + drivers/tee/optee/smc_abi.c | 66 ++++++++++++++++++++++--------- 2 files changed, 48 insertions(+), 19 deletions(-) diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h index 04ae58892608..f52b1cf20eab 100644 --- a/drivers/tee/optee/optee_private.h +++ b/drivers/tee/optee/optee_private.h @@ -175,6 +175,7 @@ struct optee { bool scan_bus_done; struct workqueue_struct *scan_bus_wq; struct work_struct scan_bus_work; + struct kref refcnt; }; struct optee_session { diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c index a1c1fa1a9c28..4fbec2acc255 100644 --- a/drivers/tee/optee/smc_abi.c +++ b/drivers/tee/optee/smc_abi.c @@ -1077,18 +1077,61 @@ static void optee_get_version(struct tee_device *teedev, *vers = v; } +static void optee_smc_delete(struct kref *kref) +{ + struct optee *optee = container_of(kref, struct optee, refcnt); + + /* + * Ask OP-TEE to free all cached shared memory objects to decrease + * reference counters and also avoid wild pointers in secure world + * into the old shared memory range. + */ + if (!optee->rpc_param_count) + optee_disable_shm_cache(optee); + + optee_smc_notif_uninit_irq(optee); + + optee_remove_common(optee); + + if (optee->smc.memremaped_shm) + memunmap(optee->smc.memremaped_shm); + + kfree(optee); +} + +static void optee_smc_release_supp(struct tee_context *ctx) +{ + struct optee *optee = tee_get_drvdata(ctx->teedev); + + optee_release_helper(ctx, optee_close_session_helper); + if (optee->scan_bus_wq) { + destroy_workqueue(optee->scan_bus_wq); + optee->scan_bus_wq = NULL; + } + optee_supp_release(&optee->supp); + kref_put(&optee->refcnt, optee_smc_delete); +} + +static void optee_smc_release(struct tee_context *ctx) +{ + struct optee *optee = tee_get_drvdata(ctx->teedev); + + optee_release_helper(ctx, optee_close_session_helper); + kref_put(&optee->refcnt, optee_smc_delete); +} + static int optee_smc_open(struct tee_context *ctx) { struct optee *optee = tee_get_drvdata(ctx->teedev); u32 sec_caps = optee->smc.sec_caps; - + kref_get(&optee->refcnt); return optee_open(ctx, sec_caps & OPTEE_SMC_SEC_CAP_MEMREF_NULL); } static const struct tee_driver_ops optee_clnt_ops = { .get_version = optee_get_version, .open = optee_smc_open, - .release = optee_release, + .release = optee_smc_release, .open_session = optee_open_session, .close_session = optee_close_session, .invoke_func = optee_invoke_func, @@ -1106,7 +1149,7 @@ static const struct tee_desc optee_clnt_desc = { static const struct tee_driver_ops optee_supp_ops = { .get_version = optee_get_version, .open = optee_smc_open, - .release = optee_release_supp, + .release = optee_smc_release_supp, .supp_recv = optee_supp_recv, .supp_send = optee_supp_send, .shm_register = optee_shm_register_supp, @@ -1319,22 +1362,7 @@ static int optee_smc_remove(struct platform_device *pdev) { struct optee *optee = platform_get_drvdata(pdev); - /* - * Ask OP-TEE to free all cached shared memory objects to decrease - * reference counters and also avoid wild pointers in secure world - * into the old shared memory range. - */ - if (!optee->rpc_param_count) - optee_disable_shm_cache(optee); - - optee_smc_notif_uninit_irq(optee); - - optee_remove_common(optee); - - if (optee->smc.memremaped_shm) - memunmap(optee->smc.memremaped_shm); - - kfree(optee); + kref_put(&optee->refcnt, optee_smc_delete); return 0; } -- 2.39.0

1 year, 10 months

Linaro OP-TEE Contribution (LOC) monthly meeting December 27

by Jens Wiklander

Hi, The LOC meeting this month is in the middle of the holidays. Let's cancel it since we don't have any urgent topics. Regards, Jens on behalf of the OP-TEE team.

1 year, 11 months

Re: [PATCH 2/4] tee: Remove vmalloc page support

by Sumit Garg

Hi Phil, Please don't top-post in the OSS mailing list. On Wed, 5 Oct 2022 at 08:59, Phil Chang (張世勳) <Phil.Chang(a)mediatek.com> wrote: > > Hi Sumit > > Thanks for mentioning that, in fact, our product is low memory devices, and continuous pages are extremely valuable. > Although our driver is not upstream yet but highly dependent on tee shm vmalloc support, Sorry but you need to get your driver mainline in order to support vmalloc interface. As otherwise it's a maintenance nightmare to support interfaces in the mainline for out-of-tree drivers. -Sumit > some scenarios are driver alloc high order pages but system memory is fragmentation so that alloc failed. > In this situation, vmalloc support is important and gives flexible usage to user. > > > -----Original Message----- > From: Sumit Garg <sumit.garg(a)linaro.org> > Sent: Monday, October 3, 2022 2:57 PM > To: ira.weiny(a)intel.com > Cc: Jens Wiklander <jens.wiklander(a)linaro.org>; Andrew Morton <akpm(a)linux-foundation.org>; Al Viro <viro(a)zeniv.linux.org.uk>; Fabio M. De Francesco <fmdefrancesco(a)gmail.com>; Christoph Hellwig <hch(a)lst.de>; Linus Torvalds <torvalds(a)linux-foundation.org>; op-tee(a)lists.trustedfirmware.org; linux-kernel(a)vger.kernel.org; linux-mm(a)kvack.org; Phil Chang (張世勳) <Phil.Chang(a)mediatek.com> > Subject: Re: [PATCH 2/4] tee: Remove vmalloc page support > > + Phil > > Hi Ira, > > On Sun, 2 Oct 2022 at 05:53, <ira.weiny(a)intel.com> wrote: > > > > From: Ira Weiny <ira.weiny(a)intel.com> > > > > The kernel pages used by shm_get_kernel_pages() are allocated using > > GFP_KERNEL through the following call stack: > > > > trusted_instantiate() > > trusted_payload_alloc() -> GFP_KERNEL > > <trusted key op> > > tee_shm_register_kernel_buf() > > register_shm_helper() > > shm_get_kernel_pages() > > > > Where <trusted key op> is one of: > > > > trusted_key_unseal() > > trusted_key_get_random() > > trusted_key_seal() > > > > Remove the vmalloc page support from shm_get_kernel_pages(). Replace > > with a warn on once. > > > > Cc: Jens Wiklander <jens.wiklander(a)linaro.org> > > Cc: Al Viro <viro(a)zeniv.linux.org.uk> > > Cc: "Fabio M. De Francesco" <fmdefrancesco(a)gmail.com> > > Cc: Christoph Hellwig <hch(a)lst.de> > > Cc: Linus Torvalds <torvalds(a)linux-foundation.org> > > Signed-off-by: Ira Weiny <ira.weiny(a)intel.com> > > > > --- > > Jens I went with the suggestion from Linus and Christoph and rejected > > vmalloc addresses. I did not hear back from you regarding Linus' > > question if the vmalloc page support was required by an up coming > > patch set or not. So I assumed it was something out of tree. > > It looks like I wasn't CC'd to that conversation. IIRC, support for vmalloc addresses was added recently by Phil here [1]. So I would like to give him a chance if he is planning to post a corresponding kernel driver upstream. > > [1] https://urldefense.com/v3/__https://lists.trustedfirmware.org/archives/list… > > -Sumit > > > --- > > drivers/tee/tee_shm.c | 36 ++++++++++++------------------------ > > 1 file changed, 12 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index > > 27295bda3e0b..527a6eabc03e 100644 > > --- a/drivers/tee/tee_shm.c > > +++ b/drivers/tee/tee_shm.c > > @@ -24,37 +24,25 @@ static void shm_put_kernel_pages(struct page > > **pages, size_t page_count) static int shm_get_kernel_pages(unsigned long start, size_t page_count, > > struct page **pages) { > > + struct kvec *kiov; > > size_t n; > > int rc; > > > > - if (is_vmalloc_addr((void *)start)) { > > - struct page *page; > > - > > - for (n = 0; n < page_count; n++) { > > - page = vmalloc_to_page((void *)(start + PAGE_SIZE * n)); > > - if (!page) > > - return -ENOMEM; > > - > > - get_page(page); > > - pages[n] = page; > > - } > > - rc = page_count; > > - } else { > > - struct kvec *kiov; > > - > > - kiov = kcalloc(page_count, sizeof(*kiov), GFP_KERNEL); > > - if (!kiov) > > - return -ENOMEM; > > + if (WARN_ON_ONCE(is_vmalloc_addr((void *)start))) > > + return -EINVAL; > > > > - for (n = 0; n < page_count; n++) { > > - kiov[n].iov_base = (void *)(start + n * PAGE_SIZE); > > - kiov[n].iov_len = PAGE_SIZE; > > - } > > + kiov = kcalloc(page_count, sizeof(*kiov), GFP_KERNEL); > > + if (!kiov) > > + return -ENOMEM; > > > > - rc = get_kernel_pages(kiov, page_count, 0, pages); > > - kfree(kiov); > > + for (n = 0; n < page_count; n++) { > > + kiov[n].iov_base = (void *)(start + n * PAGE_SIZE); > > + kiov[n].iov_len = PAGE_SIZE; > > } > > > > + rc = get_kernel_pages(kiov, page_count, 0, pages); > > + kfree(kiov); > > + > > return rc; > > } > > > > -- > > 2.37.2 > >

1 year, 11 months

2024

2023

2022

2021

2020

OP-TEE December 2022