Re: [EXT] Re: [PATCH 1/1] tee: new ioctl to a register tee_shm from a dmabuf file descriptor
Hi Sumit,
Thanks for your comments, please find my answer below.
On ven., 2022-08-12 at 19:36 +0530, Sumit Garg wrote:
> Caution: EXT Email
>
> Hi Olivier,
>
> On Thu, 11 Aug 2022 at 19:26, Olivier Masse
> wrote:
> >
> > From: Etienne Carriere
> >
> > This change allows userland to create a tee_shm object that refers
> > to a dmabuf reference.
> >
> > Userland provides a dmabuf file descriptor as buffer reference.
> > The created tee_shm object exported as a brand new dmabuf reference
> > used to provide a clean fd to userland. Userland shall closed this
> > new
> > fd to release the tee_shm object resources. The initial dmabuf
> > resources
> > are tracked independently through original dmabuf file descriptor.
> >
> > Once the buffer is registered and until it is released, TEE driver
> > keeps a refcount on the registered dmabuf structure.
> >
> > This change only support dmabuf references that relates to
> > physically
> > contiguous memory buffers.
>
> What limits you to extend this feature to non-contiguous memory
> buffers? I believe that should be possible with OP-TEE dynamic shared
> memory which gives you the granularity to register a list of pages.
But it will need more logic in OPTEE OS to verify that all pages are in
the Secure Data Path protected area.
Our solution use a fixed protected reserved memory region and do not
rely on a dynamic protection set in secure.
Best regards,
Olivier
>
> -Sumit
>
> >
> > New tee_shm flag to identify tee_shm objects built from a
> > registered
> > dmabuf: TEE_SHM_EXT_DMA_BUF. Such tee_shm structures are flagged
> > with
> > TEE_SHM_EXT_DMA_BUF.
> >
> > Co-Developed-by: Etienne Carriere
> > Signed-off-by: Olivier Masse
> > From:
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Flinaro-swg%2Flinux.gitdata=05%7C01%7Colivier.masse%40nxp.com%7C204e0821d1c84355ed4208da7c6be5c6%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637959100100176447%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7Csdata=MRfnQNbwAIj%2Bnd9tSjSb5Nzrv3sQVpVMIdOxxRfu6U4%3Dreserved=0
> > (cherry picked from commit
> > 41e21e5c405530590dc2dd10b2a8dbe64589840f)
> > ---
> > 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(-)
> >
> > diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> > index 8aa1a4836b92..7c45cbf85eb9 100644
> > --- a/drivers/tee/tee_core.c
> > +++ b/drivers/tee/tee_core.c
> > @@ -355,6 +355,42 @@ 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(, 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, , 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 params_from_user(struct tee_context *ctx, struct
> > tee_param *params,
> > size_t num_params,
> > struct tee_ioctl_param __user *uparams)
> > @@ -829,6 +865,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:
> > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> > index 836872467dc6..55a3fbbb022e 100644
> > --- a/drivers/tee/tee_shm.c
> > +++ b/drivers/tee/tee_shm.c
> > @@ -4,6 +4,7 @@
> > */
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
>
Re: [EXT] Re: [PATCH 1/1] tee: new ioctl to a register tee_shm from a dmabuf file descriptor
Hi Christian,
On ven., 2022-08-12 at 12:18 +0200, Christian König wrote:
> Caution: EXT Email
>
> Hi Etienne,
>
> at least I don't see anything obvious broken.
>
> Just two comments:
>
> 1. Dmitry is working on a change which renames some functions and
> makes
> it mandatory to call them with the dma_resv lock held.
>
> Depending on how you want to upstream this change you will certainly
> run
> into conflicts with that.
ok could you send me some link to his changes ?
>
> 2. Would it be possible to do this dynamically? In other words does
> the
> tee driver has a concept of buffers moving around?
I'm not sure to understand what you mean by a moving buffer ?
For information the TEE need to map the buffer in secure which is done
when calling the invoke operation function.
>
> Am 11.08.22 um 15:56 schrieb Olivier Masse:
> > From: Etienne Carriere
> >
> > This change allows userland to create a tee_shm object that refers
> > to a dmabuf reference.
> >
> > Userland provides a dmabuf file descriptor as buffer reference.
> > The created tee_shm object exported as a brand new dmabuf reference
> > used to provide a clean fd to userland. Userland shall closed this
> > new
> > fd to release the tee_shm object resources. The initial dmabuf
> > resources
> > are tracked independently through original dmabuf file descriptor.
> >
> > Once the buffer is registered and until it is released, TEE driver
> > keeps a refcount on the registered dmabuf structure.
> >
> > This change only support dmabuf references that relates to
> > physically
> > contiguous memory buffers.
>
> That sounds like a pretty hard restriction, but I obviously don't see
> how to avoid it either.
>
> Regards,
> Christian.
>
> >
> > New tee_shm flag to identify tee_shm objects built from a
> > registered
> > dmabuf: TEE_SHM_EXT_DMA_BUF. Such tee_shm structures are flagged
> > with
> > TEE_SHM_EXT_DMA_BUF.
> >
> > Co-Developed-by: Etienne Carriere
> > Signed-off-by: Olivier Masse
> > From:
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Flinaro-swg%2Flinux.gitdata=05%7C01%7Colivier.masse%40nxp.com%7C03c2d3cc95b8429693c108da7c4bf7a7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637958962989857231%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7Csdata=vz8Y7ZwKXteePpWQVO8EhmZ1oyIX0xOCRd%2BGH7xHjII%3Dreserved=0
> > (cherry picked from commit
> > 41e21e5c405530590dc2dd10b2a8dbe64589840f)
> > ---
> > 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(-)
> >
> > diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> > index 8aa1a4836b92..7c45cbf85eb9 100644
> > --- a/drivers/tee/tee_core.c
> > +++ b/drivers/tee/tee_core.c
> > @@ -355,6 +355,42 @@ 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(, 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, , 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 params_from_user(struct tee_context *ctx, struct
> > tee_param *params,
> > size_t num_params,
> > struct tee_ioctl_param __user *uparams)
> > @@ -829,6 +865,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:
> > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> > index
