Re: [Tee-dev] [PATCH 4/5] xen/arm: optee: handle share buffer translation error

Hi Volodymyr,

On 8/23/19 7:48 PM, Volodymyr Babchuk wrote:

There is a case possible, when OP-TEE asks guest to allocate shared buffer, but Xen for some reason can't translate buffer's addresses. In this situation we should do two things:

1. Tell guest to free allocated buffer, so there will be no memory

leak for guest.

2. Tell OP-TEE that buffer allocation failed.

To ask guest to free allocated buffer we should perform the same thing, as OP-TEE does - issue RPC request. This is done by filling request buffer (luckily we can reuse the same buffer, that OP-TEE used to issue original request) and then return to guest with special return code.

Then we need to handle next call from guest in a special way: as RPC was issued by Xen, not by OP-TEE, it should be handled by Xen. Basically, this is the mechanism to preempt OP-TEE mediator.

The same mechanism can be used in the future to preempt mediator during translation large (>512 pages) shared buffers.

Signed-off-by: Volodymyr Babchuk volodymyr_babchuk@epam.com

xen/arch/arm/tee/optee.c | 167 +++++++++++++++++++++++++++++++-------- 1 file changed, 136 insertions(+), 31 deletions(-)

diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c index 3ce6e7fa55..4eebc60b62 100644 --- a/xen/arch/arm/tee/optee.c +++ b/xen/arch/arm/tee/optee.c @@ -96,6 +96,11 @@ OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM | \ OPTEE_SMC_SEC_CAP_DYNAMIC_SHM) +enum optee_call_state {

• OPTEEM_CALL_NORMAL = 0,

enum always start counting at 0. Also, looking at the code, it does not seem you need to know the value. Right?

• OPTEEM_CALL_XEN_RPC,

I am a bit confused, the enum is called optee_call_state but all the enum are prefixed with OPTEEM_CALL_. Why the discrepancy?

+};

• static unsigned int __read_mostly max_optee_threads;

/* @@ -112,6 +117,9 @@ struct optee_std_call { paddr_t guest_arg_ipa; int optee_thread_id; int rpc_op;

• /* Saved buffer type for the last buffer allocate request */

Looking at the code, it feels to me you are saving the buffer type for the current command and not the last. Did I miss anything?

• unsigned int rpc_buffer_type;
• enum optee_call_state state; uint64_t rpc_data_cookie; bool in_flight; register_t rpc_params[2];

@@ -299,6 +307,7 @@ static struct optee_std_call *allocate_std_call(struct optee_domain *ctx) call->optee_thread_id = -1; call->in_flight = true;

• call->state = OPTEEM_CALL_NORMAL;

spin_lock(&ctx->lock); list_add_tail(&call->list, &ctx->call_list); @@ -1075,6 +1084,10 @@ static int handle_rpc_return(struct optee_domain *ctx, ret = -ERESTART; }

•    /* Save the buffer type in case we will want to free it */
  

•    if ( shm_rpc->xen_arg->cmd == OPTEE_RPC_CMD_SHM_ALLOC )
  

•        call->rpc_buffer_type = shm_rpc->xen_arg->params[0].u.value.a;
  

•     unmap_domain_page(shm_rpc->xen_arg);
  }
  

@@ -1239,18 +1252,102 @@ err: return; } +/*

• • Prepare RPC request to free shared buffer in the same way, as
• • OP-TEE does this.
• • Return values:
• • true - successfully prepared RPC request
• • false - there was an error
• */

+static bool issue_rpc_cmd_free(struct optee_domain *ctx,

•                           struct cpu_user_regs *regs,
  

•                           struct optee_std_call *call,
  

•                           struct shm_rpc *shm_rpc,
  

•                           uint64_t cookie)
  

