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
linaro-mm-sig@lists.linaro.org
-
Jens Wiklander