* Re: [PATCH v6 03/10] optee: account for direction while converting parameters [not found] < <CAHUa44H5Zc_POJ_RWaVd4iFVagRkFaN+8Ajs=19FKboZapbQHw@mail.gmail.com> @ 2025-03-20 9:25 ` Sumit Garg 2025-03-20 13:00 ` Jens Wiklander 0 siblings, 1 reply; 9+ messages in thread From: Sumit Garg @ 2025-03-20 9:25 UTC (permalink / raw) To: op-tee [-- Attachment #1: Type: text/plain, Size: 13084 bytes --] 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@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@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. -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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v6 03/10] optee: account for direction while converting parameters 2025-03-20 9:25 ` [PATCH v6 03/10] optee: account for direction while converting parameters Sumit Garg @ 2025-03-20 13:00 ` Jens Wiklander 0 siblings, 0 replies; 9+ messages in thread From: Jens Wiklander @ 2025-03-20 13:00 UTC (permalink / raw) To: op-tee [-- Attachment #1: Type: text/plain, Size: 14083 bytes --] Hi Sumit, On Thu, Mar 20, 2025 at 10:25 AM Sumit Garg <sumit.garg@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@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@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 ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: < <CAHUa44HXHwR1_LbjG6T5OqnafXiS4-kHeKjV9TurGORx3SsprQ@mail.gmail.com>]
* Re: [PATCH v6 03/10] optee: account for direction while converting parameters [not found] < <CAHUa44HXHwR1_LbjG6T5OqnafXiS4-kHeKjV9TurGORx3SsprQ@mail.gmail.com> @ 2025-04-01 7:45 ` Sumit Garg 2025-04-01 8:21 ` Jens Wiklander 0 siblings, 1 reply; 9+ messages in thread From: Sumit Garg @ 2025-04-01 7:45 UTC (permalink / raw) To: op-tee [-- Attachment #1: Type: text/plain, Size: 15577 bytes --] On Tue, Mar 25, 2025 at 09:50:35AM +0100, Jens Wiklander wrote: > On Tue, Mar 25, 2025 at 6:56 AM Sumit Garg <sumit.garg@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@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@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@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. params_to_user() doesn't take any additional parameter like "update_out" which is complicating the program flow here. Can't we rather follow similar practice for {to/from}_msg_param() APIs? -Sumit ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v6 03/10] optee: account for direction while converting parameters 2025-04-01 7:45 ` Sumit Garg @ 2025-04-01 8:21 ` Jens Wiklander 0 siblings, 0 replies; 9+ messages in thread From: Jens Wiklander @ 2025-04-01 8:21 UTC (permalink / raw) To: op-tee [-- Attachment #1: Type: text/plain, Size: 16424 bytes --] On Tue, Apr 1, 2025 at 9:45 AM Sumit Garg <sumit.garg@kernel.org> wrote: > > On Tue, Mar 25, 2025 at 09:50:35AM +0100, Jens Wiklander wrote: > > On Tue, Mar 25, 2025 at 6:56 AM Sumit Garg <sumit.garg@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@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@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. > > params_to_user() doesn't take any additional parameter like "update_out" > which is complicating the program flow here. Can't we rather follow > similar practice for {to/from}_msg_param() APIs? I'm afraid something special is needed. handle_rpc_supp_cmd() needs all the fields to be updated, from_msg_param() prepares input and attribute type may have changed when to_msg_param() is called. Cheers, Jens ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: < <CAHUa44HY-jZCoDXKnx-f4gBoABRVdh1+6PA1AYHSva9aL=rDAA@mail.gmail.com>]
* Re: [PATCH v6 03/10] optee: account for direction while converting parameters [not found] < <CAHUa44HY-jZCoDXKnx-f4gBoABRVdh1+6PA1AYHSva9aL=rDAA@mail.gmail.com> @ 2025-03-25 5:55 ` Sumit Garg 2025-03-25 8:50 ` Jens Wiklander 0 siblings, 1 reply; 9+ messages in thread From: Sumit Garg @ 2025-03-25 5:55 UTC (permalink / raw) To: op-tee [-- Attachment #1: Type: text/plain, Size: 14836 bytes --] 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@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@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@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? -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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v6 03/10] optee: account for direction while converting parameters 2025-03-25 5:55 ` Sumit Garg @ 2025-03-25 8:50 ` Jens Wiklander 0 siblings, 0 replies; 9+ messages in thread From: Jens Wiklander @ 2025-03-25 8:50 UTC (permalink / raw) To: op-tee [-- Attachment #1: Type: text/plain, Size: 16132 bytes --] On Tue, Mar 25, 2025 at 6:56 AM Sumit Garg <sumit.garg@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@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@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@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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v6 00/10] TEE subsystem for restricted dma-buf allocations
@ 2025-03-05 13:04 Jens Wiklander
2025-03-05 13:04 ` [PATCH v6 03/10] optee: account for direction while converting parameters Jens Wiklander
0 siblings, 1 reply; 9+ messages in thread
From: Jens Wiklander @ 2025-03-05 13:04 UTC (permalink / raw)
To: op-tee
[-- Attachment #1: Type: text/plain, Size: 7355 bytes --]
Hi,
This patch set allocates the restricted DMA-bufs from a DMA-heap
instantiated from the TEE subsystem.
The TEE subsystem handles the DMA-buf allocations since it is the TEE
(OP-TEE, AMD-TEE, TS-TEE, or perhaps a future QTEE) which sets up the
restrictions for the memory used for the DMA-bufs.
The DMA-heap uses a restricted memory pool provided by the backend TEE
driver, allowing it to choose how to allocate the restricted physical
memory.
The allocated DMA-bufs must be imported with a new TEE_IOC_SHM_REGISTER_FD
before they can be passed as arguments when requesting services from the
secure world.
Three use-cases (Secure Video Playback, Trusted UI, and Secure Video
Recording) has been identified so far to serve as examples of what can be
expected. The use-cases has predefined DMA-heap names,
"restricted,secure-video", "restricted,trusted-ui", and
"restricted,secure-video-record". The backend driver registers restricted
memory pools for the use-cases it supports.
Each use-case has it's own restricted memory pool since different use-cases
requires isolation from different parts of the system. A restricted memory
pool can be based on a static carveout instantiated while probing the TEE
backend driver, or dynamically allocated from CMA and made restricted as
needed by the TEE.
This can be tested on a RockPi 4B+ with the following steps:
repo init -u https://github.com/jenswi-linaro/manifest.git -m rockpi4.xml \
-b prototype/sdp-v6
repo sync -j8
cd build
make toolchains -j$(nproc)
make all -j$(nproc)
# Copy ../out/rockpi4.img to an SD card and boot the RockPi from that
# Connect a monitor to the RockPi
# login and at the prompt:
gst-launch-1.0 videotestsrc ! \
aesenc key=1f9423681beb9a79215820f6bda73d0f \
iv=e9aa8e834d8d70b7e0d254ff670dd718 serialize-iv=true ! \
aesdec key=1f9423681beb9a79215820f6bda73d0f ! \
kmssink
The aesdec module has been hacked to use an OP-TEE TA to decrypt the stream
into restricted DMA-bufs which are consumed by the kmssink.
The primitive QEMU tests from previous patch set can be tested on RockPi
in the same way with:
xtest --sdp-basic
The primitive test are tested on QEMU with the following steps:
repo init -u https://github.com/jenswi-linaro/manifest.git -m qemu_v8.xml \
-b prototype/sdp-v6
repo sync -j8
cd build
make toolchains -j$(nproc)
make SPMC_AT_EL=1 all -j$(nproc)
make SPMC_AT_EL=1 run-only
# login and at the prompt:
xtest --sdp-basic
The SPMC_AT_EL=1 parameter configures the build with FF-A and an SPMC at
S-EL1 inside OP-TEE. The parameter can be changed into SPMC_AT_EL=n to test
without FF-A using the original SMC ABI instead. Please remember to do
%rm -rf ../trusted-firmware-a/build/qemu
for TF-A to be rebuilt properly using the new configuration.
https://optee.readthedocs.io/en/latest/building/prerequisites.html
list dependencies needed to build the above.
The tests are pretty basic, mostly checking that a Trusted Application in
the secure world can access and manipulate the memory. There are also some
negative tests for out of bounds buffers etc.
Thanks,
Jens
Changes since V5:
* Removing "tee: add restricted memory allocation" and
"tee: add TEE_IOC_RSTMEM_FD_INFO"
* Adding "tee: implement restricted DMA-heap",
"tee: new ioctl to a register tee_shm from a dmabuf file descriptor",
"tee: add tee_shm_alloc_cma_phys_mem()",
"optee: pass parent device to tee_device_alloc()", and
"tee: tee_device_alloc(): copy dma_mask from parent device"
* The two TEE driver OPs "rstmem_alloc()" and "rstmem_free()" are replaced
with a struct tee_rstmem_pool abstraction.
* Replaced the the TEE_IOC_RSTMEM_ALLOC user space API with the DMA-heap API
Changes since V4:
* Adding the patch "tee: add TEE_IOC_RSTMEM_FD_INFO" needed by the
GStreamer demo
* Removing the dummy CPU access and mmap functions from the dma_buf_ops
* Fixing a compile error in "optee: FF-A: dynamic restricted memory allocation"
reported by kernel test robot <lkp@intel.com>
Changes since V3:
* Make the use_case and flags field in struct tee_shm u32's instead of
u16's
* Add more description for TEE_IOC_RSTMEM_ALLOC in the header file
* Import namespace DMA_BUF in module tee, reported by lkp(a)intel.com
* Added a note in the commit message for "optee: account for direction
while converting parameters" why it's needed
* Factor out dynamic restricted memory allocation from
"optee: support restricted memory allocation" into two new commits
"optee: FF-A: dynamic restricted memory allocation" and
"optee: smc abi: dynamic restricted memory allocation"
* Guard CMA usage with #ifdef CONFIG_CMA, effectively disabling dynamic
restricted memory allocate if CMA isn't configured
Changes since the V2 RFC:
* Based on v6.12
* Replaced the flags for SVP and Trusted UID memory with a u32 field with
unique id for each use case
* Added dynamic allocation of restricted memory pools
* Added OP-TEE ABI both with and without FF-A for dynamic restricted memory
* Added support for FF-A with FFA_LEND
Changes since the V1 RFC:
* Based on v6.11
* Complete rewrite, replacing the restricted heap with TEE_IOC_RSTMEM_ALLOC
Changes since Olivier's post [2]:
* Based on Yong Wu's post [1] where much of dma-buf handling is done in
the generic restricted heap
* Simplifications and cleanup
* New commit message for "dma-buf: heaps: add Linaro restricted dmabuf heap
support"
* Replaced the word "secure" with "restricted" where applicable
Etienne Carriere (1):
tee: new ioctl to a register tee_shm from a dmabuf file descriptor
Jens Wiklander (9):
tee: tee_device_alloc(): copy dma_mask from parent device
optee: pass parent device to tee_device_alloc()
optee: account for direction while converting parameters
optee: sync secure world ABI headers
tee: implement restricted DMA-heap
tee: add tee_shm_alloc_cma_phys_mem()
optee: support restricted memory allocation
optee: FF-A: dynamic restricted memory allocation
optee: smc abi: dynamic restricted memory allocation
drivers/tee/Makefile | 1 +
drivers/tee/optee/Makefile | 1 +
drivers/tee/optee/call.c | 10 +-
drivers/tee/optee/core.c | 1 +
drivers/tee/optee/ffa_abi.c | 194 +++++++++++-
drivers/tee/optee/optee_ffa.h | 27 +-
drivers/tee/optee/optee_msg.h | 65 ++++-
drivers/tee/optee/optee_private.h | 55 +++-
drivers/tee/optee/optee_smc.h | 71 ++++-
drivers/tee/optee/rpc.c | 31 +-
drivers/tee/optee/rstmem.c | 329 +++++++++++++++++++++
drivers/tee/optee/smc_abi.c | 190 ++++++++++--
drivers/tee/tee_core.c | 147 +++++++---
drivers/tee/tee_heap.c | 470 ++++++++++++++++++++++++++++++
drivers/tee/tee_private.h | 7 +
drivers/tee/tee_shm.c | 199 ++++++++++++-
include/linux/tee_core.h | 67 +++++
include/linux/tee_drv.h | 10 +
include/uapi/linux/tee.h | 29 ++
19 files changed, 1781 insertions(+), 123 deletions(-)
create mode 100644 drivers/tee/optee/rstmem.c
create mode 100644 drivers/tee/tee_heap.c
base-commit: 7eb172143d5508b4da468ed59ee857c6e5e01da6
--
2.43.0
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v6 03/10] optee: account for direction while converting parameters 2025-03-05 13:04 [PATCH v6 00/10] TEE subsystem for restricted dma-buf allocations Jens Wiklander @ 2025-03-05 13:04 ` Jens Wiklander 2025-03-13 10:41 ` Sumit Garg 0 siblings, 1 reply; 9+ messages in thread From: Jens Wiklander @ 2025-03-05 13:04 UTC (permalink / raw) To: op-tee [-- Attachment #1: Type: text/plain, Size: 21342 bytes --] 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@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: diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h index dc0f355ef72a..20eda508dbac 100644 --- a/drivers/tee/optee/optee_private.h +++ b/drivers/tee/optee/optee_private.h @@ -185,10 +185,12 @@ struct optee_ops { bool system_thread); int (*to_msg_param)(struct optee *optee, struct optee_msg_param *msg_params, - size_t num_params, const struct tee_param *params); + size_t num_params, const struct tee_param *params, + bool update_out); int (*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); }; /** @@ -316,23 +318,35 @@ void optee_release(struct tee_context *ctx); void optee_release_supp(struct tee_context *ctx); static inline void optee_from_msg_param_value(struct tee_param *p, u32 attr, - const struct optee_msg_param *mp) + const struct optee_msg_param *mp, + bool update_out) { - p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT + - attr - OPTEE_MSG_ATTR_TYPE_VALUE_INPUT; - p->u.value.a = mp->u.value.a; - p->u.value.b = mp->u.value.b; - p->u.value.c = mp->u.value.c; + if (!update_out) + p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT + + attr - OPTEE_MSG_ATTR_TYPE_VALUE_INPUT; + + if (attr == OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT || + attr == OPTEE_MSG_ATTR_TYPE_VALUE_INOUT || !update_out) { + p->u.value.a = mp->u.value.a; + p->u.value.b = mp->u.value.b; + p->u.value.c = mp->u.value.c; + } } static inline void optee_to_msg_param_value(struct optee_msg_param *mp, - const struct tee_param *p) + const struct tee_param *p, + bool update_out) { - mp->attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT + p->attr - - TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT; - mp->u.value.a = p->u.value.a; - mp->u.value.b = p->u.value.b; - mp->u.value.c = p->u.value.c; + if (!update_out) + mp->attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT + p->attr - + TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT; + + if (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT || + p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT || !update_out) { + mp->u.value.a = p->u.value.a; + mp->u.value.b = p->u.value.b; + mp->u.value.c = p->u.value.c; + } } void optee_cq_init(struct optee_call_queue *cq, int thread_count); diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c index ebbbd42b0e3e..580e6b9b0606 100644 --- a/drivers/tee/optee/rpc.c +++ b/drivers/tee/optee/rpc.c @@ -63,7 +63,7 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx, } if (optee->ops->from_msg_param(optee, params, arg->num_params, - arg->params)) + arg->params, false /*!update_out*/)) goto bad; for (i = 0; i < arg->num_params; i++) { @@ -107,7 +107,8 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx, } else { params[3].u.value.a = msg.len; if (optee->ops->to_msg_param(optee, arg->params, - arg->num_params, params)) + arg->num_params, params, + true /*update_out*/)) arg->ret = TEEC_ERROR_BAD_PARAMETERS; else arg->ret = TEEC_SUCCESS; @@ -188,6 +189,7 @@ static void handle_rpc_func_cmd_wait(struct optee_msg_arg *arg) static void handle_rpc_supp_cmd(struct tee_context *ctx, struct optee *optee, struct optee_msg_arg *arg) { + bool update_out = false; struct tee_param *params; arg->ret_origin = TEEC_ORIGIN_COMMS; @@ -200,15 +202,21 @@ static void handle_rpc_supp_cmd(struct tee_context *ctx, struct optee *optee, } if (optee->ops->from_msg_param(optee, params, arg->num_params, - arg->params)) { + arg->params, update_out)) { arg->ret = TEEC_ERROR_BAD_PARAMETERS; goto out; } arg->ret = optee_supp_thrd_req(ctx, arg->cmd, arg->num_params, params); + /* + * Special treatment for OPTEE_RPC_CMD_SHM_ALLOC since input is a + * value type, but the output is a memref type. + */ + if (arg->cmd != OPTEE_RPC_CMD_SHM_ALLOC) + update_out = true; if (optee->ops->to_msg_param(optee, arg->params, arg->num_params, - params)) + params, update_out)) arg->ret = TEEC_ERROR_BAD_PARAMETERS; out: kfree(params); @@ -270,7 +278,7 @@ static void handle_rpc_func_rpmb_probe_reset(struct tee_context *ctx, if (arg->num_params != ARRAY_SIZE(params) || optee->ops->from_msg_param(optee, params, arg->num_params, - arg->params) || + arg->params, false /*!update_out*/) || params[0].attr != TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT) { arg->ret = TEEC_ERROR_BAD_PARAMETERS; return; @@ -280,7 +288,8 @@ static void handle_rpc_func_rpmb_probe_reset(struct tee_context *ctx, params[0].u.value.b = 0; params[0].u.value.c = 0; if (optee->ops->to_msg_param(optee, arg->params, - arg->num_params, params)) { + arg->num_params, params, + true /*update_out*/)) { arg->ret = TEEC_ERROR_BAD_PARAMETERS; return; } @@ -324,7 +333,7 @@ static void handle_rpc_func_rpmb_probe_next(struct tee_context *ctx, if (arg->num_params != ARRAY_SIZE(params) || optee->ops->from_msg_param(optee, params, arg->num_params, - arg->params) || + arg->params, false /*!update_out*/) || params[0].attr != TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT || params[1].attr != TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT) { arg->ret = TEEC_ERROR_BAD_PARAMETERS; @@ -358,7 +367,8 @@ static void handle_rpc_func_rpmb_probe_next(struct tee_context *ctx, params[0].u.value.b = rdev->descr.capacity; params[0].u.value.c = rdev->descr.reliable_wr_count; if (optee->ops->to_msg_param(optee, arg->params, - arg->num_params, params)) { + arg->num_params, params, + true /*update_out*/)) { arg->ret = TEEC_ERROR_BAD_PARAMETERS; return; } @@ -384,7 +394,7 @@ static void handle_rpc_func_rpmb_frames(struct tee_context *ctx, if (arg->num_params != ARRAY_SIZE(params) || optee->ops->from_msg_param(optee, params, arg->num_params, - arg->params) || + arg->params, false /*!update_out*/) || params[0].attr != TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT || params[1].attr != TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT) { arg->ret = TEEC_ERROR_BAD_PARAMETERS; @@ -401,7 +411,8 @@ static void handle_rpc_func_rpmb_frames(struct tee_context *ctx, goto out; } if (optee->ops->to_msg_param(optee, arg->params, - arg->num_params, params)) { + arg->num_params, params, + true /*update_out*/)) { arg->ret = TEEC_ERROR_BAD_PARAMETERS; goto out; } diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c index 165fadd9abc9..cfdae266548b 100644 --- a/drivers/tee/optee/smc_abi.c +++ b/drivers/tee/optee/smc_abi.c @@ -81,20 +81,26 @@ static int optee_cpuhp_disable_pcpu_irq(unsigned int cpu) */ static int from_msg_param_tmp_mem(struct tee_param *p, u32 attr, - const struct optee_msg_param *mp) + const struct optee_msg_param *mp, + bool update_out) { struct tee_shm *shm; phys_addr_t pa; int rc; + if (update_out) { + if (attr == OPTEE_MSG_ATTR_TYPE_TMEM_INPUT) + return 0; + goto out; + } + p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT + attr - OPTEE_MSG_ATTR_TYPE_TMEM_INPUT; - p->u.memref.size = mp->u.tmem.size; shm = (struct tee_shm *)(unsigned long)mp->u.tmem.shm_ref; if (!shm) { p->u.memref.shm_offs = 0; p->u.memref.shm = NULL; - return 0; + goto out; } rc = tee_shm_get_pa(shm, 0, &pa); @@ -103,18 +109,25 @@ static int from_msg_param_tmp_mem(struct tee_param *p, u32 attr, p->u.memref.shm_offs = mp->u.tmem.buf_ptr - pa; p->u.memref.shm = shm; - +out: + p->u.memref.size = mp->u.tmem.size; return 0; } static void from_msg_param_reg_mem(struct tee_param *p, u32 attr, - const struct optee_msg_param *mp) + const struct optee_msg_param *mp, + bool update_out) { struct tee_shm *shm; + if (update_out) { + if (attr == OPTEE_MSG_ATTR_TYPE_RMEM_INPUT) + return; + goto out; + } + p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT + attr - OPTEE_MSG_ATTR_TYPE_RMEM_INPUT; - p->u.memref.size = mp->u.rmem.size; shm = (struct tee_shm *)(unsigned long)mp->u.rmem.shm_ref; if (shm) { @@ -124,6 +137,8 @@ static void from_msg_param_reg_mem(struct tee_param *p, u32 attr, p->u.memref.shm_offs = 0; p->u.memref.shm = NULL; } +out: + p->u.memref.size = mp->u.rmem.size; } /** @@ -133,11 +148,13 @@ static void from_msg_param_reg_mem(struct tee_param *p, u32 attr, * @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_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) { int rc; size_t n; @@ -149,25 +166,27 @@ static int optee_from_msg_param(struct optee *optee, struct tee_param *params, 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_TMEM_INPUT: case OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT: case OPTEE_MSG_ATTR_TYPE_TMEM_INOUT: - rc = from_msg_param_tmp_mem(p, attr, mp); + rc = from_msg_param_tmp_mem(p, attr, mp, update_out); if (rc) return rc; break; case OPTEE_MSG_ATTR_TYPE_RMEM_INPUT: case OPTEE_MSG_ATTR_TYPE_RMEM_OUTPUT: case OPTEE_MSG_ATTR_TYPE_RMEM_INOUT: - from_msg_param_reg_mem(p, attr, mp); + from_msg_param_reg_mem(p, attr, mp, update_out); break; default: @@ -178,20 +197,25 @@ static int optee_from_msg_param(struct optee *optee, struct tee_param *params, } static int to_msg_param_tmp_mem(struct optee_msg_param *mp, - const struct tee_param *p) + const struct tee_param *p, bool update_out) { int rc; phys_addr_t pa; + if (update_out) { + if (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT) + return 0; + goto out; + } + mp->attr = OPTEE_MSG_ATTR_TYPE_TMEM_INPUT + p->attr - TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT; mp->u.tmem.shm_ref = (unsigned long)p->u.memref.shm; - mp->u.tmem.size = p->u.memref.size; if (!p->u.memref.shm) { mp->u.tmem.buf_ptr = 0; - return 0; + goto out; } rc = tee_shm_get_pa(p->u.memref.shm, p->u.memref.shm_offs, &pa); @@ -201,19 +225,27 @@ static int to_msg_param_tmp_mem(struct optee_msg_param *mp, mp->u.tmem.buf_ptr = pa; mp->attr |= OPTEE_MSG_ATTR_CACHE_PREDEFINED << OPTEE_MSG_ATTR_CACHE_SHIFT; - +out: + mp->u.tmem.size = p->u.memref.size; return 0; } static int to_msg_param_reg_mem(struct optee_msg_param *mp, - const struct tee_param *p) + const struct tee_param *p, bool update_out) { + if (update_out) { + if (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT) + return 0; + goto out; + } + mp->attr = OPTEE_MSG_ATTR_TYPE_RMEM_INPUT + p->attr - TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT; mp->u.rmem.shm_ref = (unsigned long)p->u.memref.shm; - mp->u.rmem.size = p->u.memref.size; mp->u.rmem.offs = p->u.memref.shm_offs; +out: + mp->u.rmem.size = p->u.memref.size; return 0; } @@ -223,11 +255,13 @@ static int to_msg_param_reg_mem(struct optee_msg_param *mp, * @msg_params: OPTEE_MSG parameters * @num_params: number of elements in the parameter arrays * @params: subsystem itnernal parameter representation + * @update_out: update parameter for output only * Returns 0 on success or <0 on failure */ static int optee_to_msg_param(struct optee *optee, struct optee_msg_param *msg_params, - size_t num_params, const struct tee_param *params) + size_t num_params, const struct tee_param *params, + bool update_out) { int rc; size_t n; @@ -238,21 +272,23 @@ static int optee_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 (tee_shm_is_dynamic(p->u.memref.shm)) - rc = to_msg_param_reg_mem(mp, p); + rc = to_msg_param_reg_mem(mp, p, update_out); else - rc = to_msg_param_tmp_mem(mp, p); + rc = to_msg_param_tmp_mem(mp, p, update_out); if (rc) return rc; break; -- 2.43.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v6 03/10] optee: account for direction while converting parameters 2025-03-05 13:04 ` [PATCH v6 03/10] optee: account for direction while converting parameters Jens Wiklander @ 2025-03-13 10:41 ` Sumit Garg 2025-03-17 7:42 ` Jens Wiklander 0 siblings, 1 reply; 9+ messages in thread From: Sumit Garg @ 2025-03-13 10:41 UTC (permalink / raw) To: op-tee [-- Attachment #1: Type: text/plain, Size: 24503 bytes --] 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@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; 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 > diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h > index dc0f355ef72a..20eda508dbac 100644 > --- a/drivers/tee/optee/optee_private.h > +++ b/drivers/tee/optee/optee_private.h > @@ -185,10 +185,12 @@ struct optee_ops { > bool system_thread); > int (*to_msg_param)(struct optee *optee, > struct optee_msg_param *msg_params, > - size_t num_params, const struct tee_param *params); > + size_t num_params, const struct tee_param *params, > + bool update_out); > int (*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); > }; > > /** > @@ -316,23 +318,35 @@ void optee_release(struct tee_context *ctx); > void optee_release_supp(struct tee_context *ctx); > > static inline void optee_from_msg_param_value(struct tee_param *p, u32 attr, > - const struct optee_msg_param *mp) > + const struct optee_msg_param *mp, > + bool update_out) > { > - p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT + > - attr - OPTEE_MSG_ATTR_TYPE_VALUE_INPUT; > - p->u.value.a = mp->u.value.a; > - p->u.value.b = mp->u.value.b; > - p->u.value.c = mp->u.value.c; > + if (!update_out) > + p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT + > + attr - OPTEE_MSG_ATTR_TYPE_VALUE_INPUT; > + > + if (attr == OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT || > + attr == OPTEE_MSG_ATTR_TYPE_VALUE_INOUT || !update_out) { > + p->u.value.a = mp->u.value.a; > + p->u.value.b = mp->u.value.b; > + p->u.value.c = mp->u.value.c; > + } > } > > static inline void optee_to_msg_param_value(struct optee_msg_param *mp, > - const struct tee_param *p) > + const struct tee_param *p, > + bool update_out) > { > - mp->attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT + p->attr - > - TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT; > - mp->u.value.a = p->u.value.a; > - mp->u.value.b = p->u.value.b; > - mp->u.value.c = p->u.value.c; > + if (!update_out) > + mp->attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT + p->attr - > + TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT; > + > + if (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT || > + p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT || !update_out) { > + mp->u.value.a = p->u.value.a; > + mp->u.value.b = p->u.value.b; > + mp->u.value.c = p->u.value.c; > + } > } > > void optee_cq_init(struct optee_call_queue *cq, int thread_count); > diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c > index ebbbd42b0e3e..580e6b9b0606 100644 > --- a/drivers/tee/optee/rpc.c > +++ b/drivers/tee/optee/rpc.c > @@ -63,7 +63,7 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx, > } > > if (optee->ops->from_msg_param(optee, params, arg->num_params, > - arg->params)) > + arg->params, false /*!update_out*/)) > goto bad; > > for (i = 0; i < arg->num_params; i++) { > @@ -107,7 +107,8 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx, > } else { > params[3].u.value.a = msg.len; > if (optee->ops->to_msg_param(optee, arg->params, > - arg->num_params, params)) > + arg->num_params, params, > + true /*update_out*/)) > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > else > arg->ret = TEEC_SUCCESS; > @@ -188,6 +189,7 @@ static void handle_rpc_func_cmd_wait(struct optee_msg_arg *arg) > static void handle_rpc_supp_cmd(struct tee_context *ctx, struct optee *optee, > struct optee_msg_arg *arg) > { > + bool update_out = false; > struct tee_param *params; > > arg->ret_origin = TEEC_ORIGIN_COMMS; > @@ -200,15 +202,21 @@ static void handle_rpc_supp_cmd(struct tee_context *ctx, struct optee *optee, > } > > if (optee->ops->from_msg_param(optee, params, arg->num_params, > - arg->params)) { > + arg->params, update_out)) { > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > goto out; > } > > arg->ret = optee_supp_thrd_req(ctx, arg->cmd, arg->num_params, params); > > + /* > + * Special treatment for OPTEE_RPC_CMD_SHM_ALLOC since input is a > + * value type, but the output is a memref type. > + */ > + if (arg->cmd != OPTEE_RPC_CMD_SHM_ALLOC) > + update_out = true; > if (optee->ops->to_msg_param(optee, arg->params, arg->num_params, > - params)) > + params, update_out)) > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > out: > kfree(params); > @@ -270,7 +278,7 @@ static void handle_rpc_func_rpmb_probe_reset(struct tee_context *ctx, > > if (arg->num_params != ARRAY_SIZE(params) || > optee->ops->from_msg_param(optee, params, arg->num_params, > - arg->params) || > + arg->params, false /*!update_out*/) || > params[0].attr != TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT) { > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > return; > @@ -280,7 +288,8 @@ static void handle_rpc_func_rpmb_probe_reset(struct tee_context *ctx, > params[0].u.value.b = 0; > params[0].u.value.c = 0; > if (optee->ops->to_msg_param(optee, arg->params, > - arg->num_params, params)) { > + arg->num_params, params, > + true /*update_out*/)) { > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > return; > } > @@ -324,7 +333,7 @@ static void handle_rpc_func_rpmb_probe_next(struct tee_context *ctx, > > if (arg->num_params != ARRAY_SIZE(params) || > optee->ops->from_msg_param(optee, params, arg->num_params, > - arg->params) || > + arg->params, false /*!update_out*/) || > params[0].attr != TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT || > params[1].attr != TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT) { > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > @@ -358,7 +367,8 @@ static void handle_rpc_func_rpmb_probe_next(struct tee_context *ctx, > params[0].u.value.b = rdev->descr.capacity; > params[0].u.value.c = rdev->descr.reliable_wr_count; > if (optee->ops->to_msg_param(optee, arg->params, > - arg->num_params, params)) { > + arg->num_params, params, > + true /*update_out*/)) { > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > return; > } > @@ -384,7 +394,7 @@ static void handle_rpc_func_rpmb_frames(struct tee_context *ctx, > > if (arg->num_params != ARRAY_SIZE(params) || > optee->ops->from_msg_param(optee, params, arg->num_params, > - arg->params) || > + arg->params, false /*!update_out*/) || > params[0].attr != TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT || > params[1].attr != TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT) { > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > @@ -401,7 +411,8 @@ static void handle_rpc_func_rpmb_frames(struct tee_context *ctx, > goto out; > } > if (optee->ops->to_msg_param(optee, arg->params, > - arg->num_params, params)) { > + arg->num_params, params, > + true /*update_out*/)) { > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > goto out; > } > diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c > index 165fadd9abc9..cfdae266548b 100644 > --- a/drivers/tee/optee/smc_abi.c > +++ b/drivers/tee/optee/smc_abi.c > @@ -81,20 +81,26 @@ static int optee_cpuhp_disable_pcpu_irq(unsigned int cpu) > */ > > static int from_msg_param_tmp_mem(struct tee_param *p, u32 attr, > - const struct optee_msg_param *mp) > + const struct optee_msg_param *mp, > + bool update_out) > { > struct tee_shm *shm; > phys_addr_t pa; > int rc; > > + if (update_out) { > + if (attr == OPTEE_MSG_ATTR_TYPE_TMEM_INPUT) > + return 0; > + goto out; > + } > + > p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT + > attr - OPTEE_MSG_ATTR_TYPE_TMEM_INPUT; > - p->u.memref.size = mp->u.tmem.size; > shm = (struct tee_shm *)(unsigned long)mp->u.tmem.shm_ref; > if (!shm) { > p->u.memref.shm_offs = 0; > p->u.memref.shm = NULL; > - return 0; > + goto out; > } > > rc = tee_shm_get_pa(shm, 0, &pa); > @@ -103,18 +109,25 @@ static int from_msg_param_tmp_mem(struct tee_param *p, u32 attr, > > p->u.memref.shm_offs = mp->u.tmem.buf_ptr - pa; > p->u.memref.shm = shm; > - > +out: > + p->u.memref.size = mp->u.tmem.size; > return 0; > } > > static void from_msg_param_reg_mem(struct tee_param *p, u32 attr, > - const struct optee_msg_param *mp) > + const struct optee_msg_param *mp, > + bool update_out) > { > struct tee_shm *shm; > > + if (update_out) { > + if (attr == OPTEE_MSG_ATTR_TYPE_RMEM_INPUT) > + return; > + goto out; > + } > + > p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT + > attr - OPTEE_MSG_ATTR_TYPE_RMEM_INPUT; > - p->u.memref.size = mp->u.rmem.size; > shm = (struct tee_shm *)(unsigned long)mp->u.rmem.shm_ref; > > if (shm) { > @@ -124,6 +137,8 @@ static void from_msg_param_reg_mem(struct tee_param *p, u32 attr, > p->u.memref.shm_offs = 0; > p->u.memref.shm = NULL; > } > +out: > + p->u.memref.size = mp->u.rmem.size; > } > > /** > @@ -133,11 +148,13 @@ static void from_msg_param_reg_mem(struct tee_param *p, u32 attr, > * @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_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) > { > int rc; > size_t n; > @@ -149,25 +166,27 @@ static int optee_from_msg_param(struct optee *optee, struct tee_param *params, > > 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_TMEM_INPUT: > case OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT: > case OPTEE_MSG_ATTR_TYPE_TMEM_INOUT: > - rc = from_msg_param_tmp_mem(p, attr, mp); > + rc = from_msg_param_tmp_mem(p, attr, mp, update_out); > if (rc) > return rc; > break; > case OPTEE_MSG_ATTR_TYPE_RMEM_INPUT: > case OPTEE_MSG_ATTR_TYPE_RMEM_OUTPUT: > case OPTEE_MSG_ATTR_TYPE_RMEM_INOUT: > - from_msg_param_reg_mem(p, attr, mp); > + from_msg_param_reg_mem(p, attr, mp, update_out); > break; > > default: > @@ -178,20 +197,25 @@ static int optee_from_msg_param(struct optee *optee, struct tee_param *params, > } > > static int to_msg_param_tmp_mem(struct optee_msg_param *mp, > - const struct tee_param *p) > + const struct tee_param *p, bool update_out) > { > int rc; > phys_addr_t pa; > > + if (update_out) { > + if (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT) > + return 0; > + goto out; > + } > + > mp->attr = OPTEE_MSG_ATTR_TYPE_TMEM_INPUT + p->attr - > TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT; > > mp->u.tmem.shm_ref = (unsigned long)p->u.memref.shm; > - mp->u.tmem.size = p->u.memref.size; > > if (!p->u.memref.shm) { > mp->u.tmem.buf_ptr = 0; > - return 0; > + goto out; > } > > rc = tee_shm_get_pa(p->u.memref.shm, p->u.memref.shm_offs, &pa); > @@ -201,19 +225,27 @@ static int to_msg_param_tmp_mem(struct optee_msg_param *mp, > mp->u.tmem.buf_ptr = pa; > mp->attr |= OPTEE_MSG_ATTR_CACHE_PREDEFINED << > OPTEE_MSG_ATTR_CACHE_SHIFT; > - > +out: > + mp->u.tmem.size = p->u.memref.size; > return 0; > } > > static int to_msg_param_reg_mem(struct optee_msg_param *mp, > - const struct tee_param *p) > + const struct tee_param *p, bool update_out) > { > + if (update_out) { > + if (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT) > + return 0; > + goto out; > + } > + > mp->attr = OPTEE_MSG_ATTR_TYPE_RMEM_INPUT + p->attr - > TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT; > > mp->u.rmem.shm_ref = (unsigned long)p->u.memref.shm; > - mp->u.rmem.size = p->u.memref.size; > mp->u.rmem.offs = p->u.memref.shm_offs; > +out: > + mp->u.rmem.size = p->u.memref.size; > return 0; > } > > @@ -223,11 +255,13 @@ static int to_msg_param_reg_mem(struct optee_msg_param *mp, > * @msg_params: OPTEE_MSG parameters > * @num_params: number of elements in the parameter arrays > * @params: subsystem itnernal parameter representation > + * @update_out: update parameter for output only > * Returns 0 on success or <0 on failure > */ > static int optee_to_msg_param(struct optee *optee, > struct optee_msg_param *msg_params, > - size_t num_params, const struct tee_param *params) > + size_t num_params, const struct tee_param *params, > + bool update_out) > { > int rc; > size_t n; > @@ -238,21 +272,23 @@ static int optee_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 (tee_shm_is_dynamic(p->u.memref.shm)) > - rc = to_msg_param_reg_mem(mp, p); > + rc = to_msg_param_reg_mem(mp, p, update_out); > else > - rc = to_msg_param_tmp_mem(mp, p); > + rc = to_msg_param_tmp_mem(mp, p, update_out); > if (rc) > return rc; > break; > -- > 2.43.0 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v6 03/10] optee: account for direction while converting parameters 2025-03-13 10:41 ` Sumit Garg @ 2025-03-17 7:42 ` Jens Wiklander 0 siblings, 0 replies; 9+ messages in thread From: Jens Wiklander @ 2025-03-17 7:42 UTC (permalink / raw) To: op-tee [-- Attachment #1: Type: text/plain, Size: 30975 bytes --] Hi Sumit, On Thu, Mar 13, 2025 at 11:41 AM Sumit Garg <sumit.garg@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@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. 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 > > > diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h > > index dc0f355ef72a..20eda508dbac 100644 > > --- a/drivers/tee/optee/optee_private.h > > +++ b/drivers/tee/optee/optee_private.h > > @@ -185,10 +185,12 @@ struct optee_ops { > > bool system_thread); > > int (*to_msg_param)(struct optee *optee, > > struct optee_msg_param *msg_params, > > - size_t num_params, const struct tee_param *params); > > + size_t num_params, const struct tee_param *params, > > + bool update_out); > > int (*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); > > }; > > > > /** > > @@ -316,23 +318,35 @@ void optee_release(struct tee_context *ctx); > > void optee_release_supp(struct tee_context *ctx); > > > > static inline void optee_from_msg_param_value(struct tee_param *p, u32 attr, > > - const struct optee_msg_param *mp) > > + const struct optee_msg_param *mp, > > + bool update_out) > > { > > - p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT + > > - attr - OPTEE_MSG_ATTR_TYPE_VALUE_INPUT; > > - p->u.value.a = mp->u.value.a; > > - p->u.value.b = mp->u.value.b; > > - p->u.value.c = mp->u.value.c; > > + if (!update_out) > > + p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT + > > + attr - OPTEE_MSG_ATTR_TYPE_VALUE_INPUT; > > + > > + if (attr == OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT || > > + attr == OPTEE_MSG_ATTR_TYPE_VALUE_INOUT || !update_out) { > > + p->u.value.a = mp->u.value.a; > > + p->u.value.b = mp->u.value.b; > > + p->u.value.c = mp->u.value.c; > > + } > > } > > > > static inline void optee_to_msg_param_value(struct optee_msg_param *mp, > > - const struct tee_param *p) > > + const struct tee_param *p, > > + bool update_out) > > { > > - mp->attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT + p->attr - > > - TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT; > > - mp->u.value.a = p->u.value.a; > > - mp->u.value.b = p->u.value.b; > > - mp->u.value.c = p->u.value.c; > > + if (!update_out) > > + mp->attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT + p->attr - > > + TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT; > > + > > + if (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT || > > + p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT || !update_out) { > > + mp->u.value.a = p->u.value.a; > > + mp->u.value.b = p->u.value.b; > > + mp->u.value.c = p->u.value.c; > > + } > > } > > > > void optee_cq_init(struct optee_call_queue *cq, int thread_count); > > diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c > > index ebbbd42b0e3e..580e6b9b0606 100644 > > --- a/drivers/tee/optee/rpc.c > > +++ b/drivers/tee/optee/rpc.c > > @@ -63,7 +63,7 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx, > > } > > > > if (optee->ops->from_msg_param(optee, params, arg->num_params, > > - arg->params)) > > + arg->params, false /*!update_out*/)) > > goto bad; > > > > for (i = 0; i < arg->num_params; i++) { > > @@ -107,7 +107,8 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx, > > } else { > > params[3].u.value.a = msg.len; > > if (optee->ops->to_msg_param(optee, arg->params, > > - arg->num_params, params)) > > + arg->num_params, params, > > + true /*update_out*/)) > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > else > > arg->ret = TEEC_SUCCESS; > > @@ -188,6 +189,7 @@ static void handle_rpc_func_cmd_wait(struct optee_msg_arg *arg) > > static void handle_rpc_supp_cmd(struct tee_context *ctx, struct optee *optee, > > struct optee_msg_arg *arg) > > { > > + bool update_out = false; > > struct tee_param *params; > > > > arg->ret_origin = TEEC_ORIGIN_COMMS; > > @@ -200,15 +202,21 @@ static void handle_rpc_supp_cmd(struct tee_context *ctx, struct optee *optee, > > } > > > > if (optee->ops->from_msg_param(optee, params, arg->num_params, > > - arg->params)) { > > + arg->params, update_out)) { > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > goto out; > > } > > > > arg->ret = optee_supp_thrd_req(ctx, arg->cmd, arg->num_params, params); > > > > + /* > > + * Special treatment for OPTEE_RPC_CMD_SHM_ALLOC since input is a > > + * value type, but the output is a memref type. > > + */ > > + if (arg->cmd != OPTEE_RPC_CMD_SHM_ALLOC) > > + update_out = true; > > if (optee->ops->to_msg_param(optee, arg->params, arg->num_params, > > - params)) > > + params, update_out)) > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > out: > > kfree(params); > > @@ -270,7 +278,7 @@ static void handle_rpc_func_rpmb_probe_reset(struct tee_context *ctx, > > > > if (arg->num_params != ARRAY_SIZE(params) || > > optee->ops->from_msg_param(optee, params, arg->num_params, > > - arg->params) || > > + arg->params, false /*!update_out*/) || > > params[0].attr != TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT) { > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > return; > > @@ -280,7 +288,8 @@ static void handle_rpc_func_rpmb_probe_reset(struct tee_context *ctx, > > params[0].u.value.b = 0; > > params[0].u.value.c = 0; > > if (optee->ops->to_msg_param(optee, arg->params, > > - arg->num_params, params)) { > > + arg->num_params, params, > > + true /*update_out*/)) { > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > return; > > } > > @@ -324,7 +333,7 @@ static void handle_rpc_func_rpmb_probe_next(struct tee_context *ctx, > > > > if (arg->num_params != ARRAY_SIZE(params) || > > optee->ops->from_msg_param(optee, params, arg->num_params, > > - arg->params) || > > + arg->params, false /*!update_out*/) || > > params[0].attr != TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT || > > params[1].attr != TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT) { > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > @@ -358,7 +367,8 @@ static void handle_rpc_func_rpmb_probe_next(struct tee_context *ctx, > > params[0].u.value.b = rdev->descr.capacity; > > params[0].u.value.c = rdev->descr.reliable_wr_count; > > if (optee->ops->to_msg_param(optee, arg->params, > > - arg->num_params, params)) { > > + arg->num_params, params, > > + true /*update_out*/)) { > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > return; > > } > > @@ -384,7 +394,7 @@ static void handle_rpc_func_rpmb_frames(struct tee_context *ctx, > > > > if (arg->num_params != ARRAY_SIZE(params) || > > optee->ops->from_msg_param(optee, params, arg->num_params, > > - arg->params) || > > + arg->params, false /*!update_out*/) || > > params[0].attr != TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT || > > params[1].attr != TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT) { > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > @@ -401,7 +411,8 @@ static void handle_rpc_func_rpmb_frames(struct tee_context *ctx, > > goto out; > > } > > if (optee->ops->to_msg_param(optee, arg->params, > > - arg->num_params, params)) { > > + arg->num_params, params, > > + true /*update_out*/)) { > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > goto out; > > } > > diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c > > index 165fadd9abc9..cfdae266548b 100644 > > --- a/drivers/tee/optee/smc_abi.c > > +++ b/drivers/tee/optee/smc_abi.c > > @@ -81,20 +81,26 @@ static int optee_cpuhp_disable_pcpu_irq(unsigned int cpu) > > */ > > > > static int from_msg_param_tmp_mem(struct tee_param *p, u32 attr, > > - const struct optee_msg_param *mp) > > + const struct optee_msg_param *mp, > > + bool update_out) > > { > > struct tee_shm *shm; > > phys_addr_t pa; > > int rc; > > > > + if (update_out) { > > + if (attr == OPTEE_MSG_ATTR_TYPE_TMEM_INPUT) > > + return 0; > > + goto out; > > + } > > + > > p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT + > > attr - OPTEE_MSG_ATTR_TYPE_TMEM_INPUT; > > - p->u.memref.size = mp->u.tmem.size; > > shm = (struct tee_shm *)(unsigned long)mp->u.tmem.shm_ref; > > if (!shm) { > > p->u.memref.shm_offs = 0; > > p->u.memref.shm = NULL; > > - return 0; > > + goto out; > > } > > > > rc = tee_shm_get_pa(shm, 0, &pa); > > @@ -103,18 +109,25 @@ static int from_msg_param_tmp_mem(struct tee_param *p, u32 attr, > > > > p->u.memref.shm_offs = mp->u.tmem.buf_ptr - pa; > > p->u.memref.shm = shm; > > - > > +out: > > + p->u.memref.size = mp->u.tmem.size; > > return 0; > > } > > > > static void from_msg_param_reg_mem(struct tee_param *p, u32 attr, > > - const struct optee_msg_param *mp) > > + const struct optee_msg_param *mp, > > + bool update_out) > > { > > struct tee_shm *shm; > > > > + if (update_out) { > > + if (attr == OPTEE_MSG_ATTR_TYPE_RMEM_INPUT) > > + return; > > + goto out; > > + } > > + > > p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT + > > attr - OPTEE_MSG_ATTR_TYPE_RMEM_INPUT; > > - p->u.memref.size = mp->u.rmem.size; > > shm = (struct tee_shm *)(unsigned long)mp->u.rmem.shm_ref; > > > > if (shm) { > > @@ -124,6 +137,8 @@ static void from_msg_param_reg_mem(struct tee_param *p, u32 attr, > > p->u.memref.shm_offs = 0; > > p->u.memref.shm = NULL; > > } > > +out: > > + p->u.memref.size = mp->u.rmem.size; > > } > > > > /** > > @@ -133,11 +148,13 @@ static void from_msg_param_reg_mem(struct tee_param *p, u32 attr, > > * @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_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) > > { > > int rc; > > size_t n; > > @@ -149,25 +166,27 @@ static int optee_from_msg_param(struct optee *optee, struct tee_param *params, > > > > 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_TMEM_INPUT: > > case OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT: > > case OPTEE_MSG_ATTR_TYPE_TMEM_INOUT: > > - rc = from_msg_param_tmp_mem(p, attr, mp); > > + rc = from_msg_param_tmp_mem(p, attr, mp, update_out); > > if (rc) > > return rc; > > break; > > case OPTEE_MSG_ATTR_TYPE_RMEM_INPUT: > > case OPTEE_MSG_ATTR_TYPE_RMEM_OUTPUT: > > case OPTEE_MSG_ATTR_TYPE_RMEM_INOUT: > > - from_msg_param_reg_mem(p, attr, mp); > > + from_msg_param_reg_mem(p, attr, mp, update_out); > > break; > > > > default: > > @@ -178,20 +197,25 @@ static int optee_from_msg_param(struct optee *optee, struct tee_param *params, > > } > > > > static int to_msg_param_tmp_mem(struct optee_msg_param *mp, > > - const struct tee_param *p) > > + const struct tee_param *p, bool update_out) > > { > > int rc; > > phys_addr_t pa; > > > > + if (update_out) { > > + if (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT) > > + return 0; > > + goto out; > > + } > > + > > mp->attr = OPTEE_MSG_ATTR_TYPE_TMEM_INPUT + p->attr - > > TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT; > > > > mp->u.tmem.shm_ref = (unsigned long)p->u.memref.shm; > > - mp->u.tmem.size = p->u.memref.size; > > > > if (!p->u.memref.shm) { > > mp->u.tmem.buf_ptr = 0; > > - return 0; > > + goto out; > > } > > > > rc = tee_shm_get_pa(p->u.memref.shm, p->u.memref.shm_offs, &pa); > > @@ -201,19 +225,27 @@ static int to_msg_param_tmp_mem(struct optee_msg_param *mp, > > mp->u.tmem.buf_ptr = pa; > > mp->attr |= OPTEE_MSG_ATTR_CACHE_PREDEFINED << > > OPTEE_MSG_ATTR_CACHE_SHIFT; > > - > > +out: > > + mp->u.tmem.size = p->u.memref.size; > > return 0; > > } > > > > static int to_msg_param_reg_mem(struct optee_msg_param *mp, > > - const struct tee_param *p) > > + const struct tee_param *p, bool update_out) > > { > > + if (update_out) { > > + if (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT) > > + return 0; > > + goto out; > > + } > > + > > mp->attr = OPTEE_MSG_ATTR_TYPE_RMEM_INPUT + p->attr - > > TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT; > > > > mp->u.rmem.shm_ref = (unsigned long)p->u.memref.shm; > > - mp->u.rmem.size = p->u.memref.size; > > mp->u.rmem.offs = p->u.memref.shm_offs; > > +out: > > + mp->u.rmem.size = p->u.memref.size; > > return 0; > > } > > > > @@ -223,11 +255,13 @@ static int to_msg_param_reg_mem(struct optee_msg_param *mp, > > * @msg_params: OPTEE_MSG parameters > > * @num_params: number of elements in the parameter arrays > > * @params: subsystem itnernal parameter representation > > + * @update_out: update parameter for output only > > * Returns 0 on success or <0 on failure > > */ > > static int optee_to_msg_param(struct optee *optee, > > struct optee_msg_param *msg_params, > > - size_t num_params, const struct tee_param *params) > > + size_t num_params, const struct tee_param *params, > > + bool update_out) > > { > > int rc; > > size_t n; > > @@ -238,21 +272,23 @@ static int optee_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 (tee_shm_is_dynamic(p->u.memref.shm)) > > - rc = to_msg_param_reg_mem(mp, p); > > + rc = to_msg_param_reg_mem(mp, p, update_out); > > else > > - rc = to_msg_param_tmp_mem(mp, p); > > + rc = to_msg_param_tmp_mem(mp, p, update_out); > > if (rc) > > return rc; > > break; > > -- > > 2.43.0 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-04-01 8:21 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] < <CAHUa44H5Zc_POJ_RWaVd4iFVagRkFaN+8Ajs=19FKboZapbQHw@mail.gmail.com>
2025-03-20 9:25 ` [PATCH v6 03/10] optee: account for direction while converting parameters Sumit Garg
2025-03-20 13:00 ` Jens Wiklander
[not found] < <CAHUa44HXHwR1_LbjG6T5OqnafXiS4-kHeKjV9TurGORx3SsprQ@mail.gmail.com>
2025-04-01 7:45 ` Sumit Garg
2025-04-01 8:21 ` Jens Wiklander
[not found] < <CAHUa44HY-jZCoDXKnx-f4gBoABRVdh1+6PA1AYHSva9aL=rDAA@mail.gmail.com>
2025-03-25 5:55 ` Sumit Garg
2025-03-25 8:50 ` Jens Wiklander
2025-03-05 13:04 [PATCH v6 00/10] TEE subsystem for restricted dma-buf allocations Jens Wiklander
2025-03-05 13:04 ` [PATCH v6 03/10] optee: account for direction while converting parameters Jens Wiklander
2025-03-13 10:41 ` Sumit Garg
2025-03-17 7:42 ` Jens Wiklander
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox