[PATCH v2 2/2] optee: allow plain TMEM buffers with NULL address

Julien Grall julien at xen.org
Wed Jul 1 10:03:12 UTC 2020



On 29/06/2020 08:42, Paul Durrant wrote:
>> -----Original Message-----
>> From: Julien Grall <julien at xen.org>
>> Sent: 26 June 2020 18:54
>> To: Stefano Stabellini <sstabellini at kernel.org>; paul at xen.org
>> Cc: Volodymyr Babchuk <Volodymyr_Babchuk at epam.com>; xen-devel at lists.xenproject.org; op-
>> tee at lists.trustedfirmware.org
>> Subject: Re: [PATCH v2 2/2] optee: allow plain TMEM buffers with NULL address
>>
>> (using paul xen.org's email)
>>
> 
> Thanks. Avoids annoying warning banners :-)
> 
>> Hi,
>>
>> Apologies for the late answer.
>>
>> On 23/06/2020 22:09, Stefano Stabellini wrote:
>>> On Tue, 23 Jun 2020, Julien Grall wrote:
>>>> On 23/06/2020 03:49, Volodymyr Babchuk wrote:
>>>>>
>>>>> Hi Stefano,
>>>>>
>>>>> Stefano Stabellini writes:
>>>>>
>>>>>> On Fri, 19 Jun 2020, 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
>>>>>>> reference with the required size and returns it back to the
>>>>>>> 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.
>>>>>>>
>>>>>>> OP-TEE represents this null memory reference as a TMEM parameter with
>>>>>>> buf_ptr = 0x0. This is the only case when we should allow TMEM
>>>>>>> buffer without the OPTEE_MSG_ATTR_NONCONTIG flag. This also the
>>>>>>> special case for a buffer with OPTEE_MSG_ATTR_NONCONTIG flag.
>>>>>>>
>>>>>>> This could lead to a potential issue, because IPA 0x0 is a valid
>>>>>>> address, but OP-TEE will treat it as a special case. So, care should
>>>>>>> be taken when construction OP-TEE enabled guest to make sure that such
>>>>>>> guest have no memory at IPA 0x0 and none of its memory is mapped at PA
>>>>>>> 0x0.
>>>>>>>
>>>>>>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk at epam.com>
>>>>>>> ---
>>>>>>>
>>>>>>> Changes from v1:
>>>>>>>     - Added comment with TODO about possible PA/IPA 0x0 issue
>>>>>>>     - The same is described in the commit message
>>>>>>>     - Added check in translate_noncontig() for the NULL ptr buffer
>>>>>>>
>>>>>>> ---
>>>>>>>     xen/arch/arm/tee/optee.c | 27 ++++++++++++++++++++++++---
>>>>>>>     1 file changed, 24 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
>>>>>>> index 6963238056..70bfef7e5f 100644
>>>>>>> --- a/xen/arch/arm/tee/optee.c
>>>>>>> +++ b/xen/arch/arm/tee/optee.c
>>>>>>> @@ -215,6 +215,15 @@ static bool optee_probe(void)
>>>>>>>         return true;
>>>>>>>     }
>>>>>>>     +/*
>>>>>>> + * TODO: There is a potential issue with guests that either have RAM
>>>>>>> + * at IPA of 0x0 or some of theirs memory is mapped at PA 0x0. This is
>>>>>>                                   ^ their
>>>>>>
>>>>>>> + * because PA of 0x0 is considered as NULL pointer by OP-TEE. It will
>>>>>>> + * not be able to map buffer with such pointer to TA address space, or
>>>>>>> + * use such buffer for communication with the guest. We either need to
>>>>>>> + * check that guest have no such mappings or ensure that OP-TEE
>>>>>>> + * enabled guest will not be created with such mappings.
>>>>>>> + */
>>>>>>>     static int optee_domain_init(struct domain *d)
>>>>>>>     {
>>>>>>>         struct arm_smccc_res resp;
>>>>>>> @@ -725,6 +734,15 @@ static int translate_noncontig(struct optee_domain
>>>>>>> *ctx,
>>>>>>>             uint64_t next_page_data;
>>>>>>>         } *guest_data, *xen_data;
>>>>>>>     +    /*
>>>>>>> +     * Special case: buffer with buf_ptr == 0x0 is considered as NULL
>>>>>>> +     * pointer by OP-TEE. No translation is needed. This can lead to
>>>>>>> +     * an issue as IPA 0x0 is a valid address for Xen. See the comment
>>>>>>> +     * near optee_domain_init()
>>>>>>> +     */
>>>>>>> +    if ( !param->u.tmem.buf_ptr )
>>>>>>> +        return 0;
>>>>>>
>>>>>> Given that today it is not possible for this to happen, it could even be
>>>>>> an ASSERT. But I think I would just return an error, maybe -EINVAL?
>>>>>
>>>>> Hmm, looks like my comment is somewhat misleading :(
>>>>
>>>> How about the following comment:
>>>>
>>>> We don't want to translate NULL (0) as it can be used by the guest to fetch
>>>> the size of the buffer to allocate. This behavior depends on TA, but there is
>>>> a guarantee that OP-TEE will not try to map it (see more details on top of
>>>> optee_domain_init()).
>>>>
>>>>>
>>>>> What I mean, is that param->u.tmem.buf_ptr == 0 is the normal situation.
>>>>> This is the special case, when OP-TEE treats this buffer as a NULL. So
>>>>> we are doing nothing there. Thus, "return 0".
>>>>>
>>>>> But, as Julien pointed out, we can have machine where 0x0 is the valid
>>>>> memory address and there is a chance, that some guest will use it as a
>>>>> pointer to buffer.
>>>>>
>>>>>> Aside from this, and the small grammar issue, everything else looks fine
>>>>>> to me.
>>>>>>
>>>>>> Let's wait for Julien's reply, but if this is the only thing I could fix
>>>>>> on commit.
>>>>
>>>> I agree with Volodymyr, this is the normal case here. There are more work to
>>>> prevent MFN 0 to be mapped in the guest but this shouldn't be an issue today.
>>>
>>> Let's put the MFN 0 issue aside for a moment.
>>>
>>>   From the commit message I thought that if the guest wanted to pass a
>>> NULL buffer ("Null memory reference") then it would also *not* set
>>> OPTEE_MSG_ATTR_NONCONTIG, which would be handled by the "else" statement
>>> also modified by this patch. Thus, I thought that reaching
>>> translate_noncontig with buf_ptr == NULL would always be an error.
>>>
>>> But re-reading the commit message and from both your answers it is not
>>> the case: a "Null memory reference" is allowed with
>>> OPTEE_MSG_ATTR_NONCONTIG set too.
>>>
>>> Thus, I have no further comments and the improvements on the in-code
>>> comment could be done on commit.
>>
>> Good :). IIRC Paul gave a provisional RaB for this series. @Paul, now
>> that we are settled, could we get a formal one?
> 
> Sure.
> 
> Release-acked-by: Paul Durrant <paul at xen.org>

Thanks!

It is not clear to me what Stefano had in mind for the "in-code 
comment". So I will leave him committing the series.

Cheers,

-- 
Julien Grall


More information about the OP-TEE mailing list