Re: [PATCH] xen/arm: optee: allow plain TMEM buffers with NULL address

[Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>]

Julien Grall writes:

> On 19/06/2020 00:29, Volodymyr Babchuk wrote:
>>
>> Hi Julien,
>
> Hi Volodymyr,
>
>>
>> Julien Grall writes:
>>
>>> (+Paul)
>>>
>>> Hi,
>>>
>>> On 18/05/2020 02:53, Volodymyr Babchuk wrote:
>>>> Trusted Applications use popular approach to determine required size
>>>> of buffer: client provides a memory reference with the NULL pointer to
>>>> a buffer. This is so called "Null memory reference".  TA updates the
>>>
>>> NIT: You use double space after '.' here but all the others use a
>>> single space.
>>
>> Oops, missed that.
>>
>>>> reference with the required size and returns it back to client. Then
>>>> client allocates buffer of needed size and repeats the operation.
>>>>
>>>> This behavior is described in TEE Client API Specification, paragraph
>>>> 3.2.5. Memory References.
>>>
>>>  From the spec, it is not a clear cut that NULL will always used as
>>> fetching the required size of an output buffer. In particular, they
>>> suggest to refer to the protocol.
>>>
>>> In your commit message you indirectly point to an example where 0/NULL
>>> would have a different meaning depending on the flags. This is not
>>> explained in the TEE Client API Specification. Do you have some
>>> pointer I could use to check the behavior?
>>
>> Well, GP specification describes application interface. It does not
>> specifies how implementation should handle this internally. Basically,
>> GP defines functions, data types, macros, etc to be used by CAs and
>> TAs. But it does not define how exactly call from CA will be delivered
>> to TA. Implementation is free to use any means as far, as they conform
>> with rules described in the specification.
>>
>> OPTEE's REE<->TEE interface is described in the header files, that I
>> added to xen (optee_msg.h, optee_rpc_cmd.h,optee_smc.h). But it does not
>> describe every aspect, unfortunately.
>
> Thanks for digging-through! More below.
>
>>
>>>>
>>>> OP-TEE represents this null memory reference as a TMEM parameter with
>>>> buf_ptr == NULL. This is the only case when we should allow TMEM
>>>> buffer without the OPTEE_MSG_ATTR_NONCONTIG flag.
>>>
>>> IIUC, 0 with OPTEE_MSG_ATTR_NONCONTIG set would mean "use the buffer
>>> at address 0" but with the flag cleared, it would mean "return the
>>> size". Am I correct?
>>
>> Not exactly. This flag does not affect behavior for buffers with address
>> NULL. In any case, this would not mean "return the size" to
>> OP-TEE. This is because OP-TEE works as a transport between CA and TA
>> and it does not assign any meaning to client buffers. But certainly,
>> null buffer will mean "return the size" for some TAs, as described in
>> GlobalPlatform specification.
>
> Does it mean a guest TA may potentially access address 0?

TA is running in S-EL0. That buffer with PA=0x0 will be not mapped in TA
address space at all. So, if TA will try to access address 0, it
will be terminated with data abort.

> I am asking that because 0 can be a valid host physical address. We
> are currently carving out 0 from the heap allocator to workaround a
> bug, but there is no promise this address will be used by the boot
> allocator and therefore Xen.
>

Well, this is a potential issue in OP-TEE. It does not treat 0 as a
valid address. So, in that rare case, when REE will try to use 0
as a correct address for data buffer, OP-TEE will not map it into
TA address space, instead it will pass NULL to TA, so TA will think that
no buffer was provided.

>> Reason why I prohibited buffers without OPTEE_MSG_ATTR_NONCONTIG flag in
>> the the original patch is that such buffers could span across page
>> boundary, which is no-go for fragmented p2m.
>>
>> But I missed that special case: null buffer without
>> OPTEE_MSG_ATTR_NONCONTIG flag. As this is a special type of buffer, it
>> can be allowed with and without said flag.
>
> Looking at translate_noncontig(), there is no specific treatment for
> NULL. So the address will get translated. This is likely to result to
> an error as usually a guest doesn't have anything mapped at address 0.

Yes, you are right. Right now, optee client driver will not emit buffer
wit OPTEE_MSG_ATTR_NONCONTIG and address 0. But this is possible. And
this should be handled correctly. I'll add fix for that. Today, as
well. Thanks for pointing this out.

-- 
Volodymyr Babchuk at EPAM