Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

2 participants
2906 discussions

Re: [PATCH] udmabuf: fix a buf size overflow issue during udmabuf creation

by Christian König

Am 26.03.25 um 03:59 schrieb Kasireddy, Vivek: > Hi Christian, > >> Subject: Re: [PATCH] udmabuf: fix a buf size overflow issue during udmabuf >> creation >> >> Am 25.03.25 um 07:23 schrieb Kasireddy, Vivek: >>> Hi Christian, >>> >>>> Am 21.03.25 um 17:41 schrieb Xiaogang.Chen: >>>>> From: Xiaogang Chen <xiaogang.chen(a)amd.com> >>>>> >>>>> by casting size_limit_mb to u64 when calculate pglimit. >>>>> >>>>> Signed-off-by: Xiaogang Chen<Xiaogang.Chen(a)amd.com> >>>> Reviewed-by: Christian König <christian.koenig(a)amd.com> >>>> >>>> If nobody objects I'm going to push that to drm-misc-fixes. >>> No objection but I wish the author would have added more details in the >> commit >>> message particularly the value they have used to trigger the overflow. I >> guess >>> Xiaogang can still comment here and briefly describe the exact use- >> case/test-case >>> they are running where they encountered this issue. >> Isn't that obvious? At least it was for me. >> >> As soon as you have a value larger than 4095 the 32bit multiplication >> overflows, resulting in incorrectly limiting the buffer size. > Right, that part makes sense. I was mostly curious about why or how they > were using such a large buffer (use-case details). Well I suggested that we use udmabuf to implement shareable dma-bufs which can be allocated from a specific NUMA node and are also accounted in memcg. But to be honest I have absolutely no idea what's the use case for a buffer larger than 4GiB. Regards, Christian. > > > Thanks, > Vivek > >> Regards, >> Christian. >> >>> Thanks, >>> Vivek >>> >>>> Regards, >>>> Christian. >>>> >>>>> --- >>>>> drivers/dma-buf/udmabuf.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c >>>>> index 8ce1f074c2d3..e99e3a65a470 100644 >>>>> --- a/drivers/dma-buf/udmabuf.c >>>>> +++ b/drivers/dma-buf/udmabuf.c >>>>> @@ -398,7 +398,7 @@ static long udmabuf_create(struct miscdevice >>>> *device, >>>>> if (!ubuf) >>>>> return -ENOMEM; >>>>> >>>>> - pglimit = (size_limit_mb * 1024 * 1024) >> PAGE_SHIFT; >>>>> + pglimit = ((u64)size_limit_mb * 1024 * 1024) >> PAGE_SHIFT; >>>>> for (i = 0; i < head->count; i++) { >>>>> pgoff_t subpgcnt; >>>>>

3 months, 1 week

Re: [PATCH v6 08/10] optee: support restricted memory allocation

by Jens Wiklander

On Tue, Mar 25, 2025 at 8:07 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > On Wed, Mar 05, 2025 at 02:04:14PM +0100, Jens Wiklander wrote: > > Add support in the OP-TEE backend driver for restricted memory > > allocation. The support is limited to only the SMC ABI and for secure > > video buffers. > > > > OP-TEE is probed for the range of restricted physical memory and a > > memory pool allocator is initialized if OP-TEE have support for such > > memory. > > > > Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> > > --- > > drivers/tee/optee/core.c | 1 + > > drivers/tee/optee/smc_abi.c | 44 +++++++++++++++++++++++++++++++++++-- > > 2 files changed, 43 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c > > index c75fddc83576..c7fd8040480e 100644 > > --- a/drivers/tee/optee/core.c > > +++ b/drivers/tee/optee/core.c > > @@ -181,6 +181,7 @@ void optee_remove_common(struct optee *optee) > > tee_device_unregister(optee->supp_teedev); > > tee_device_unregister(optee->teedev); > > > > + tee_device_unregister_all_dma_heaps(optee->teedev); > > tee_shm_pool_free(optee->pool); > > optee_supp_uninit(&optee->supp); > > mutex_destroy(&optee->call_queue.mutex); > > diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c > > index cfdae266548b..a14ff0b7d3b3 100644 > > --- a/drivers/tee/optee/smc_abi.c > > +++ b/drivers/tee/optee/smc_abi.c > > @@ -1620,6 +1620,41 @@ static inline int optee_load_fw(struct platform_device *pdev, > > } > > #endif > > > > +static int optee_sdp_pool_init(struct optee *optee) > > +{ > > + enum tee_dma_heap_id heap_id = TEE_DMA_HEAP_SECURE_VIDEO_PLAY; > > + struct tee_rstmem_pool *pool; > > + int rc; > > + > > + if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_SDP) { > > Is this SDP capability an ABI yet since we haven't supported it in > upstream kernel? If no then can we rename it as > OPTEE_SMC_SEC_CAP_RSTMEM? No problem. We can rename it. > > > + union { > > + struct arm_smccc_res smccc; > > + struct optee_smc_get_sdp_config_result result; > > + } res; > > + > > + optee->smc.invoke_fn(OPTEE_SMC_GET_SDP_CONFIG, 0, 0, 0, 0, 0, 0, > > + 0, &res.smccc); > > + if (res.result.status != OPTEE_SMC_RETURN_OK) { > > + pr_err("Secure Data Path service not available\n"); > > + return 0; > > + } > > + > > + pool = tee_rstmem_static_pool_alloc(res.result.start, > > + res.result.size); > > + if (IS_ERR(pool)) > > + return PTR_ERR(pool); > > + > > + rc = tee_device_register_dma_heap(optee->teedev, heap_id, pool); > > + if (rc) > > + goto err; > > + } > > + > > + return 0; > > +err: > > + pool->ops->destroy_pool(pool); > > + return rc; > > +} > > + > > static int optee_probe(struct platform_device *pdev) > > { > > optee_invoke_fn *invoke_fn; > > @@ -1715,7 +1750,7 @@ static int optee_probe(struct platform_device *pdev) > > optee = kzalloc(sizeof(*optee), GFP_KERNEL); > > if (!optee) { > > rc = -ENOMEM; > > - goto err_free_pool; > > + goto err_free_shm_pool; > > } > > > > optee->ops = &optee_ops; > > @@ -1788,6 +1823,10 @@ static int optee_probe(struct platform_device *pdev) > > pr_info("Asynchronous notifications enabled\n"); > > } > > > > + rc = optee_sdp_pool_init(optee); > > s/optee_sdp_pool_init/optee_rstmem_pool_init/ OK Cheers, Jens > > -Sumit > > > + if (rc) > > + goto err_notif_uninit; > > + > > /* > > * Ensure that there are no pre-existing shm objects before enabling > > * the shm cache so that there's no chance of receiving an invalid > > @@ -1823,6 +1862,7 @@ static int optee_probe(struct platform_device *pdev) > > optee_disable_shm_cache(optee); > > optee_smc_notif_uninit_irq(optee); > > optee_unregister_devices(); > > + tee_device_unregister_all_dma_heaps(optee->teedev); > > err_notif_uninit: > > optee_notif_uninit(optee); > > err_close_ctx: > > @@ -1839,7 +1879,7 @@ static int optee_probe(struct platform_device *pdev) > > tee_device_unregister(optee->teedev); > > err_free_optee: > > kfree(optee); > > -err_free_pool: > > +err_free_shm_pool: > > tee_shm_pool_free(pool); > > if (memremaped_shm) > > memunmap(memremaped_shm); > > -- > > 2.43.0 > >

3 months, 1 week

Re: [PATCH v6 06/10] tee: new ioctl to a register tee_shm from a dmabuf file descriptor

by Jens Wiklander