+{

• register_t r1, r2;
• /* In case if guest will forget to update it with meaningful value */
• shm_rpc->xen_arg->ret = TEEC_ERROR_GENERIC;
• shm_rpc->xen_arg->cmd = OPTEE_RPC_CMD_SHM_FREE;
• shm_rpc->xen_arg->num_params = 1;
• shm_rpc->xen_arg->params[0].attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT;
• shm_rpc->xen_arg->params[0].u.value.a = call->rpc_buffer_type;
• shm_rpc->xen_arg->params[0].u.value.b = cookie;
• if ( access_guest_memory_by_ipa(current->domain,

•                                gfn_to_gaddr(shm_rpc->gfn),
  

•                                shm_rpc->xen_arg,
  

•                                OPTEE_MSG_GET_ARG_SIZE(1),
  

•                                true) )
  

• {

•    /*
  

•     * Well, this is quite bad. We have error in error path.
  

•     * This can happen only if guest behaves badly, so all
  

•     * we can do is to return error to OP-TEE and leave
  

•     * guest's memory leaked.
  

Could you expand a bit more what you mean by "guest's memory leaked"? What the state of the page from Xen PoV? I.e. is there any reference taken by the OP-TEE mediator? Will the page be freed once the guest is destroyed?...

•     */
  

•    shm_rpc->xen_arg->ret = TEEC_ERROR_GENERIC;
  

•    shm_rpc->xen_arg->num_params = 0;
  

•    return false;
  

• }
• uint64_to_regpair(&r1, &r2, shm_rpc->cookie);
• call->state = OPTEEM_CALL_XEN_RPC;
• call->rpc_op = OPTEE_SMC_RPC_FUNC_CMD;
• call->rpc_params[0] = r1;
• call->rpc_params[1] = r2;
• call->optee_thread_id = get_user_reg(regs, 3);
• set_user_reg(regs, 0, OPTEE_SMC_RETURN_RPC_CMD);
• set_user_reg(regs, 1, r1);
• set_user_reg(regs, 2, r2);
• return true;

+}

+/* Handles return from Xen-issued RPC */ +static void handle_xen_rpc_return(struct optee_domain *ctx,

•                              struct cpu_user_regs *regs,
  

•                              struct optee_std_call *call,
  

•                              struct shm_rpc *shm_rpc)
  

+{

• call->state = OPTEEM_CALL_NORMAL;
• /*

• * Right now we have only one reason to be there - we asked guest
  

• * to free shared buffer and it did it. Now we can tell OP-TEE that
  

• * buffer allocation failed.
  

• */
  

Should we add an ASSERT to ensure the command is the one we expect?

• /*

• * We are not checking return value from a guest because we assume
  

• * that OPTEE_RPC_CMD_SHM_FREE newer fails.
  

s/newer/never/

• */
  

• shm_rpc->xen_arg->ret = TEEC_ERROR_GENERIC;
• shm_rpc->xen_arg->num_params = 0;

+}

• /*
  • This function is called when guest is finished processing RPC
  • request from OP-TEE and wished to resume the interrupted standard
  • call.
• • Return values:
• • false - there was an error, do not call OP-TEE
• • true - success, proceed as normal
  */

-static void handle_rpc_cmd_alloc(struct optee_domain *ctx, +static bool handle_rpc_cmd_alloc(struct optee_domain *ctx, struct cpu_user_regs *regs, struct optee_std_call *call, struct shm_rpc *shm_rpc) { if ( shm_rpc->xen_arg->ret || shm_rpc->xen_arg->num_params != 1 )

•    return;
  

•    return true;
  

if ( shm_rpc->xen_arg->params[0].attr != (OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT | OPTEE_MSG_ATTR_NONCONTIG) ) @@ -1258,7 +1355,7 @@ static void handle_rpc_cmd_alloc(struct optee_domain *ctx, gdprintk(XENLOG_WARNING, "Invalid attrs for shared mem buffer: %"PRIx64"\n", shm_rpc->xen_arg->params[0].attr);

•    return;
  

•    return true;
  }
  

/* Free pg list for buffer */ @@ -1274,21 +1371,14 @@ static void handle_rpc_cmd_alloc(struct optee_domain *ctx, { call->rpc_data_cookie = 0; /*

•     * Okay, so there was problem with guest's buffer and we need
  

•     * to tell about this to OP-TEE.
  

•     */
  

•    shm_rpc->xen_arg->ret = TEEC_ERROR_GENERIC;
  

•    shm_rpc->xen_arg->num_params = 0;
  

•    /*
  

•     * TODO: With current implementation, OP-TEE will not issue
  

•     * RPC to free this buffer. Guest and OP-TEE will be out of
  

•     * sync: guest believes that it provided buffer to OP-TEE,
  

•     * while OP-TEE thinks of opposite. Ideally, we need to
  

•     * emulate RPC with OPTEE_MSG_RPC_CMD_SHM_FREE command.
  

•     * We are unable to translate guest's buffer, so we need tell guest
  

•     * to free it, before returning error to OP-TEE.
  

Do you mean "reporting" instead of "returning"? Also s/error/an error/

       */

•    gprintk(XENLOG_WARNING,
  

•            "translate_noncontig() failed, OP-TEE/guest state is out of sync.\n");
  

•    return !issue_rpc_cmd_free(ctx, regs, call, shm_rpc,
  

•                               shm_rpc->xen_arg->params[0].u.tmem.shm_ref);
  }
  

• return true; }

static void handle_rpc_cmd(struct optee_domain *ctx, struct cpu_user_regs *regs, @@ -1338,22 +1428,37 @@ static void handle_rpc_cmd(struct optee_domain *ctx, struct cpu_user_regs *regs, goto out; }

• switch (shm_rpc->xen_arg->cmd)

• if ( call->state == OPTEEM_CALL_NORMAL ) {

• case OPTEE_RPC_CMD_GET_TIME:
• case OPTEE_RPC_CMD_WAIT_QUEUE:
• case OPTEE_RPC_CMD_SUSPEND:

•    break;
  

• case OPTEE_RPC_CMD_SHM_ALLOC:

•    handle_rpc_cmd_alloc(ctx, regs, call, shm_rpc);
  

•    break;
  

• case OPTEE_RPC_CMD_SHM_FREE:

•    free_optee_shm_buf(ctx, shm_rpc->xen_arg->params[0].u.value.b);
  

•    if ( call->rpc_data_cookie == shm_rpc->xen_arg->params[0].u.value.b )
  

•        call->rpc_data_cookie = 0;
  

•    break;
  

• default:

•    break;
  

•    switch (shm_rpc->xen_arg->cmd)
  

•    {
  

•    case OPTEE_RPC_CMD_GET_TIME:
  

•    case OPTEE_RPC_CMD_WAIT_QUEUE:
  

•    case OPTEE_RPC_CMD_SUSPEND:
  

•        break;
  

•    case OPTEE_RPC_CMD_SHM_ALLOC:
  

•        if ( !handle_rpc_cmd_alloc(ctx, regs, call, shm_rpc) )
  

•        {
  

•            /* We failed to translate buffer, report back to guest */
  

•            unmap_domain_page(shm_rpc->xen_arg);
  

•            put_std_call(ctx, call);
  

•            return;
  

•        }
  

•        break;
  

•    case OPTEE_RPC_CMD_SHM_FREE:
  

•        free_optee_shm_buf(ctx, shm_rpc->xen_arg->params[0].u.value.b);
  

•        if ( call->rpc_data_cookie ==
  

•             shm_rpc->xen_arg->params[0].u.value.b )
  

•            call->rpc_data_cookie = 0;
  

•        break;
  

•    default:
  

•        break;
  

•    }
  

• }
• else
• {

•    handle_xen_rpc_return(ctx, regs, call, shm_rpc);
  }
  

out:

Cheers,