On 29/06/2020 08:42, Paul Durrant wrote:
-----Original Message----- From: Julien Grall julien@xen.org Sent: 26 June 2020 18:54 To: Stefano Stabellini sstabellini@kernel.org; paul@xen.org Cc: Volodymyr Babchuk Volodymyr_Babchuk@epam.com; xen-devel@lists.xenproject.org; op- tee@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@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@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,