Hi Sumit, On Tue, Mar 25, 2025 at 7:50 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > Hi Jens, > > On Wed, Mar 05, 2025 at 02:04:12PM +0100, Jens Wiklander wrote: > > From: Etienne Carriere <etienne.carriere(a)linaro.org> > > > > Enable userspace to create a tee_shm object that refers to a dmabuf > > reference. > > > > Userspace registers the dmabuf file descriptor as in a tee_shm object. > > The registration is completed with a tee_shm file descriptor returned to > > userspace. > > > > Userspace is free to close the dmabuf file descriptor now since all the > > resources are now held via the tee_shm object. > > > > Closing the tee_shm file descriptor will release all resources used by the > > tee_shm object. > > > > This change only support dmabuf references that relates to physically > > contiguous memory buffers. > > > > New tee_shm flag to identify tee_shm objects built from a registered > > dmabuf, TEE_SHM_DMA_BUF. > > > > Signed-off-by: Etienne Carriere <etienne.carriere(a)linaro.org> > > Signed-off-by: Olivier Masse <olivier.masse(a)nxp.com> > > Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> > > --- > > drivers/tee/tee_core.c | 145 ++++++++++++++++++++++++++----------- > > drivers/tee/tee_private.h | 1 + > > drivers/tee/tee_shm.c | 146 ++++++++++++++++++++++++++++++++++++-- > > include/linux/tee_core.h | 1 + > > include/linux/tee_drv.h | 10 +++ > > include/uapi/linux/tee.h | 29 ++++++++ > > 6 files changed, 288 insertions(+), 44 deletions(-) > > > > I am still trying to find if we really need a separate IOCTL to register > DMA heap with TEE subsystem. Can't we initialize tee_shm as a member of > struct tee_heap_buffer in tee_dma_heap_alloc() where the allocation > happens? No, that's not possible since we don't have a tee_context availble so we can't assign an ID that userspace can use for this shm object. We could add new attribute types TEE_IOCTL_PARAM_ATTR_TYPE_MEMFD_*, but it's a bit more complicated than that since we'd also need to update for the life cycle. > We can always find a reference back to tee_shm object from DMA > buffer. Yes Cheers, Jens > > -Sumit > > > diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c > > index 685afcaa3ea1..3a71643766d5 100644 > > --- a/drivers/tee/tee_core.c > > +++ b/drivers/tee/tee_core.c > > @@ -353,6 +353,103 @@ tee_ioctl_shm_register(struct tee_context *ctx, > > return ret; > > } > > > > +static int > > +tee_ioctl_shm_register_fd(struct tee_context *ctx, > > + struct tee_ioctl_shm_register_fd_data __user *udata) > > +{ > > + struct tee_ioctl_shm_register_fd_data data; > > + struct tee_shm *shm; > > + long ret; > > + > > + if (copy_from_user(&data, udata, sizeof(data))) > > + return -EFAULT; > > + > > + /* Currently no input flags are supported */ > > + if (data.flags) > > + return -EINVAL; > > + > > + shm = tee_shm_register_fd(ctx, data.fd); > > + if (IS_ERR(shm)) > > + return -EINVAL; > > + > > + data.id = shm->id; > > + data.flags = shm->flags; > > + data.size = shm->size; > > + > > + if (copy_to_user(udata, &data, sizeof(data))) > > + ret = -EFAULT; > > + else > > + ret = tee_shm_get_fd(shm); > > + > > + /* > > + * When user space closes the file descriptor the shared memory > > + * should be freed or if tee_shm_get_fd() failed then it will > > + * be freed immediately. > > + */ > > + tee_shm_put(shm); > > + return ret; > > +} > > + > > +static int param_from_user_memref(struct tee_context *ctx, > > + struct tee_param_memref *memref, > > + struct tee_ioctl_param *ip) > > +{ > > + struct tee_shm *shm; > > + size_t offs = 0; > > + > > + /* > > + * If a NULL pointer is passed to a TA in the TEE, > > + * the ip.c IOCTL parameters is set to TEE_MEMREF_NULL > > + * indicating a NULL memory reference. > > + */ > > + if (ip->c != TEE_MEMREF_NULL) { > > + /* > > + * If we fail to get a pointer to a shared > > + * memory object (and increase the ref count) > > + * from an identifier we return an error. All > > + * pointers that has been added in params have > > + * an increased ref count. It's the callers > > + * responibility to do tee_shm_put() on all > > + * resolved pointers. > > + */ > > + shm = tee_shm_get_from_id(ctx, ip->c); > > + if (IS_ERR(shm)) > > + return PTR_ERR(shm); > > + > > + /* > > + * Ensure offset + size does not overflow > > + * offset and does not overflow the size of > > + * the referred shared memory object. > > + */ > > + if ((ip->a + ip->b) < ip->a || > > + (ip->a + ip->b) > shm->size) { > > + tee_shm_put(shm); > > + return -EINVAL; > > + } > > + > > + if (shm->flags & TEE_SHM_DMA_BUF) { > > + struct tee_shm *parent_shm; > > + > > + parent_shm = tee_shm_get_parent_shm(shm, &offs); > > + if (parent_shm) { > > + tee_shm_put(shm); > > + shm = parent_shm; > > + } > > + } > > + } else if (ctx->cap_memref_null) { > > + /* Pass NULL pointer to OP-TEE */ > > + shm = NULL; > > + } else { > > + return -EINVAL; > > + } > > + > > + memref->shm_offs = ip->a + offs; > > + memref->size = ip->b; > > + memref->shm = shm; > > + > > + return 0; > > +} > > + > > static int params_from_user(struct tee_context *ctx, struct tee_param *params, > > size_t num_params, > > struct tee_ioctl_param __user *uparams) > > @@ -360,8 +457,8 @@ static int params_from_user(struct tee_context *ctx, struct tee_param *params, > > size_t n; > > > > for (n = 0; n < num_params; n++) { > > - struct tee_shm *shm; > > struct tee_ioctl_param ip; > > + int rc; > > > > if (copy_from_user(&ip, uparams + n, sizeof(ip))) > > return -EFAULT; > > @@ -384,45 +481,10 @@ static int params_from_user(struct tee_context *ctx, struct tee_param *params, > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT: > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT: > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT: > > - /* > > - * If a NULL pointer is passed to a TA in the TEE, > > - * the ip.c IOCTL parameters is set to TEE_MEMREF_NULL > > - * indicating a NULL memory reference. > > - */ > > - if (ip.c != TEE_MEMREF_NULL) { > > - /* > > - * If we fail to get a pointer to a shared > > - * memory object (and increase the ref count) > > - * from an identifier we return an error. All > > - * pointers that has been added in params have > > - * an increased ref count. It's the callers > > - * responibility to do tee_shm_put() on all > > - * resolved pointers. > > - */ > > - shm = tee_shm_get_from_id(ctx, ip.c); > > - if (IS_ERR(shm)) > > - return PTR_ERR(shm); > > - > > - /* > > - * Ensure offset + size does not overflow > > - * offset and does not overflow the size of > > - * the referred shared memory object. > > - */ > > - if ((ip.a + ip.b) < ip.a || > > - (ip.a + ip.b) > shm->size) { > > - tee_shm_put(shm); > > - return -EINVAL; > > - } > > - } else if (ctx->cap_memref_null) { > > - /* Pass NULL pointer to OP-TEE */ > > - shm = NULL; > > - } else { > > - return -EINVAL; > > - } > > - > > - params[n].u.memref.shm_offs = ip.a; > > - params[n].u.memref.size = ip.b; > > - params[n].u.memref.shm = shm; > > + rc = param_from_user_memref(ctx, &params[n].u.memref, > > + &ip); > > + if (rc) > > + return rc; > > break; > > default: > > /* Unknown attribute */ > > @@ -827,6 +889,8 @@ static long tee_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > > return tee_ioctl_shm_alloc(ctx, uarg); > > case TEE_IOC_SHM_REGISTER: > > return tee_ioctl_shm_register(ctx, uarg); > > + case TEE_IOC_SHM_REGISTER_FD: > > + return tee_ioctl_shm_register_fd(ctx, uarg); > > case TEE_IOC_OPEN_SESSION: > > return tee_ioctl_open_session(ctx, uarg); > > case TEE_IOC_INVOKE: > > @@ -1288,3 +1352,4 @@ MODULE_AUTHOR("Linaro"); > > MODULE_DESCRIPTION("TEE Driver"); > > MODULE_VERSION("1.0"); > > MODULE_LICENSE("GPL v2"); > > +MODULE_IMPORT_NS("DMA_BUF"); > > diff --git a/drivers/tee/tee_private.h b/drivers/tee/tee_private.h > > index 6c6ff5d5eed2..aad7f6c7e0f0 100644 > > --- a/drivers/tee/tee_private.h > > +++ b/drivers/tee/tee_private.h > > @@ -24,6 +24,7 @@ void teedev_ctx_put(struct tee_context *ctx); > > struct tee_shm *tee_shm_alloc_user_buf(struct tee_context *ctx, size_t size); > > struct tee_shm *tee_shm_register_user_buf(struct tee_context *ctx, > > unsigned long addr, size_t length); > > +struct tee_shm *tee_shm_get_parent_shm(struct tee_shm *shm, size_t *offs); > > > > int tee_heap_update_from_dma_buf(struct tee_device *teedev, > > struct dma_buf *dmabuf, size_t *offset, > > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c > > index daf6e5cfd59a..8b79918468b5 100644 > > --- a/drivers/tee/tee_shm.c > > +++ b/drivers/tee/tee_shm.c > > @@ -4,6 +4,7 @@ > > */ > > #include <linux/anon_inodes.h> > > #include <linux/device.h> > > +#include <linux/dma-buf.h> > > #include <linux/idr.h> > > #include <linux/io.h> > > #include <linux/mm.h> > > @@ -15,6 +16,16 @@ > > #include <linux/highmem.h> > > #include "tee_private.h" > > > > +/* extra references appended to shm object for registered shared memory */ > > +struct tee_shm_dmabuf_ref { > > + struct tee_shm shm; > > + size_t offset; > > + struct dma_buf *dmabuf; > > + struct dma_buf_attachment *attach; > > + struct sg_table *sgt; > > + struct tee_shm *parent_shm; > > +}; > > + > > static void shm_put_kernel_pages(struct page **pages, size_t page_count) > > { > > size_t n; > > @@ -45,7 +56,23 @@ static void release_registered_pages(struct tee_shm *shm) > > > > static void tee_shm_release(struct tee_device *teedev, struct tee_shm *shm) > > { > > - if (shm->flags & TEE_SHM_POOL) { > > + struct tee_shm *parent_shm = NULL; > > + void *p = shm; > > + > > + if (shm->flags & TEE_SHM_DMA_BUF) { > > + struct tee_shm_dmabuf_ref *ref; > > + > > + ref = container_of(shm, struct tee_shm_dmabuf_ref, shm); > > + parent_shm = ref->parent_shm; > > + p = ref; > > + if (ref->attach) { > > + dma_buf_unmap_attachment(ref->attach, ref->sgt, > > + DMA_BIDIRECTIONAL); > > + > > + dma_buf_detach(ref->dmabuf, ref->attach); > > + } > > + dma_buf_put(ref->dmabuf); > > + } else if (shm->flags & TEE_SHM_POOL) { > > teedev->pool->ops->free(teedev->pool, shm); > > } else if (shm->flags & TEE_SHM_DYNAMIC) { > > int rc = teedev->desc->ops->shm_unregister(shm->ctx, shm); > > @@ -57,9 +84,10 @@ static void tee_shm_release(struct tee_device *teedev, struct tee_shm *shm) > > release_registered_pages(shm); > > } > > > > - teedev_ctx_put(shm->ctx); > > + if (shm->ctx) > > + teedev_ctx_put(shm->ctx); > > > > - kfree(shm); > > + kfree(p); > > > > tee_device_put(teedev); > > } > > @@ -169,7 +197,7 @@ struct tee_shm *tee_shm_alloc_user_buf(struct tee_context *ctx, size_t size) > > * tee_client_invoke_func(). The memory allocated is later freed with a > > * call to tee_shm_free(). > > * > > - * @returns a pointer to 'struct tee_shm' > > + * @returns a pointer to 'struct tee_shm' on success, and ERR_PTR on failure > > */ > > struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size) > > { > > @@ -179,6 +207,116 @@ struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size) > > } > > EXPORT_SYMBOL_GPL(tee_shm_alloc_kernel_buf); > > > > +struct tee_shm *tee_shm_register_fd(struct tee_context *ctx, int fd) > > +{ > > + struct tee_shm_dmabuf_ref *ref; > > + int rc; > > + > > + if (!tee_device_get(ctx->teedev)) > > + return ERR_PTR(-EINVAL); > > + > > + teedev_ctx_get(ctx); > > + > > + ref = kzalloc(sizeof(*ref), GFP_KERNEL); > > + if (!ref) { > > + rc = -ENOMEM; > > + goto err_put_tee; > > + } > > + > > + refcount_set(&ref->shm.refcount, 1); > > + ref->shm.ctx = ctx; > > + ref->shm.id = -1; > > + ref->shm.flags = TEE_SHM_DMA_BUF; > > + > > + ref->dmabuf = dma_buf_get(fd); > > + if (IS_ERR(ref->dmabuf)) { > > + rc = PTR_ERR(ref->dmabuf); > > + goto err_kfree_ref; > > + } > > + > > + rc = tee_heap_update_from_dma_buf(ctx->teedev, ref->dmabuf, > > + &ref->offset, &ref->shm, > > + &ref->parent_shm); > > + if (!rc) > > + goto out; > > + if (rc != -EINVAL) > > + goto err_put_dmabuf; > > + > > + ref->attach = dma_buf_attach(ref->dmabuf, &ctx->teedev->dev); > > + if (IS_ERR(ref->attach)) { > > + rc = PTR_ERR(ref->attach); > > + goto err_put_dmabuf; > > + } > > + > > + ref->sgt = dma_buf_map_attachment(ref->attach, DMA_BIDIRECTIONAL); > > + if (IS_ERR(ref->sgt)) { > > + rc = PTR_ERR(ref->sgt); > > + goto err_detach; > > + } > > + > > + if (sg_nents(ref->sgt->sgl) != 1) { > > + rc = PTR_ERR(ref->sgt->sgl); > > + goto err_unmap_attachement; > > + } > > + > > + ref->shm.paddr = page_to_phys(sg_page(ref->sgt->sgl)); > > + ref->shm.size = ref->sgt->sgl->length; > > + > > +out: > > + mutex_lock(&ref->shm.ctx->teedev->mutex); > > + ref->shm.id = idr_alloc(&ref->shm.ctx->teedev->idr, &ref->shm, > > + 1, 0, GFP_KERNEL); > > + mutex_unlock(&ref->shm.ctx->teedev->mutex); > > + if (ref->shm.id < 0) { > > + rc = ref->shm.id; > > + if (ref->attach) > > + goto err_unmap_attachement; > > + goto err_put_dmabuf; > > + } > > + > > + return &ref->shm; > > + > > +err_unmap_attachement: > > + dma_buf_unmap_attachment(ref->attach, ref->sgt, DMA_BIDIRECTIONAL); > > +err_detach: > > + dma_buf_detach(ref->dmabuf, ref->attach); > > +err_put_dmabuf: > > + dma_buf_put(ref->dmabuf); > > +err_kfree_ref: > > + kfree(ref); > > +err_put_tee: > > + teedev_ctx_put(ctx); > > + tee_device_put(ctx->teedev); > > + > > + return ERR_PTR(rc); > > +} > > +EXPORT_SYMBOL_GPL(tee_shm_register_fd); > > + > > +struct tee_shm *tee_shm_get_parent_shm(struct tee_shm *shm, size_t *offs) > > +{ > > + struct tee_shm *parent_shm = NULL; > > + > > + if (shm->flags & TEE_SHM_DMA_BUF) { > > + struct tee_shm_dmabuf_ref *ref; > > + > > + ref = container_of(shm, struct tee_shm_dmabuf_ref, shm); > > + if (ref->parent_shm) { > > + /* > > + * the shm already has one reference to > > + * ref->parent_shm so we should be clear of 0. > > + * We're getting another reference since the caller > > + * of this function expects to put the returned > > + * parent_shm when it's done with it. > > + */ > > + parent_shm = ref->parent_shm; > > + refcount_inc(&parent_shm->refcount); > > + *offs = ref->offset; > > + } > > + } > > + > > + return parent_shm; > > +} > > + > > /** > > * tee_shm_alloc_priv_buf() - Allocate shared memory for a privately shared > > * kernel buffer > > diff --git a/include/linux/tee_core.h b/include/linux/tee_core.h > > index 16ef078247ae..6bd833b6d0e1 100644 > > --- a/include/linux/tee_core.h > > +++ b/include/linux/tee_core.h > > @@ -28,6 +28,7 @@ > > #define TEE_SHM_USER_MAPPED BIT(1) /* Memory mapped in user space */ > > #define TEE_SHM_POOL BIT(2) /* Memory allocated from pool */ > > #define TEE_SHM_PRIV BIT(3) /* Memory private to TEE driver */ > > +#define TEE_SHM_DMA_BUF BIT(4) /* Memory with dma-buf handle */ > > > > #define TEE_DEVICE_FLAG_REGISTERED 0x1 > > #define TEE_MAX_DEV_NAME_LEN 32 > > diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h > > index a54c203000ed..824f1251de60 100644 > > --- a/include/linux/tee_drv.h > > +++ b/include/linux/tee_drv.h > > @@ -116,6 +116,16 @@ struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size); > > struct tee_shm *tee_shm_register_kernel_buf(struct tee_context *ctx, > > void *addr, size_t length); > > > > +/** > > + * tee_shm_register_fd() - Register shared memory from file descriptor > > + * > > + * @ctx: Context that allocates the shared memory > > + * @fd: Shared memory file descriptor reference > > + * > > + * @returns a pointer to 'struct tee_shm' on success, and ERR_PTR on failure > > + */ > > +struct tee_shm *tee_shm_register_fd(struct tee_context *ctx, int fd); > > + > > /** > > * tee_shm_free() - Free shared memory > > * @shm: Handle to shared memory to free > > diff --git a/include/uapi/linux/tee.h b/include/uapi/linux/tee.h > > index d0430bee8292..1f9a4ac2b211 100644 > > --- a/include/uapi/linux/tee.h > > +++ b/include/uapi/linux/tee.h > > @@ -118,6 +118,35 @@ struct tee_ioctl_shm_alloc_data { > > #define TEE_IOC_SHM_ALLOC _IOWR(TEE_IOC_MAGIC, TEE_IOC_BASE + 1, \ > > struct tee_ioctl_shm_alloc_data) > > > > +/** > > + * struct tee_ioctl_shm_register_fd_data - Shared memory registering argument > > + * @fd: [in] File descriptor identifying the shared memory > > + * @size: [out] Size of shared memory to allocate > > + * @flags: [in] Flags to/from allocation. > > + * @id: [out] Identifier of the shared memory > > + * > > + * The flags field should currently be zero as input. Updated by the call > > + * with actual flags as defined by TEE_IOCTL_SHM_* above. > > + * This structure is used as argument for TEE_IOC_SHM_REGISTER_FD below. > > + */ > > +struct tee_ioctl_shm_register_fd_data { > > + __s64 fd; > > + __u64 size; > > + __u32 flags; > > + __s32 id; > > +}; > > + > > +/** > > + * TEE_IOC_SHM_REGISTER_FD - register a shared memory from a file descriptor > > + * > > + * Returns a file descriptor on success or < 0 on failure > > + * > > + * The returned file descriptor refers to the shared memory object in kernel > > + * land. The shared memory is freed when the descriptor is closed. > > + */ > > +#define TEE_IOC_SHM_REGISTER_FD _IOWR(TEE_IOC_MAGIC, TEE_IOC_BASE + 8, \ > > + struct tee_ioctl_shm_register_fd_data) > > + > > /** > > * struct tee_ioctl_buf_data - Variable sized buffer > > * @buf_ptr: [in] A __user pointer to a buffer > > -- > > 2.43.0 > >

3 months, 1 week

Re: [PATCH v6 05/10] tee: implement restricted DMA-heap

by Jens Wiklander

Hi Sumit, On Tue, Mar 25, 2025 at 7:33 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > Hi Jens, > > On Wed, Mar 05, 2025 at 02:04:11PM +0100, Jens Wiklander wrote: > > Implement DMA heap for restricted DMA-buf allocation in the TEE > > subsystem. > > > > Restricted memory refers to memory buffers behind a hardware enforced > > firewall. It is not accessible to the kernel during normal circumstances > > but rather only accessible to certain hardware IPs or CPUs executing in > > higher or differently privileged mode than the kernel itself. This > > interface allows to allocate and manage such restricted memory buffers > > via interaction with a TEE implementation. > > > > The restricted memory is allocated for a specific use-case, like Secure > > Video Playback, Trusted UI, or Secure Video Recording where certain > > hardware devices can access the memory. > > > > The DMA-heaps are enabled explicitly by the TEE backend driver. The TEE > > backend drivers needs to implement restricted memory pool to manage the > > restricted memory. > > > > Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> > > --- > > drivers/tee/Makefile | 1 + > > drivers/tee/tee_heap.c | 470 ++++++++++++++++++++++++++++++++++++++ > > drivers/tee/tee_private.h | 6 + > > include/linux/tee_core.h | 62 +++++ > > 4 files changed, 539 insertions(+) > > create mode 100644 drivers/tee/tee_heap.c > > > > diff --git a/drivers/tee/Makefile b/drivers/tee/Makefile > > index 5488cba30bd2..949a6a79fb06 100644 > > --- a/drivers/tee/Makefile > > +++ b/drivers/tee/Makefile > > @@ -1,6 +1,7 @@ > > # SPDX-License-Identifier: GPL-2.0 > > obj-$(CONFIG_TEE) += tee.o > > tee-objs += tee_core.o > > +tee-objs += tee_heap.o > > tee-objs += tee_shm.o > > tee-objs += tee_shm_pool.o > > obj-$(CONFIG_OPTEE) += optee/ > > diff --git a/drivers/tee/tee_heap.c b/drivers/tee/tee_heap.c > > new file mode 100644 > > index 000000000000..476ab2e27260 > > --- /dev/null > > +++ b/drivers/tee/tee_heap.c > > @@ -0,0 +1,470 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Copyright (c) 2025, Linaro Limited > > + */ > > + > > +#include <linux/scatterlist.h> > > +#include <linux/dma-buf.h> > > +#include <linux/dma-heap.h> > > +#include <linux/genalloc.h> > > +#include <linux/module.h> > > +#include <linux/scatterlist.h> > > +#include <linux/slab.h> > > +#include <linux/tee_core.h> > > +#include <linux/xarray.h> > > Lets try to follow alphabetical order here. Sure > > > + > > +#include "tee_private.h" > > + > > +struct tee_dma_heap { > > + struct dma_heap *heap; > > + enum tee_dma_heap_id id; > > + struct tee_rstmem_pool *pool; > > + struct tee_device *teedev; > > + /* Protects pool and teedev above */ > > + struct mutex mu; > > +}; > > + > > +struct tee_heap_buffer { > > + struct tee_rstmem_pool *pool; > > + struct tee_device *teedev; > > + size_t size; > > + size_t offs; > > + struct sg_table table; > > +}; > > + > > +struct tee_heap_attachment { > > + struct sg_table table; > > + struct device *dev; > > +}; > > + > > +struct tee_rstmem_static_pool { > > + struct tee_rstmem_pool pool; > > + struct gen_pool *gen_pool; > > + phys_addr_t pa_base; > > +}; > > + > > +#if !IS_MODULE(CONFIG_TEE) && IS_ENABLED(CONFIG_DMABUF_HEAPS) > > Can this dependency rather be better managed via Kconfig? This was the easiest yet somewhat flexible solution I could find. If you have something better, let's use that instead. > > > +static DEFINE_XARRAY_ALLOC(tee_dma_heap); > > + > > +static int copy_sg_table(struct sg_table *dst, struct sg_table *src) > > +{ > > + struct scatterlist *dst_sg; > > + struct scatterlist *src_sg; > > + int ret; > > + int i; > > + > > + ret = sg_alloc_table(dst, src->orig_nents, GFP_KERNEL); > > + if (ret) > > + return ret; > > + > > + dst_sg = dst->sgl; > > + for_each_sgtable_sg(src, src_sg, i) { > > + sg_set_page(dst_sg, sg_page(src_sg), src_sg->length, > > + src_sg->offset); > > + dst_sg = sg_next(dst_sg); > > + } > > + > > + return 0; > > +} > > + > > +static int tee_heap_attach(struct dma_buf *dmabuf, > > + struct dma_buf_attachment *attachment) > > +{ > > + struct tee_heap_buffer *buf = dmabuf->priv; > > + struct tee_heap_attachment *a; > > + int ret; > > + > > + a = kzalloc(sizeof(*a), GFP_KERNEL); > > + if (!a) > > + return -ENOMEM; > > + > > + ret = copy_sg_table(&a->table, &buf->table); > > + if (ret) { > > + kfree(a); > > + return ret; > > + } > > + > > + a->dev = attachment->dev; > > + attachment->priv = a; > > + > > + return 0; > > +} > > + > > +static void tee_heap_detach(struct dma_buf *dmabuf, > > + struct dma_buf_attachment *attachment) > > +{ > > + struct tee_heap_attachment *a = attachment->priv; > > + > > + sg_free_table(&a->table); > > + kfree(a); > > +} > > + > > +static struct sg_table * > > +tee_heap_map_dma_buf(struct dma_buf_attachment *attachment, > > + enum dma_data_direction direction) > > +{ > > + struct tee_heap_attachment *a = attachment->priv; > > + int ret; > > + > > + ret = dma_map_sgtable(attachment->dev, &a->table, direction, > > + DMA_ATTR_SKIP_CPU_SYNC); > > + if (ret) > > + return ERR_PTR(ret); > > + > > + return &a->table; > > +} > > + > > +static void tee_heap_unmap_dma_buf(struct dma_buf_attachment *attachment, > > + struct sg_table *table, > > + enum dma_data_direction direction) > > +{ > > + struct tee_heap_attachment *a = attachment->priv; > > + > > + WARN_ON(&a->table != table); > > + > > + dma_unmap_sgtable(attachment->dev, table, direction, > > + DMA_ATTR_SKIP_CPU_SYNC); > > +} > > + > > +static void tee_heap_buf_free(struct dma_buf *dmabuf) > > +{ > > + struct tee_heap_buffer *buf = dmabuf->priv; > > + struct tee_device *teedev = buf->teedev; > > + > > + buf->pool->ops->free(buf->pool, &buf->table); > > + tee_device_put(teedev); > > +} > > + > > +static const struct dma_buf_ops tee_heap_buf_ops = { > > + .attach = tee_heap_attach, > > + .detach = tee_heap_detach, > > + .map_dma_buf = tee_heap_map_dma_buf, > > + .unmap_dma_buf = tee_heap_unmap_dma_buf, > > + .release = tee_heap_buf_free, > > +}; > > + > > +static struct dma_buf *tee_dma_heap_alloc(struct dma_heap *heap, > > + unsigned long len, u32 fd_flags, > > + u64 heap_flags) > > +{ > > + struct tee_dma_heap *h = dma_heap_get_drvdata(heap); > > + DEFINE_DMA_BUF_EXPORT_INFO(exp_info); > > + struct tee_device *teedev = NULL; > > + struct tee_heap_buffer *buf; > > + struct tee_rstmem_pool *pool; > > + struct dma_buf *dmabuf; > > + int rc; > > + > > + mutex_lock(&h->mu); > > + if (tee_device_get(h->teedev)) { > > + teedev = h->teedev; > > + pool = h->pool; > > + } > > + mutex_unlock(&h->mu); > > + > > + if (!teedev) > > + return ERR_PTR(-EINVAL); > > + > > + buf = kzalloc(sizeof(*buf), GFP_KERNEL); > > + if (!buf) { > > + dmabuf = ERR_PTR(-ENOMEM); > > + goto err; > > + } > > + buf->size = len; > > + buf->pool = pool; > > + buf->teedev = teedev; > > + > > + rc = pool->ops->alloc(pool, &buf->table, len, &buf->offs); > > + if (rc) { > > + dmabuf = ERR_PTR(rc); > > + goto err_kfree; > > + } > > + > > + exp_info.ops = &tee_heap_buf_ops; > > + exp_info.size = len; > > + exp_info.priv = buf; > > + exp_info.flags = fd_flags; > > + dmabuf = dma_buf_export(&exp_info); > > + if (IS_ERR(dmabuf)) > > + goto err_rstmem_free; > > + > > + return dmabuf; > > + > > +err_rstmem_free: > > + pool->ops->free(pool, &buf->table); > > +err_kfree: > > + kfree(buf); > > +err: > > + tee_device_put(h->teedev); > > + return dmabuf; > > +} > > + > > +static const struct dma_heap_ops tee_dma_heap_ops = { > > + .allocate = tee_dma_heap_alloc, > > +}; > > + > > +static const char *heap_id_2_name(enum tee_dma_heap_id id) > > +{ > > + switch (id) { > > + case TEE_DMA_HEAP_SECURE_VIDEO_PLAY: > > + return "restricted,secure-video"; > > + case TEE_DMA_HEAP_TRUSTED_UI: > > + return "restricted,trusted-ui"; > > + case TEE_DMA_HEAP_SECURE_VIDEO_RECORD: > > + return "restricted,secure-video-record"; > > + default: > > + return NULL; > > + } > > +} > > + > > +static int alloc_dma_heap(struct tee_device *teedev, enum tee_dma_heap_id id, > > + struct tee_rstmem_pool *pool) > > +{ > > + struct dma_heap_export_info exp_info = { > > + .ops = &tee_dma_heap_ops, > > + .name = heap_id_2_name(id), > > + }; > > + struct tee_dma_heap *h; > > + int rc; > > + > > + if (!exp_info.name) > > + return -EINVAL; > > + > > + if (xa_reserve(&tee_dma_heap, id, GFP_KERNEL)) { > > + if (!xa_load(&tee_dma_heap, id)) > > + return -EEXIST; > > + return -ENOMEM; > > + } > > + > > + h = kzalloc(sizeof(*h), GFP_KERNEL); > > + if (!h) > > + return -ENOMEM; > > + h->id = id; > > + h->teedev = teedev; > > + h->pool = pool; > > + mutex_init(&h->mu); > > + > > + exp_info.priv = h; > > + h->heap = dma_heap_add(&exp_info); > > + if (IS_ERR(h->heap)) { > > + rc = PTR_ERR(h->heap); > > + kfree(h); > > + > > + return rc; > > + } > > + > > + /* "can't fail" due to the call to xa_reserve() above */ > > + return WARN(xa_store(&tee_dma_heap, id, h, GFP_KERNEL), > > + "xa_store() failed"); > > +} > > + > > +int tee_device_register_dma_heap(struct tee_device *teedev, > > + enum tee_dma_heap_id id, > > + struct tee_rstmem_pool *pool) > > +{ > > + struct tee_dma_heap *h; > > + int rc; > > + > > + h = xa_load(&tee_dma_heap, id); > > + if (h) { > > + mutex_lock(&h->mu); > > + if (h->teedev) { > > + rc = -EBUSY; > > + } else { > > + h->teedev = teedev; > > + h->pool = pool; > > + rc = 0; > > + } > > + mutex_unlock(&h->mu); > > + } else { > > + rc = alloc_dma_heap(teedev, id, pool); > > + } > > + > > + if (rc) > > + dev_err(&teedev->dev, "can't register DMA heap id %d (%s)\n", > > + id, heap_id_2_name(id)); > > + > > + return rc; > > +} > > + > > +void tee_device_unregister_all_dma_heaps(struct tee_device *teedev) > > +{ > > + struct tee_rstmem_pool *pool; > > + struct tee_dma_heap *h; > > + u_long i; > > + > > + xa_for_each(&tee_dma_heap, i, h) { > > + if (h) { > > + pool = NULL; > > + mutex_lock(&h->mu); > > + if (h->teedev == teedev) { > > + pool = h->pool; > > + h->teedev = NULL; > > + h->pool = NULL; > > + } > > + mutex_unlock(&h->mu); > > + if (pool) > > + pool->ops->destroy_pool(pool); > > + } > > + } > > +} > > +EXPORT_SYMBOL_GPL(tee_device_unregister_all_dma_heaps); > > + > > +int tee_heap_update_from_dma_buf(struct tee_device *teedev, > > + struct dma_buf *dmabuf, size_t *offset, > > + struct tee_shm *shm, > > + struct tee_shm **parent_shm) > > +{ > > + struct tee_heap_buffer *buf; > > + int rc; > > + > > + /* The DMA-buf must be from our heap */ > > + if (dmabuf->ops != &tee_heap_buf_ops) > > + return -EINVAL; > > + > > + buf = dmabuf->priv; > > + /* The buffer must be from the same teedev */ > > + if (buf->teedev != teedev) > > + return -EINVAL; > > + > > + shm->size = buf->size; > > + > > + rc = buf->pool->ops->update_shm(buf->pool, &buf->table, buf->offs, shm, > > + parent_shm); > > + if (!rc && *parent_shm) > > + *offset = buf->offs; > > + > > + return rc; > > +} > > +#else > > +int tee_device_register_dma_heap(struct tee_device *teedev __always_unused, > > + enum tee_dma_heap_id id __always_unused, > > + struct tee_rstmem_pool *pool __always_unused) > > +{ > > + return -EINVAL; > > +} > > +EXPORT_SYMBOL_GPL(tee_device_register_dma_heap); > > + > > +void > > +tee_device_unregister_all_dma_heaps(struct tee_device *teedev __always_unused) > > +{ > > +} > > +EXPORT_SYMBOL_GPL(tee_device_unregister_all_dma_heaps); > > + > > +int tee_heap_update_from_dma_buf(struct tee_device *teedev __always_unused, > > + struct dma_buf *dmabuf __always_unused, > > + size_t *offset __always_unused, > > + struct tee_shm *shm __always_unused, > > + struct tee_shm **parent_shm __always_unused) > > +{ > > + return -EINVAL; > > +} > > +#endif > > + > > +static struct tee_rstmem_static_pool * > > +to_rstmem_static_pool(struct tee_rstmem_pool *pool) > > +{ > > + return container_of(pool, struct tee_rstmem_static_pool, pool); > > +} > > + > > +static int rstmem_pool_op_static_alloc(struct tee_rstmem_pool *pool, > > + struct sg_table *sgt, size_t size, > > + size_t *offs) > > +{ > > + struct tee_rstmem_static_pool *stp = to_rstmem_static_pool(pool); > > + phys_addr_t pa; > > + int ret; > > + > > + pa = gen_pool_alloc(stp->gen_pool, size); > > + if (!pa) > > + return -ENOMEM; > > + > > + ret = sg_alloc_table(sgt, 1, GFP_KERNEL); > > + if (ret) { > > + gen_pool_free(stp->gen_pool, pa, size); > > + return ret; > > + } > > + > > + sg_set_page(sgt->sgl, phys_to_page(pa), size, 0); > > + *offs = pa - stp->pa_base; > > + > > + return 0; > > +} > > + > > +static void rstmem_pool_op_static_free(struct tee_rstmem_pool *pool, > > + struct sg_table *sgt) > > +{ > > + struct tee_rstmem_static_pool *stp = to_rstmem_static_pool(pool); > > + struct scatterlist *sg; > > + int i; > > + > > + for_each_sgtable_sg(sgt, sg, i) > > + gen_pool_free(stp->gen_pool, sg_phys(sg), sg->length); > > + sg_free_table(sgt); > > +} > > + > > +static int rstmem_pool_op_static_update_shm(struct tee_rstmem_pool *pool, > > + struct sg_table *sgt, size_t offs, > > + struct tee_shm *shm, > > + struct tee_shm **parent_shm) > > +{ > > + struct tee_rstmem_static_pool *stp = to_rstmem_static_pool(pool); > > + > > + shm->paddr = stp->pa_base + offs; > > + *parent_shm = NULL; > > + > > + return 0; > > +} > > + > > +static void rstmem_pool_op_static_destroy_pool(struct tee_rstmem_pool *pool) > > +{ > > + struct tee_rstmem_static_pool *stp = to_rstmem_static_pool(pool); > > + > > + gen_pool_destroy(stp->gen_pool); > > + kfree(stp); > > +} > > + > > +static struct tee_rstmem_pool_ops rstmem_pool_ops_static = { > > + .alloc = rstmem_pool_op_static_alloc, > > + .free = rstmem_pool_op_static_free, > > + .update_shm = rstmem_pool_op_static_update_shm, > > + .destroy_pool = rstmem_pool_op_static_destroy_pool, > > +}; > > + > > +struct tee_rstmem_pool *tee_rstmem_static_pool_alloc(phys_addr_t paddr, > > + size_t size) > > +{ > > + const size_t page_mask = PAGE_SIZE - 1; > > + struct tee_rstmem_static_pool *stp; > > + int rc; > > + > > + /* Check it's page aligned */ > > + if ((paddr | size) & page_mask) > > + return ERR_PTR(-EINVAL); > > + > > + stp = kzalloc(sizeof(*stp), GFP_KERNEL); > > + if (!stp) > > + return ERR_PTR(-ENOMEM); > > + > > + stp->gen_pool = gen_pool_create(PAGE_SHIFT, -1); > > + if (!stp->gen_pool) { > > + rc = -ENOMEM; > > + goto err_free; > > + } > > + > > + rc = gen_pool_add(stp->gen_pool, paddr, size, -1); > > + if (rc) > > + goto err_free_pool; > > + > > + stp->pool.ops = &rstmem_pool_ops_static; > > + stp->pa_base = paddr; > > + return &stp->pool; > > + > > +err_free_pool: > > + gen_pool_destroy(stp->gen_pool); > > +err_free: > > + kfree(stp); > > + > > + return ERR_PTR(rc); > > +} > > +EXPORT_SYMBOL_GPL(tee_rstmem_static_pool_alloc); > > diff --git a/drivers/tee/tee_private.h b/drivers/tee/tee_private.h > > index 9bc50605227c..6c6ff5d5eed2 100644 > > --- a/drivers/tee/tee_private.h > > +++ b/drivers/tee/tee_private.h > > @@ -8,6 +8,7 @@ > > #include <linux/cdev.h> > > #include <linux/completion.h> > > #include <linux/device.h> > > +#include <linux/dma-buf.h> > > #include <linux/kref.h> > > #include <linux/mutex.h> > > #include <linux/types.h> > > @@ -24,4 +25,9 @@ struct tee_shm *tee_shm_alloc_user_buf(struct tee_context *ctx, size_t size); > > struct tee_shm *tee_shm_register_user_buf(struct tee_context *ctx, > > unsigned long addr, size_t length); > > > > +int tee_heap_update_from_dma_buf(struct tee_device *teedev, > > + struct dma_buf *dmabuf, size_t *offset, > > + struct tee_shm *shm, > > + struct tee_shm **parent_shm); > > + > > #endif /*TEE_PRIVATE_H*/ > > diff --git a/include/linux/tee_core.h b/include/linux/tee_core.h > > index a38494d6b5f4..16ef078247ae 100644 > > --- a/include/linux/tee_core.h > > +++ b/include/linux/tee_core.h > > @@ -8,9 +8,11 @@ > > > > #include <linux/cdev.h> > > #include <linux/device.h> > > +#include <linux/dma-buf.h> > > #include <linux/idr.h> > > #include <linux/kref.h> > > #include <linux/list.h> > > +#include <linux/scatterlist.h> > > #include <linux/tee.h> > > #include <linux/tee_drv.h> > > #include <linux/types.h> > > @@ -30,6 +32,12 @@ > > #define TEE_DEVICE_FLAG_REGISTERED 0x1 > > #define TEE_MAX_DEV_NAME_LEN 32 > > > > +enum tee_dma_heap_id { > > + TEE_DMA_HEAP_SECURE_VIDEO_PLAY = 1, > > + TEE_DMA_HEAP_TRUSTED_UI, > > + TEE_DMA_HEAP_SECURE_VIDEO_RECORD, > > +}; > > + > > /** > > * struct tee_device - TEE Device representation > > * @name: name of device > > @@ -116,6 +124,33 @@ struct tee_desc { > > u32 flags; > > }; > > > > +/** > > + * struct tee_rstmem_pool - restricted memory pool > > + * @ops: operations > > + * > > + * This is an abstract interface where this struct is expected to be > > + * embedded in another struct specific to the implementation. > > + */ > > +struct tee_rstmem_pool { > > + const struct tee_rstmem_pool_ops *ops; > > +}; > > + > > +/** > > + * struct tee_rstmem_pool_ops - restricted memory pool operations > > + * @alloc: called when allocating restricted memory > > + * @free: called when freeing restricted memory > > + * @destroy_pool: called when destroying the pool > > + */ > > +struct tee_rstmem_pool_ops { > > + int (*alloc)(struct tee_rstmem_pool *pool, struct sg_table *sgt, > > + size_t size, size_t *offs); > > + void (*free)(struct tee_rstmem_pool *pool, struct sg_table *sgt); > > + int (*update_shm)(struct tee_rstmem_pool *pool, struct sg_table *sgt, > > + size_t offs, struct tee_shm *shm, > > + struct tee_shm **parent_shm); > > This API isn't descibed in kdoc comment above. Can you descibe the role > of this API and when it's needed? Yes, I'll add something. Cheers, Jens > > -Sumit > > > + void (*destroy_pool)(struct tee_rstmem_pool *pool); > > +}; > > + > > /** > > * tee_device_alloc() - Allocate a new struct tee_device instance > > * @teedesc: Descriptor for this driver > > @@ -154,6 +189,11 @@ int tee_device_register(struct tee_device *teedev); > > */ > > void tee_device_unregister(struct tee_device *teedev); > > > > +int tee_device_register_dma_heap(struct tee_device *teedev, > > + enum tee_dma_heap_id id, > > + struct tee_rstmem_pool *pool); > > +void tee_device_unregister_all_dma_heaps(struct tee_device *teedev); > > + > > /** > > * tee_device_set_dev_groups() - Set device attribute groups > > * @teedev: Device to register > > @@ -229,6 +269,28 @@ static inline void tee_shm_pool_free(struct tee_shm_pool *pool) > > pool->ops->destroy_pool(pool); > > } > > > > +/** > > + * tee_rstmem_static_pool_alloc() - Create a restricted memory manager > > + * @paddr: Physical address of start of pool > > + * @size: Size in bytes of the pool > > + * > > + * @returns pointer to a 'struct tee_shm_pool' or an ERR_PTR on failure. > > + */ > > +struct tee_rstmem_pool *tee_rstmem_static_pool_alloc(phys_addr_t paddr, > > + size_t size); > > + > > +/** > > + * tee_rstmem_pool_free() - Free a restricted memory pool > > + * @pool: The restricted memory pool to free > > + * > > + * There must be no remaining restricted memory allocated from this pool > > + * when this function is called. > > + */ > > +static inline void tee_rstmem_pool_free(struct tee_rstmem_pool *pool) > > +{ > > + pool->ops->destroy_pool(pool); > > +} > > + > > /** > > * tee_get_drvdata() - Return driver_data pointer > > * @returns the driver_data pointer supplied to tee_register(). > > -- > > 2.43.0 > >

3 months, 1 week

Re: [PATCH] udmabuf: fix a buf size overflow issue during udmabuf creation

by Christian König

Am 25.03.25 um 07:23 schrieb Kasireddy, Vivek: > Hi Christian, > >> Am 21.03.25 um 17:41 schrieb Xiaogang.Chen: >>> From: Xiaogang Chen <xiaogang.chen(a)amd.com> >>> >>> by casting size_limit_mb to u64 when calculate pglimit. >>> >>> Signed-off-by: Xiaogang Chen<Xiaogang.Chen(a)amd.com> >> Reviewed-by: Christian König <christian.koenig(a)amd.com> >> >> If nobody objects I'm going to push that to drm-misc-fixes. > No objection but I wish the author would have added more details in the commit > message particularly the value they have used to trigger the overflow. I guess > Xiaogang can still comment here and briefly describe the exact use-case/test-case > they are running where they encountered this issue. Isn't that obvious? At least it was for me. As soon as you have a value larger than 4095 the 32bit multiplication overflows, resulting in incorrectly limiting the buffer size. Regards, Christian. > > Thanks, > Vivek > >> Regards, >> Christian. >> >>> --- >>> drivers/dma-buf/udmabuf.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c >>> index 8ce1f074c2d3..e99e3a65a470 100644 >>> --- a/drivers/dma-buf/udmabuf.c >>> +++ b/drivers/dma-buf/udmabuf.c >>> @@ -398,7 +398,7 @@ static long udmabuf_create(struct miscdevice >> *device, >>> if (!ubuf) >>> return -ENOMEM; >>> >>> - pglimit = (size_limit_mb * 1024 * 1024) >> PAGE_SHIFT; >>> + pglimit = ((u64)size_limit_mb * 1024 * 1024) >> PAGE_SHIFT; >>> for (i = 0; i < head->count; i++) { >>> pgoff_t subpgcnt; >>>

3 months, 1 week

Re: [PATCH v6 03/10] optee: account for direction while converting parameters

by Jens Wiklander

On Tue, Mar 25, 2025 at 6:56 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > On Thu, Mar 20, 2025 at 02:00:57PM +0100, Jens Wiklander wrote: > > Hi Sumit, > > > > On Thu, Mar 20, 2025 at 10:25 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > > > > > Hi Jens, > > > > > > On Mon, Mar 17, 2025 at 08:42:01AM +0100, Jens Wiklander wrote: > > > > Hi Sumit, > > > > > > > > On Thu, Mar 13, 2025 at 11:41 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > > > > > > > > > Hi Jens, > > > > > > > > > > On Wed, Mar 05, 2025 at 02:04:09PM +0100, Jens Wiklander wrote: > > > > > > The OP-TEE backend driver has two internal function pointers to convert > > > > > > between the subsystem type struct tee_param and the OP-TEE type struct > > > > > > optee_msg_param. > > > > > > > > > > > > The conversion is done from one of the types to the other, which is then > > > > > > involved in some operation and finally converted back to the original > > > > > > type. When converting to prepare the parameters for the operation, all > > > > > > fields must be taken into account, but then converting back, it's enough > > > > > > to update only out-values and out-sizes. So, an update_out parameter is > > > > > > added to the conversion functions to tell if all or only some fields > > > > > > must be copied. > > > > > > > > > > > > This is needed in a later patch where it might get confusing when > > > > > > converting back in from_msg_param() callback since an allocated > > > > > > restricted SHM can be using the sec_world_id of the used restricted > > > > > > memory pool and that doesn't translate back well. > > > > > > > > > > > > Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> > > > > > > --- > > > > > > drivers/tee/optee/call.c | 10 ++-- > > > > > > drivers/tee/optee/ffa_abi.c | 43 +++++++++++++---- > > > > > > drivers/tee/optee/optee_private.h | 42 +++++++++++------ > > > > > > drivers/tee/optee/rpc.c | 31 +++++++++---- > > > > > > drivers/tee/optee/smc_abi.c | 76 +++++++++++++++++++++++-------- > > > > > > 5 files changed, 144 insertions(+), 58 deletions(-) > > > > > > > > > > > > diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c > > > > > > index 16eb953e14bb..f1533b894726 100644 > > > > > > --- a/drivers/tee/optee/call.c > > > > > > +++ b/drivers/tee/optee/call.c > > > > > > @@ -400,7 +400,8 @@ int optee_open_session(struct tee_context *ctx, > > > > > > export_uuid(msg_arg->params[1].u.octets, &client_uuid); > > > > > > > > > > > > rc = optee->ops->to_msg_param(optee, msg_arg->params + 2, > > > > > > - arg->num_params, param); > > > > > > + arg->num_params, param, > > > > > > + false /*!update_out*/); > > > > > > if (rc) > > > > > > goto out; > > > > > > > > > > > > @@ -427,7 +428,8 @@ int optee_open_session(struct tee_context *ctx, > > > > > > } > > > > > > > > > > > > if (optee->ops->from_msg_param(optee, param, arg->num_params, > > > > > > - msg_arg->params + 2)) { > > > > > > + msg_arg->params + 2, > > > > > > + true /*update_out*/)) { > > > > > > arg->ret = TEEC_ERROR_COMMUNICATION; > > > > > > arg->ret_origin = TEEC_ORIGIN_COMMS; > > > > > > /* Close session again to avoid leakage */ > > > > > > @@ -541,7 +543,7 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg, > > > > > > msg_arg->cancel_id = arg->cancel_id; > > > > > > > > > > > > rc = optee->ops->to_msg_param(optee, msg_arg->params, arg->num_params, > > > > > > - param); > > > > > > + param, false /*!update_out*/); > > > > > > if (rc) > > > > > > goto out; > > > > > > > > > > > > @@ -551,7 +553,7 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg, > > > > > > } > > > > > > > > > > > > if (optee->ops->from_msg_param(optee, param, arg->num_params, > > > > > > - msg_arg->params)) { > > > > > > + msg_arg->params, true /*update_out*/)) { > > > > > > msg_arg->ret = TEEC_ERROR_COMMUNICATION; > > > > > > msg_arg->ret_origin = TEEC_ORIGIN_COMMS; > > > > > > } > > > > > > diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c > > > > > > index 4ca1d5161b82..e4b08cd195f3 100644 > > > > > > --- a/drivers/tee/optee/ffa_abi.c > > > > > > +++ b/drivers/tee/optee/ffa_abi.c > > > > > > @@ -122,15 +122,21 @@ static int optee_shm_rem_ffa_handle(struct optee *optee, u64 global_id) > > > > > > */ > > > > > > > > > > > > static void from_msg_param_ffa_mem(struct optee *optee, struct tee_param *p, > > > > > > - u32 attr, const struct optee_msg_param *mp) > > > > > > + u32 attr, const struct optee_msg_param *mp, > > > > > > + bool update_out) > > > > > > { > > > > > > struct tee_shm *shm = NULL; > > > > > > u64 offs_high = 0; > > > > > > u64 offs_low = 0; > > > > > > > > > > > > + if (update_out) { > > > > > > + if (attr == OPTEE_MSG_ATTR_TYPE_FMEM_INPUT) > > > > > > + return; > > > > > > + goto out; > > > > > > + } > > > > > > + > > > > > > p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT + > > > > > > attr - OPTEE_MSG_ATTR_TYPE_FMEM_INPUT; > > > > > > - p->u.memref.size = mp->u.fmem.size; > > > > > > > > > > > > if (mp->u.fmem.global_id != OPTEE_MSG_FMEM_INVALID_GLOBAL_ID) > > > > > > shm = optee_shm_from_ffa_handle(optee, mp->u.fmem.global_id); > > > > > > @@ -141,6 +147,8 @@ static void from_msg_param_ffa_mem(struct optee *optee, struct tee_param *p, > > > > > > offs_high = mp->u.fmem.offs_high; > > > > > > } > > > > > > p->u.memref.shm_offs = offs_low | offs_high << 32; > > > > > > +out: > > > > > > + p->u.memref.size = mp->u.fmem.size; > > > > > > } > > > > > > > > > > > > /** > > > > > > @@ -150,12 +158,14 @@ static void from_msg_param_ffa_mem(struct optee *optee, struct tee_param *p, > > > > > > * @params: subsystem internal parameter representation > > > > > > * @num_params: number of elements in the parameter arrays > > > > > > * @msg_params: OPTEE_MSG parameters > > > > > > + * @update_out: update parameter for output only > > > > > > * > > > > > > * Returns 0 on success or <0 on failure > > > > > > */ > > > > > > static int optee_ffa_from_msg_param(struct optee *optee, > > > > > > struct tee_param *params, size_t num_params, > > > > > > - const struct optee_msg_param *msg_params) > > > > > > + const struct optee_msg_param *msg_params, > > > > > > + bool update_out) > > > > > > { > > > > > > size_t n; > > > > > > > > > > > > @@ -166,18 +176,20 @@ static int optee_ffa_from_msg_param(struct optee *optee, > > > > > > > > > > > > switch (attr) { > > > > > > case OPTEE_MSG_ATTR_TYPE_NONE: > > > > > > + if (update_out) > > > > > > + break; > > > > > > p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE; > > > > > > memset(&p->u, 0, sizeof(p->u)); > > > > > > break; > > > > > > case OPTEE_MSG_ATTR_TYPE_VALUE_INPUT: > > > > > > case OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT: > > > > > > case OPTEE_MSG_ATTR_TYPE_VALUE_INOUT: > > > > > > - optee_from_msg_param_value(p, attr, mp); > > > > > > + optee_from_msg_param_value(p, attr, mp, update_out); > > > > > > break; > > > > > > case OPTEE_MSG_ATTR_TYPE_FMEM_INPUT: > > > > > > case OPTEE_MSG_ATTR_TYPE_FMEM_OUTPUT: > > > > > > case OPTEE_MSG_ATTR_TYPE_FMEM_INOUT: > > > > > > - from_msg_param_ffa_mem(optee, p, attr, mp); > > > > > > + from_msg_param_ffa_mem(optee, p, attr, mp, update_out); > > > > > > break; > > > > > > default: > > > > > > return -EINVAL; > > > > > > @@ -188,10 +200,16 @@ static int optee_ffa_from_msg_param(struct optee *optee, > > > > > > } > > > > > > > > > > > > static int to_msg_param_ffa_mem(struct optee_msg_param *mp, > > > > > > - const struct tee_param *p) > > > > > > + const struct tee_param *p, bool update_out) > > > > > > { > > > > > > struct tee_shm *shm = p->u.memref.shm; > > > > > > > > > > > > + if (update_out) { > > > > > > + if (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT) > > > > > > + return 0; > > > > > > + goto out; > > > > > > + } > > > > > > + > > > > > > mp->attr = OPTEE_MSG_ATTR_TYPE_FMEM_INPUT + p->attr - > > > > > > TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT; > > > > > > > > > > > > @@ -211,6 +229,7 @@ static int to_msg_param_ffa_mem(struct optee_msg_param *mp, > > > > > > memset(&mp->u, 0, sizeof(mp->u)); > > > > > > mp->u.fmem.global_id = OPTEE_MSG_FMEM_INVALID_GLOBAL_ID; > > > > > > } > > > > > > +out: > > > > > > mp->u.fmem.size = p->u.memref.size; > > > > > > > > > > > > return 0; > > > > > > @@ -222,13 +241,15 @@ static int to_msg_param_ffa_mem(struct optee_msg_param *mp, > > > > > > * @optee: main service struct > > > > > > * @msg_params: OPTEE_MSG parameters > > > > > > * @num_params: number of elements in the parameter arrays > > > > > > - * @params: subsystem itnernal parameter representation > > > > > > + * @params: subsystem internal parameter representation > > > > > > + * @update_out: update parameter for output only > > > > > > * Returns 0 on success or <0 on failure > > > > > > */ > > > > > > static int optee_ffa_to_msg_param(struct optee *optee, > > > > > > struct optee_msg_param *msg_params, > > > > > > size_t num_params, > > > > > > - const struct tee_param *params) > > > > > > + const struct tee_param *params, > > > > > > + bool update_out) > > > > > > { > > > > > > size_t n; > > > > > > > > > > > > @@ -238,18 +259,20 @@ static int optee_ffa_to_msg_param(struct optee *optee, > > > > > > > > > > > > switch (p->attr) { > > > > > > case TEE_IOCTL_PARAM_ATTR_TYPE_NONE: > > > > > > + if (update_out) > > > > > > + break; > > > > > > mp->attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE; > > > > > > memset(&mp->u, 0, sizeof(mp->u)); > > > > > > break; > > > > > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT: > > > > > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT: > > > > > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT: > > > > > > - optee_to_msg_param_value(mp, p); > > > > > > + optee_to_msg_param_value(mp, p, update_out); > > > > > > break; > > > > > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT: > > > > > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT: > > > > > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT: > > > > > > - if (to_msg_param_ffa_mem(mp, p)) > > > > > > + if (to_msg_param_ffa_mem(mp, p, update_out)) > > > > > > return -EINVAL; > > > > > > break; > > > > > > default: > > > > > > > > > > Can we rather handle it as follows to improve code readability and > > > > > maintainence long term? Ditto for all other places. > > > > > > > > > > static int optee_ffa_to_msg_param(struct optee *optee, > > > > > struct optee_msg_param *msg_params, > > > > > size_t num_params, > > > > > const struct tee_param *params, > > > > > bool update_out) > > > > > { > > > > > size_t n; > > > > > > > > > > for (n = 0; n < num_params; n++) { > > > > > const struct tee_param *p = params + n; > > > > > struct optee_msg_param *mp = msg_params + n; > > > > > > > > > > if (update_out && (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_NONE || > > > > > p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT || > > > > > p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT)) > > > > > continue; > > > > > > > > You're missing updating the length field for memrefs. > > > > > > > > > > Do we need to update length field for input memrefs when update_out is > > > set? I don't see that happening in your existing patch too. > > > > I'm sorry, I was unclear. The update_out parameter means only the > > output fields should be updated, not the attribute, offsets, ids, etc. > > That is, the length field for memrefs, and the value fields a, b, c > > for value params. Some of the memrefs aren't translated one-to-one > > with SDP, but the length field can and must be updated. > > Isn't it rather better to add another attribute type to handled SDP > special handling? This isn't special handling, all parameters get the same treatment. When updating a parameter after it has been used, this is all that needs to be done, regardless of whether it's an SDP buffer. The updates we did before this patch were redundant. This patch was introduced in the v3 of this patch set, but I don't think it's strictly needed any longer since SDP buffers are allocated differently now. I think it's nice to only update what's needed when translating back a parameter (just as in params_to_user() in drivers/tee/tee_core.c), but if you don't like it, we can drop this patch. Cheers, Jens > > -Sumit > > > > > Cheers, > > Jens > > > > > > > > -Sumit > > > > > > > Cheers, > > > > Jens > > > > > > > > > > > > > > switch (p->attr) { > > > > > case TEE_IOCTL_PARAM_ATTR_TYPE_NONE: > > > > > mp->attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE; > > > > > memset(&mp->u, 0, sizeof(mp->u)); > > > > > break; > > > > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT: > > > > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT: > > > > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT: > > > > > optee_to_msg_param_value(mp, p); > > > > > break; > > > > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT: > > > > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT: > > > > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT: > > > > > if (to_msg_param_ffa_mem(mp, p)) > > > > > return -EINVAL; > > > > > break; > > > > > default: > > > > > return -EINVAL; > > > > > } > > > > > } > > > > > > > > > > return 0; > > > > > } > > > > > > > > > > -Sumit

3 months, 1 week

Re: [PATCH] udmabuf: fix a buf size overflow issue during udmabuf creation

by Christian König

Am 21.03.25 um 17:41 schrieb Xiaogang.Chen: > From: Xiaogang Chen <xiaogang.chen(a)amd.com> > > by casting size_limit_mb to u64 when calculate pglimit. > > Signed-off-by: Xiaogang Chen<Xiaogang.Chen(a)amd.com> Reviewed-by: Christian König <christian.koenig(a)amd.com> If nobody objects I'm going to push that to drm-misc-fixes. Regards, Christian. > --- > drivers/dma-buf/udmabuf.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c > index 8ce1f074c2d3..e99e3a65a470 100644 > --- a/drivers/dma-buf/udmabuf.c > +++ b/drivers/dma-buf/udmabuf.c > @@ -398,7 +398,7 @@ static long udmabuf_create(struct miscdevice *device, > if (!ubuf) > return -ENOMEM; > > - pglimit = (size_limit_mb * 1024 * 1024) >> PAGE_SHIFT; > + pglimit = ((u64)size_limit_mb * 1024 * 1024) >> PAGE_SHIFT; > for (i = 0; i < head->count; i++) { > pgoff_t subpgcnt; >

3 months, 1 week

Re: [PATCH v2 7/7] accel/rocket: Add IOCTLs for synchronizing memory accesses

by Jeffrey Hugo

On 2/25/2025 12:55 AM, Tomeu Vizoso wrote: > +int rocket_ioctl_fini_bo(struct drm_device *dev, void *data, struct drm_file *file) > +{ > + struct drm_rocket_fini_bo *args = data; > + struct drm_gem_object *gem_obj; > + struct rocket_gem_object *rkt_obj; > + struct drm_gem_shmem_object *shmem_obj; > + struct rocket_device *rdev = to_rocket_device(dev); > + > + gem_obj = drm_gem_object_lookup(file, args->handle); > + if (!gem_obj) > + return -ENOENT; > + > + rkt_obj = to_rocket_bo(gem_obj); > + shmem_obj = &rkt_obj->base; > + > + WARN_ON(rkt_obj->last_cpu_prep_op == 0); > + > + for (unsigned int core = 1; core < rdev->num_cores; core++) { > + dma_sync_sgtable_for_device(rdev->cores[core].dev, shmem_obj->sgt, > + rocket_op_to_dma_dir(rkt_obj->last_cpu_prep_op)); > + } > + > + rkt_obj->last_cpu_prep_op = 0; > + > + drm_gem_object_put(gem_obj); > + > + return 0; > +} flags must be 0, and you must check that here. You do not appear to be doing that. Otherwise, userspace may put a value in flags, which is ignored now, but later when you define flags for a purpose, existing userspace will be broken - a uapi violation.

3 months, 1 week

Re: [PATCH v2 5/7] accel/rocket: Add IOCTL for BO creation

by Jeffrey Hugo

On 2/25/2025 12:55 AM, Tomeu Vizoso wrote: > +/** > + * rocket_gem_create_object - Implementation of driver->gem_create_object. > + * @dev: DRM device > + * @size: Size in bytes of the memory the object will reference > + * > + * This lets the GEM helpers allocate object structs for us, and keep > + * our BO stats correct. > + */ I would expect that this would throw a warning when making the kernel documentation since you are not describing the return value.

3 months, 1 week

Re: [PATCH v6 03/10] optee: account for direction while converting parameters

by Jens Wiklander

Hi Sumit, On Thu, Mar 20, 2025 at 10:25 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > Hi Jens, > > On Mon, Mar 17, 2025 at 08:42:01AM +0100, Jens Wiklander wrote: > > Hi Sumit, > > > > On Thu, Mar 13, 2025 at 11:41 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > > > > > Hi Jens, > > > > > > On Wed, Mar 05, 2025 at 02:04:09PM +0100, Jens Wiklander wrote: > > > > The OP-TEE backend driver has two internal function pointers to convert > > > > between the subsystem type struct tee_param and the OP-TEE type struct > > > > optee_msg_param. > > > > > > > > The conversion is done from one of the types to the other, which is then > > > > involved in some operation and finally converted back to the original > > > > type. When converting to prepare the parameters for the operation, all > > > > fields must be taken into account, but then converting back, it's enough > > > > to update only out-values and out-sizes. So, an update_out parameter is > > > > added to the conversion functions to tell if all or only some fields > > > > must be copied. > > > > > > > > This is needed in a later patch where it might get confusing when > > > > converting back in from_msg_param() callback since an allocated > > > > restricted SHM can be using the sec_world_id of the used restricted > > > > memory pool and that doesn't translate back well. > > > > > > > > Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> > > > > --- > > > > drivers/tee/optee/call.c | 10 ++-- > > > > drivers/tee/optee/ffa_abi.c | 43 +++++++++++++---- > > > > drivers/tee/optee/optee_private.h | 42 +++++++++++------ > > > > drivers/tee/optee/rpc.c | 31 +++++++++---- > > > > drivers/tee/optee/smc_abi.c | 76 +++++++++++++++++++++++-------- > > > > 5 files changed, 144 insertions(+), 58 deletions(-) > > > > > > > > diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c > > > > index 16eb953e14bb..f1533b894726 100644 > > > > --- a/drivers/tee/optee/call.c > > > > +++ b/drivers/tee/optee/call.c > > > > @@ -400,7 +400,8 @@ int optee_open_session(struct tee_context *ctx, > > > > export_uuid(msg_arg->params[1].u.octets, &client_uuid); > > > > > > > > rc = optee->ops->to_msg_param(optee, msg_arg->params + 2, > > > > - arg->num_params, param); > > > > + arg->num_params, param, > > > > + false /*!update_out*/); > > > > if (rc) > > > > goto out; > > > > > > > > @@ -427,7 +428,8 @@ int optee_open_session(struct tee_context *ctx, > > > > } > > > > > > > > if (optee->ops->from_msg_param(optee, param, arg->num_params, > > > > - msg_arg->params + 2)) { > > > > + msg_arg->params + 2, > > > > + true /*update_out*/)) { > > > > arg->ret = TEEC_ERROR_COMMUNICATION; > > > > arg->ret_origin = TEEC_ORIGIN_COMMS; > > > > /* Close session again to avoid leakage */ > > > > @@ -541,7 +543,7 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg, > > > > msg_arg->cancel_id = arg->cancel_id; > > > > > > > > rc = optee->ops->to_msg_param(optee, msg_arg->params, arg->num_params, > > > > - param); > > > > + param, false /*!update_out*/); > > > > if (rc) > > > > goto out; > > > > > > > > @@ -551,7 +553,7 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg, > > > > } > > > > > > > > if (optee->ops->from_msg_param(optee, param, arg->num_params, > > > > - msg_arg->params)) { > > > > + msg_arg->params, true /*update_out*/)) { > > > > msg_arg->ret = TEEC_ERROR_COMMUNICATION; > > > > msg_arg->ret_origin = TEEC_ORIGIN_COMMS; > > > > } > > > > diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c > > > > index 4ca1d5161b82..e4b08cd195f3 100644 > > > > --- a/drivers/tee/optee/ffa_abi.c > > > > +++ b/drivers/tee/optee/ffa_abi.c > > > > @@ -122,15 +122,21 @@ static int optee_shm_rem_ffa_handle(struct optee *optee, u64 global_id) > > > > */ > > > > > > > > static void from_msg_param_ffa_mem(struct optee *optee, struct tee_param *p, > > > > - u32 attr, const struct optee_msg_param *mp) > > > > + u32 attr, const struct optee_msg_param *mp, > > > > + bool update_out) > > > > { > > > > struct tee_shm *shm = NULL; > > > > u64 offs_high = 0; > > > > u64 offs_low = 0; > > > > > > > > + if (update_out) { > > > > + if (attr == OPTEE_MSG_ATTR_TYPE_FMEM_INPUT) > > > > + return; > > > > + goto out; > > > > + } > > > > + > > > > p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT + > > > > attr - OPTEE_MSG_ATTR_TYPE_FMEM_INPUT; > > > > - p->u.memref.size = mp->u.fmem.size; > > > > > > > > if (mp->u.fmem.global_id != OPTEE_MSG_FMEM_INVALID_GLOBAL_ID) > > > > shm = optee_shm_from_ffa_handle(optee, mp->u.fmem.global_id); > > > > @@ -141,6 +147,8 @@ static void from_msg_param_ffa_mem(struct optee *optee, struct tee_param *p, > > > > offs_high = mp->u.fmem.offs_high; > > > > } > > > > p->u.memref.shm_offs = offs_low | offs_high << 32; > > > > +out: > > > > + p->u.memref.size = mp->u.fmem.size; > > > > } > > > > > > > > /** > > > > @@ -150,12 +158,14 @@ static void from_msg_param_ffa_mem(struct optee *optee, struct tee_param *p, > > > > * @params: subsystem internal parameter representation > > > > * @num_params: number of elements in the parameter arrays > > > > * @msg_params: OPTEE_MSG parameters > > > > + * @update_out: update parameter for output only > > > > * > > > > * Returns 0 on success or <0 on failure > > > > */ > > > > static int optee_ffa_from_msg_param(struct optee *optee, > > > > struct tee_param *params, size_t num_params, > > > > - const struct optee_msg_param *msg_params) > > > > + const struct optee_msg_param *msg_params, > > > > + bool update_out) > > > > { > > > > size_t n; > > > > > > > > @@ -166,18 +176,20 @@ static int optee_ffa_from_msg_param(struct optee *optee, > > > > > > > > switch (attr) { > > > > case OPTEE_MSG_ATTR_TYPE_NONE: > > > > + if (update_out) > > > > + break; > > > > p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE; > > > > memset(&p->u, 0, sizeof(p->u)); > > > > break; > > > > case OPTEE_MSG_ATTR_TYPE_VALUE_INPUT: > > > > case OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT: > > > > case OPTEE_MSG_ATTR_TYPE_VALUE_INOUT: > > > > - optee_from_msg_param_value(p, attr, mp); > > > > + optee_from_msg_param_value(p, attr, mp, update_out); > > > > break; > > > > case OPTEE_MSG_ATTR_TYPE_FMEM_INPUT: > > > > case OPTEE_MSG_ATTR_TYPE_FMEM_OUTPUT: > > > > case OPTEE_MSG_ATTR_TYPE_FMEM_INOUT: > > > > - from_msg_param_ffa_mem(optee, p, attr, mp); > > > > + from_msg_param_ffa_mem(optee, p, attr, mp, update_out); > > > > break; > > > > default: > > > > return -EINVAL; > > > > @@ -188,10 +200,16 @@ static int optee_ffa_from_msg_param(struct optee *optee, > > > > } > > > > > > > > static int to_msg_param_ffa_mem(struct optee_msg_param *mp, > > > > - const struct tee_param *p) > > > > + const struct tee_param *p, bool update_out) > > > > { > > > > struct tee_shm *shm = p->u.memref.shm; > > > > > > > > + if (update_out) { > > > > + if (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT) > > > > + return 0; > > > > + goto out; > > > > + } > > > > + > > > > mp->attr = OPTEE_MSG_ATTR_TYPE_FMEM_INPUT + p->attr - > > > > TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT; > > > > > > > > @@ -211,6 +229,7 @@ static int to_msg_param_ffa_mem(struct optee_msg_param *mp, > > > > memset(&mp->u, 0, sizeof(mp->u)); > > > > mp->u.fmem.global_id = OPTEE_MSG_FMEM_INVALID_GLOBAL_ID; > > > > } > > > > +out: > > > > mp->u.fmem.size = p->u.memref.size; > > > > > > > > return 0; > > > > @@ -222,13 +241,15 @@ static int to_msg_param_ffa_mem(struct optee_msg_param *mp, > > > > * @optee: main service struct > > > > * @msg_params: OPTEE_MSG parameters > > > > * @num_params: number of elements in the parameter arrays > > > > - * @params: subsystem itnernal parameter representation > > > > + * @params: subsystem internal parameter representation > > > > + * @update_out: update parameter for output only > > > > * Returns 0 on success or <0 on failure > > > > */ > > > > static int optee_ffa_to_msg_param(struct optee *optee, > > > > struct optee_msg_param *msg_params, > > > > size_t num_params, > > > > - const struct tee_param *params) > > > > + const struct tee_param *params, > > > > + bool update_out) > > > > { > > > > size_t n; > > > > > > > > @@ -238,18 +259,20 @@ static int optee_ffa_to_msg_param(struct optee *optee, > > > > > > > > switch (p->attr) { > > > > case TEE_IOCTL_PARAM_ATTR_TYPE_NONE: > > > > + if (update_out) > > > > + break; > > > > mp->attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE; > > > > memset(&mp->u, 0, sizeof(mp->u)); > > > > break; > > > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT: > > > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT: > > > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT: > > > > - optee_to_msg_param_value(mp, p); > > > > + optee_to_msg_param_value(mp, p, update_out); > > > > break; > > > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT: > > > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT: > > > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT: > > > > - if (to_msg_param_ffa_mem(mp, p)) > > > > + if (to_msg_param_ffa_mem(mp, p, update_out)) > > > > return -EINVAL; > > > > break; > > > > default: > > > > > > Can we rather handle it as follows to improve code readability and > > > maintainence long term? Ditto for all other places. > > > > > > static int optee_ffa_to_msg_param(struct optee *optee, > > > struct optee_msg_param *msg_params, > > > size_t num_params, > > > const struct tee_param *params, > > > bool update_out) > > > { > > > size_t n; > > > > > > for (n = 0; n < num_params; n++) { > > > const struct tee_param *p = params + n; > > > struct optee_msg_param *mp = msg_params + n; > > > > > > if (update_out && (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_NONE || > > > p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT || > > > p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT)) > > > continue; > > > > You're missing updating the length field for memrefs. > > > > Do we need to update length field for input memrefs when update_out is > set? I don't see that happening in your existing patch too. I'm sorry, I was unclear. The update_out parameter means only the output fields should be updated, not the attribute, offsets, ids, etc. That is, the length field for memrefs, and the value fields a, b, c for value params. Some of the memrefs aren't translated one-to-one with SDP, but the length field can and must be updated. Cheers, Jens > > -Sumit > > > Cheers, > > Jens > > > > > > > > switch (p->attr) { > > > case TEE_IOCTL_PARAM_ATTR_TYPE_NONE: > > > mp->attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE; > > > memset(&mp->u, 0, sizeof(mp->u)); > > > break; > > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT: > > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT: > > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT: > > > optee_to_msg_param_value(mp, p); > > > break; > > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT: > > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT: > > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT: > > > if (to_msg_param_ffa_mem(mp, p)) > > > return -EINVAL; > > > break; > > > default: > > > return -EINVAL; > > > } > > > } > > > > > > return 0; > > > } > > > > > > -Sumit

3 months, 1 week

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig