| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aAZFhlLv9GzHqUZc@sumit-X1>
Date: Mon, 21 Apr 2025 18:47:58 +0530
From: Sumit Garg <sumit.garg@...nel.org>
To: Amirreza Zarrabi <amirreza.zarrabi@....qualcomm.com>
Cc: Jens Wiklander <jens.wiklander@...aro.org>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konradybcio@...nel.org>,
Bartosz Golaszewski <bartosz.golaszewski@...aro.org>,
Apurupa Pattapu <quic_apurupa@...cinc.com>,
Kees Cook <kees@...nel.org>,
"Gustavo A. R. Silva" <gustavoars@...nel.org>,
Sumit Semwal <sumit.semwal@...aro.org>,
Christian König <christian.koenig@....com>,
linux-arm-msm@...r.kernel.org, op-tee@...ts.trustedfirmware.org,
linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org,
dri-devel@...ts.freedesktop.org, linaro-mm-sig@...ts.linaro.org,
linux-doc@...r.kernel.org
Subject: Re: [PATCH v3 03/11] tee: add TEE_IOCTL_PARAM_ATTR_TYPE_UBUF
On Wed, Apr 09, 2025 at 05:20:08PM +1000, Amirreza Zarrabi wrote:
>
>
> On 4/9/2025 4:41 PM, Jens Wiklander wrote:
> > Hi Amirreza,
> >
> > On Wed, Apr 9, 2025 at 2:28 AM Amirreza Zarrabi
> > <amirreza.zarrabi@....qualcomm.com> wrote:
> >>
> >> Hi jens,
> >>
> >> On 4/8/2025 10:19 PM, Jens Wiklander wrote:
> >>
> >> Hi Amirreza,
> >>
> >> On Fri, Mar 28, 2025 at 3:48 AM Amirreza Zarrabi
> >> <amirreza.zarrabi@....qualcomm.com> wrote:
> >>
> >> For drivers that can transfer data to the TEE without using shared
> >> memory from client, it is necessary to receive the user address
> >> directly, bypassing any processing by the TEE subsystem. Introduce
> >> TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT/OUTPUT/INOUT to represent
> >> userspace buffers.
> >>
> >> Signed-off-by: Amirreza Zarrabi <amirreza.zarrabi@....qualcomm.com>
> >> ---
> >> drivers/tee/tee_core.c | 33 +++++++++++++++++++++++++++++++++
> >> include/linux/tee_drv.h | 6 ++++++
> >> include/uapi/linux/tee.h | 22 ++++++++++++++++------
> >> 3 files changed, 55 insertions(+), 6 deletions(-)
> >>
> >> Is this patch needed now that the QCOMTEE driver supports shared
> >> memory? I prefer keeping changes to the ABI to a minimum.
> >>
> >> Cheers,
> >> Jens
> >>
> >> Unfortunately, this is still required. QTEE supports two types of data transfer:
> >> (1) using UBUF and (2) memory objects. Even with memory object support, some APIs still
> >> expect to receive data using UBUF. For instance, to load a TA, QTEE offers two interfaces:
> >> one where the TA binary is in UBUF and another where the TA binary is in a memory object.
> >
> > Is this a limitation in the QTEE backend driver or on the secure side?
> > Can it be fixed? I don't ask for changes in the ABI to the secure
> > world since I assume you haven't made such changes while this patch
> > set has evolved.
> >
> > Cheers,
> > Jens
>
> The secure-side ABI supports passing data using memcpy to the same
> buffer that contains the message for QTEE, rather than using a memory
> object. Some services tend to use this approach for small data instead
> of allocating a memory object. I have no choice but to expose this support.
Okay, I can see how QTEE supports embedding user buffers in fixed size
shared memory buffers allocated by the driver with maximum size limits.
OP-TEE also have support for temporary shared memory where the user
space client directly passes the buffer to share with OP-TEE. Then the
libteec [1] handles the underneath copy to and from the shared memory
allocation automatically.
So is there a limitation for QCOMTEE user space library [2] to do the
same? This way we will be able to retain the user-space ABI as well as
simplicify the kernel driver.
[1] https://github.com/OP-TEE/optee_client/blob/master/libteec/src/tee_client_api.c#L365
[2] https://github.com/quic/quic-teec
-Sumit
>
> Throughout the patchset, I have not made any change to the ABI but
> tried to provide support for the memory object in a separate,
> independent commit, distinct from the UBUF.
>
> Best regards,
> Amir
>
> >
> >>
> >> Best Regards,
> >> Amir
> >>
> >> diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> >> index 22cc7d624b0c..bc862a11d437 100644
> >> --- a/drivers/tee/tee_core.c
> >> +++ b/drivers/tee/tee_core.c
> >> @@ -404,6 +404,17 @@ static int params_from_user(struct tee_context *ctx, struct tee_param *params,
> >> params[n].u.value.b = ip.b;
> >> params[n].u.value.c = ip.c;
> >> break;
> >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT:
> >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT:
> >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT:
> >> + params[n].u.ubuf.uaddr = u64_to_user_ptr(ip.a);
> >> + params[n].u.ubuf.size = ip.b;
> >> +
> >> + if (!access_ok(params[n].u.ubuf.uaddr,
> >> + params[n].u.ubuf.size))
> >> + return -EFAULT;
> >> +
> >> + 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:
> >> @@ -472,6 +483,11 @@ static int params_to_user(struct tee_ioctl_param __user *uparams,
> >> put_user(p->u.value.c, &up->c))
> >> return -EFAULT;
> >> break;
> >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT:
> >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT:
> >> + if (put_user((u64)p->u.ubuf.size, &up->b))
> >> + return -EFAULT;
> >> + break;
> >> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT:
> >> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT:
> >> if (put_user((u64)p->u.memref.size, &up->b))
> >> @@ -672,6 +688,13 @@ static int params_to_supp(struct tee_context *ctx,
> >> ip.b = p->u.value.b;
> >> ip.c = p->u.value.c;
> >> break;
> >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT:
> >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT:
> >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT:
> >> + ip.a = (u64)p->u.ubuf.uaddr;
> >> + ip.b = p->u.ubuf.size;
> >> + ip.c = 0;
> >> + 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:
> >> @@ -774,6 +797,16 @@ static int params_from_supp(struct tee_param *params, size_t num_params,
> >> p->u.value.b = ip.b;
> >> p->u.value.c = ip.c;
> >> break;
> >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT:
> >> + case TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT:
> >> + p->u.ubuf.uaddr = u64_to_user_ptr(ip.a);
> >> + p->u.ubuf.size = ip.b;
> >> +
> >> + if (!access_ok(params[n].u.ubuf.uaddr,
> >> + params[n].u.ubuf.size))
> >> + return -EFAULT;
> >> +
> >> + break;
> >> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT:
> >> case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT:
> >> /*
> >> diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> >> index ce23fd42c5d4..d773f91c6bdd 100644
> >> --- a/include/linux/tee_drv.h
> >> +++ b/include/linux/tee_drv.h
> >> @@ -82,6 +82,11 @@ struct tee_param_memref {
> >> struct tee_shm *shm;
> >> };
> >>
> >> +struct tee_param_ubuf {
> >> + void * __user uaddr;
> >> + size_t size;
> >> +};
> >> +
> >> struct tee_param_value {
> >> u64 a;
> >> u64 b;
> >> @@ -92,6 +97,7 @@ struct tee_param {
> >> u64 attr;
> >> union {
> >> struct tee_param_memref memref;
> >> + struct tee_param_ubuf ubuf;
> >> struct tee_param_value value;
> >> } u;
> >> };
> >> diff --git a/include/uapi/linux/tee.h b/include/uapi/linux/tee.h
> >> index d0430bee8292..3e9b1ec5dfde 100644
> >> --- a/include/uapi/linux/tee.h
> >> +++ b/include/uapi/linux/tee.h
> >> @@ -151,6 +151,13 @@ struct tee_ioctl_buf_data {
> >> #define TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT 6
> >> #define TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT 7 /* input and output */
> >>
> >> +/*
> >> + * These defines userspace buffer parameters.
> >> + */
> >> +#define TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INPUT 8
> >> +#define TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_OUTPUT 9
> >> +#define TEE_IOCTL_PARAM_ATTR_TYPE_UBUF_INOUT 10 /* input and output */
> >> +
> >> /*
> >> * Mask for the type part of the attribute, leaves room for more types
> >> */
> >> @@ -186,14 +193,17 @@ struct tee_ioctl_buf_data {
> >> /**
> >> * struct tee_ioctl_param - parameter
> >> * @attr: attributes
> >> - * @a: if a memref, offset into the shared memory object, else a value parameter
> >> - * @b: if a memref, size of the buffer, else a value parameter
> >> + * @a: if a memref, offset into the shared memory object,
> >> + * else if a ubuf, address of the user buffer,
> >> + * else a value parameter
> >> + * @b: if a memref or ubuf, size of the buffer, else a value parameter
> >> * @c: if a memref, shared memory identifier, else a value parameter
> >> *
> >> - * @attr & TEE_PARAM_ATTR_TYPE_MASK indicates if memref or value is used in
> >> - * the union. TEE_PARAM_ATTR_TYPE_VALUE_* indicates value and
> >> - * TEE_PARAM_ATTR_TYPE_MEMREF_* indicates memref. TEE_PARAM_ATTR_TYPE_NONE
> >> - * indicates that none of the members are used.
> >> + * @attr & TEE_PARAM_ATTR_TYPE_MASK indicates if memref, ubuf, or value is
> >> + * used in the union. TEE_PARAM_ATTR_TYPE_VALUE_* indicates value,
> >> + * TEE_PARAM_ATTR_TYPE_MEMREF_* indicates memref, and TEE_PARAM_ATTR_TYPE_UBUF_*
> >> + * indicates ubuf. TEE_PARAM_ATTR_TYPE_NONE indicates that none of the members
> >> + * are used.
> >> *
> >> * Shared memory is allocated with TEE_IOC_SHM_ALLOC which returns an
> >> * identifier representing the shared memory object. A memref can reference
> >>
> >> --
> >> 2.34.1
> >>
>
>
Powered by blists - more mailing lists