Re: [PATCH] optee: immediately free buffers that are released by OP-TEE

[Julien Grall <julien@xxxxxxx>]

Hi Volodymyr,

On 06/05/2020 02:44, Volodymyr Babchuk wrote:
> Normal World can share buffer with OP-TEE for two reasons:
> 1. Some client application wants to exchange data with TA
> 2. OP-TEE asks for shared buffer for internal needs
>
> The second case was handle more strictly than necessary:
>
> 1. In RPC request OP-TEE asks for buffer
> 2. NW allocates buffer and provides it via RPC response
> 3. Xen pins pages and translates data
> 4. Xen provides buffer to OP-TEE
> 5. OP-TEE uses it
> 6. OP-TEE sends request to free the buffer
> 7. NW frees the buffer and sends the RPC response
> 8. Xen unpins pages and forgets about the buffer
>
> The problem is that Xen should forget about buffer in between stages 6
> and 7. I.e. the right flow should be like this:
>
> 6. OP-TEE sends request to free the buffer
> 7. Xen unpins pages and forgets about the buffer
> 8. NW frees the buffer and sends the RPC response
>
> This is because OP-TEE internally frees the buffer before sending the
> "free SHM buffer" request. So we have no reason to hold reference for
> this buffer anymore. Moreover, in multiprocessor systems NW have time
> to reuse buffer cookie for another buffer. Xen complained about this
> and denied the new buffer registration. I have seen this issue while
> running tests on iMX SoC.
>
> So, this patch basically corrects that behavior by freeing the buffer
> earlier, when handling RPC return from OP-TEE.
>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> ---
>   xen/arch/arm/tee/optee.c | 24 ++++++++++++++++++++----
>   1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
> index 6a035355db..af19fc31f8 100644
> --- a/xen/arch/arm/tee/optee.c
> +++ b/xen/arch/arm/tee/optee.c
> @@ -1099,6 +1099,26 @@ static int handle_rpc_return(struct optee_domain *ctx,
>           if ( shm_rpc->xen_arg->cmd == OPTEE_RPC_CMD_SHM_ALLOC )
>               call->rpc_buffer_type = shm_rpc->xen_arg->params[0].u.value.a;
>   
> +        /*
> +         * OP-TEE signals that it frees the buffer that it requested
> +         * before. This is the right for us to do the same.
> +         */
> +        if ( shm_rpc->xen_arg->cmd == OPTEE_RPC_CMD_SHM_FREE )
> +        {
> +            uint64_t cookie = shm_rpc->xen_arg->params[0].u.value.b;
> +
> +            free_optee_shm_buf(ctx, cookie);
> +
> +            /*
> +             * This should never happen. We have a bug either in the
> +             * OP-TEE or in the mediator.
> +             */
> +            if ( call->rpc_data_cookie && call->rpc_data_cookie != cookie )
> +                gprintk(XENLOG_ERR,
> +                        "Saved RPC cookie does not corresponds to OP-TEE's (%"PRIx64" != 
> %"PRIx64")\n",
s/corresponds/correspond/

> +                        call->rpc_data_cookie, cookie);
IIUC, if you free the wrong SHM buffer then your guest is likely to be 
running incorrectly afterwards. So shouldn't we crash the guest to avoid 
further issue?
> +            call->rpc_data_cookie = 0;
> +        }
>           unmap_domain_page(shm_rpc->xen_arg);
>       }
>   
> @@ -1464,10 +1484,6 @@ static void handle_rpc_cmd(struct optee_domain *ctx, struct cpu_user_regs *regs,
>               }
>               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;
Cheers,

--
Julien Grall