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

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

[Volodymyr Babchuk]

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 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 == NULL. This is the only case when we should allow TMEM
buffer without the OPTEE_MSG_ATTR_NONCONTIG flag.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 xen/arch/arm/tee/optee.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
index af19fc31f8..fb7d491b25 100644
--- a/xen/arch/arm/tee/optee.c
+++ b/xen/arch/arm/tee/optee.c
@@ -865,9 +865,12 @@ static int translate_params(struct optee_domain
*ctx,
             }
             else
             {
-                gdprintk(XENLOG_WARNING, "Guest tries to use old tmem
arg\n");
-                ret = -EINVAL;
-                goto out;
+                if ( call->xen_arg->params[i].u.tmem.buf_ptr )
+                {
+                    gdprintk(XENLOG_WARNING, "Guest tries to use old
tmem arg\n");
+                    ret = -EINVAL;
+                    goto out;
+                }
             }
             break;
         case OPTEE_MSG_ATTR_TYPE_NONE:

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

[Julien Grall]

(+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.

> 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?

> 
> 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?

> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
The code looks to match your commit message, but I wasn't able to match 
it with the spec. Do you have other pointer I could use to check the 
behavior?

I assume this wants to be part of Xen 4.14. The change is only for 
OP-TEE which is a tech preview feature. So the risk is very limited.

Cheers,

-- 
Julien Grall

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

[Stefano Stabellini]

Hi Paul, Julien, Volodymyr,

This is another bug fix that I would like to get in 4.14. It doesn't
look like we need any changes to this patch, assuming that it is
accurate to the spec.

It would be nice if Volodymyr could provide more info on the spec side,
but honestly I trust his experience on this.

Paul, are you OK with this going into 4.14?



On Sat, 6 Jun 2020, Julien Grall wrote:
> (+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.
> 
> > 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?
> 
> > 
> > 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?
> 
> > 
> > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> The code looks to match your commit message, but I wasn't able to match it
> with the spec. Do you have other pointer I could use to check the behavior?
> 
> I assume this wants to be part of Xen 4.14. The change is only for OP-TEE
> which is a tech preview feature. So the risk is very limited.

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

[Volodymyr Babchuk]

Hi Stefano,

Stefano Stabellini writes:

> Hi Paul, Julien, Volodymyr,
>
> This is another bug fix that I would like to get in 4.14. It doesn't
> look like we need any changes to this patch, assuming that it is
> accurate to the spec.
>
> It would be nice if Volodymyr could provide more info on the spec side,
> but honestly I trust his experience on this.

I'm sorry for the delay. Julien asks very good and deep questions and
it sometimes requires deep research, to fully and correctly answer to
them. This was exactly that case. I'll answer timely to the next e-mails.

-- 
Volodymyr Babchuk at EPAM

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

[Paul Durrant]

> -----Original Message-----
> From: Stefano Stabellini <sstabellini@kernel.org>
> Sent: 18 June 2020 23:27
> To: xadimgnik@gmail.com; paul@xen.org; pdurrant@amazon.com
> Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; sstabellini@kernel.org; xen-
> devel@lists.xenproject.org; paul@xen.org; julien@xen.org
> Subject: Re: [PATCH] xen/arm: optee: allow plain TMEM buffers with NULL address
> 
> Hi Paul, Julien, Volodymyr,
> 
> This is another bug fix that I would like to get in 4.14. It doesn't
> look like we need any changes to this patch, assuming that it is
> accurate to the spec.
> 
> It would be nice if Volodymyr could provide more info on the spec side,
> but honestly I trust his experience on this.
> 
> Paul, are you OK with this going into 4.14?
> 

In principle, yes. It appears, from Julien's comments though, that the commit message may need a little expansion (for the benefit
of those mining this code in future).

  Paul

> 
> 
> On Sat, 6 Jun 2020, Julien Grall wrote:
> > (+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.
> >
> > > 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?
> >
> > >
> > > 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?
> >
> > >
> > > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> > The code looks to match your commit message, but I wasn't able to match it
> > with the spec. Do you have other pointer I could use to check the behavior?
> >
> > I assume this wants to be part of Xen 4.14. The change is only for OP-TEE
> > which is a tech preview feature. So the risk is very limited.

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

[Volodymyr Babchuk]

Hi Julien,

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.

>>
>> 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.

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.

>
>>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> The code looks to match your commit message, but I wasn't able to
> match it with the spec. Do you have other pointer I could use to check
> the behavior?
>
> I assume this wants to be part of Xen 4.14. The change is only for
> OP-TEE which is a tech preview feature. So the risk is very limited.

Sure.

-- 
Volodymyr Babchuk at EPAM

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

[Julien Grall]

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?

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.

> 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.

Did I miss anything?

Cheers,

-- 
Julien Grall

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

[Volodymyr Babchuk]

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

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

[Julien Grall]

Hi Volodymyr,

On 19/06/2020 10:52, Volodymyr Babchuk wrote:
>>>>> 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.

Thanks! That's reassuring. Although, I think we may still need to 
prevent MFN 0 to be allocated for a guest using OP-TEE. So they don't 
end up with weird failure.

I don't think this is an issue so far, but this may change with 
Stefano's dom0less series to support direct mapping.

Cheers,

-- 
Julien Grall

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

[Stefano Stabellini]

On Fri, 19 Jun 2020, Julien Grall wrote:
> Hi Volodymyr,
> 
> On 19/06/2020 10:52, Volodymyr Babchuk wrote:
> > > > > > 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.
> 
> Thanks! That's reassuring. Although, I think we may still need to prevent MFN
> 0 to be allocated for a guest using OP-TEE. So they don't end up with weird
> failure.
> 
> I don't think this is an issue so far, but this may change with Stefano's
> dom0less series to support direct mapping.

Yes, it is interesting to know about this limitation.

In regards to this patch, it looks OK as-is in terms of code changes.
Aside from a link to this conversation in the xen-devel archives, is
there anything else you would like to add to the commit message?

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

[Julien Grall]

On 19/06/2020 18:15, Stefano Stabellini wrote:
> On Fri, 19 Jun 2020, Julien Grall wrote:
>> Hi Volodymyr,
>>
>> On 19/06/2020 10:52, Volodymyr Babchuk wrote:
>>>>>>> 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.
>>
>> Thanks! That's reassuring. Although, I think we may still need to prevent MFN
>> 0 to be allocated for a guest using OP-TEE. So they don't end up with weird
>> failure.
>>
>> I don't think this is an issue so far, but this may change with Stefano's
>> dom0less series to support direct mapping.
> 
> Yes, it is interesting to know about this limitation.
> 
> In regards to this patch, it looks OK as-is in terms of code changes.

I would disagree here. NULL has never been handled correctly for TMEM 
buffers (see [1]). I would argue this needs to be folded within this 
patch rather than be a separate one.

> Aside from a link to this conversation in the xen-devel archives, is
> there anything else you would like to add to the commit message?
+1 for the link. However, I don't think the commit message fully 
match/summarize the discussion here.

What needs to be clearly spell out is that existing OP-TEEs will never 
map MFN 0.

Cheers,

[1]  <87h7v71ixf.fsf@epam.com>

-- 
Julien Grall

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

[Stefano Stabellini]

On Fri, 19 Jun 2020, Julien Grall wrote:
> On 19/06/2020 18:15, Stefano Stabellini wrote:
> > On Fri, 19 Jun 2020, Julien Grall wrote:
> > > Hi Volodymyr,
> > > 
> > > On 19/06/2020 10:52, Volodymyr Babchuk wrote:
> > > > > > > > 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.
> > > 
> > > Thanks! That's reassuring. Although, I think we may still need to prevent
> > > MFN
> > > 0 to be allocated for a guest using OP-TEE. So they don't end up with
> > > weird
> > > failure.
> > > 
> > > I don't think this is an issue so far, but this may change with Stefano's
> > > dom0less series to support direct mapping.
> > 
> > Yes, it is interesting to know about this limitation.
> > 
> > In regards to this patch, it looks OK as-is in terms of code changes.
> 
> I would disagree here. NULL has never been handled correctly for TMEM buffers
> (see [1]). I would argue this needs to be folded within this patch rather than
> be a separate one.

I am OK with that too.


> > Aside from a link to this conversation in the xen-devel archives, is
> > there anything else you would like to add to the commit message?
> +1 for the link. However, I don't think the commit message fully
> match/summarize the discussion here.
> 
> What needs to be clearly spell out is that existing OP-TEEs will never map MFN
> 0.

That would be nice


> Cheers,
> 
> [1]  <87h7v71ixf.fsf@epam.com>