Re: [PATCH v3 07/11] optee: add support for RPC SHM buffers

[Julien Grall <julien.grall@arm.com>]

Hi Volodymyr,

On 18/01/2019 16:05, Volodymyr Babchuk wrote:
> Julien Grall writes:
>>>> If you happen to make MAX_STD_CALLS dynamic, then this should also be
>>>> dynamic.
>>> Of course.
>>>
>>> [...]
>>>>>>>      +static void handle_rpc_func_alloc(struct optee_domain *ctx,
>>>>>>> +                                  struct cpu_user_regs *regs)
>>>>>>> +{
>>>>>>> +    paddr_t ptr = get_user_reg(regs, 1) << 32 | get_user_reg(regs, 2);
>>>>>>> +
>>>>>>> +    if ( ptr & (OPTEE_MSG_NONCONTIG_PAGE_SIZE - 1) )
>>>>>>> +        gprintk(XENLOG_WARNING, "Domain returned invalid RPC command buffer\n");
>>>>>>
>>>>>> Should not you bail-out in that case? Also, I would turn it to a gdprintk.
>>>>> OP-TEE does own checks and that check will fail also. Then OP-TEE will
>>>>> issue request to free this SHM.
>>>>
>>>> I think it is better if we go on the safe-side. I.e if we know there
>>>> would be an error (like here), then you need to return an error in
>>>> from Xen rather than calling OP-TEE. Otherwise, you may end up to
>>>> nasty problem.
>>> Actually, I don't see how I can do this. This is an RPC response. I
>>> can't return error to RPC response. All I can do is to mangle RPC
>>> response in a such way, that OP-TEE surely will treat it as an error and
>>> act accordingly.
>>
>> I am not sure to understand what you mean here. Surely if the address
>> is not aligned, then OP-TEE will return an error too, right? So can't
>> you emulate OP-TEE behavior in that case?
> No, OP-TEE will not return an error. OP-TEE will handle it as an error.

Will it? Looking at the code again, you will pass either 0 (when not able to 
translate the address) or page_to_maddr(...) value. So the value will get 
truncated by Xen with just a warning.

Is it the expected behavior?

> For OP-TEE any RPC looks like a function call. So it excepts some return
> value - buffer pointer in this case. If it gets invalid pointer, it
> issues another RPC to free this buffer and then propagates error back to
> caller.
> 
> So, if I'll return error back to the guest (or rather issue RPC to free
> the buffer), OP-TEE will be still blocked, waiting for a pointer from
> the guest. Of course I can go further end emulate a new buffer
> allocation RPC but from Xen side in a hope that guest will provide valid
> buffer this time. But, what if not? And why I should decide instead of
> OP-TEE?

My main concern here is you do a print a warning and continue like nothing 
happen. As a reviewer, this is worrying because it is a call for weird behavior 
to happen in Xen. We don't want that.

Looking at the code, why can't you pass 0 as you do if you fail to pin the pages?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel