[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/2] optee: allow plain TMEM buffers with NULL address
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@xxxxxxxx>
---
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.
Cheers,
--
Julien Grall
|
