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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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