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

[Julien Grall <julien@xxxxxxx>]

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@xxxxxxxx>

--
Julien Grall