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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Julien,

Julien Grall writes:

> 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?
Yes. It checks for page alignment in the same way, as Xen does.

> 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?
No, this is a bug and I'll fix it.

>> 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.
Yep, I made a mistake there. I need to pass invalid value to OP-TEE to
make sure that it will not mistake it with semi-valid address.

> Looking at the code, why can't you pass 0 as you do if you fail to pin the 
> pages?
This is exactly what I'm going to do.

-- 
Best regards, Volodymyr Babchuk
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel