[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [RFC] WIP: optee: add OP-TEE mediator
On 04/12/17 16:24, Volodymyr Babchuk wrote:
On Mon, Dec 04, 2017 at 02:30:32PM +0000, Julien Grall wrote:
On 01/12/17 22:58, Stefano Stabellini wrote:
On Mon, 27 Nov 2017, Volodymyr Babchuk wrote:
= Page pinning =
Guest pages passed to OP-TEE need to be pinned (otherwise Xen doesn't
guarantee they'll be available). I think the right function to call for
that is get_page_from_gfn or get_page.
No direct use of get_page please. We already have plenty of helper to deal
with the translation GFN -> Page or even copying data. I don't want to see
more open-coding version because it makes difficult to interaction with
other features such as memaccess and PoD.
Okay. Could you please suggest what API should be used in my case?
Please read my previous e-mail until the end. I provided suggestions how
to handle pinning...
---
Add OP-TEE mediator as an example how TEE mediator framework
works.
OP-TEE mediator support address translation for DomUs.
It tracks execution of STD calls, correctly handles memory-related RPC
requests, tracks buffer allocated for RPCs.
With this patch OP-TEE successfully passes own tests, while client is
running in DomU. Currently it lacks some code for exceptional cases,
because this patch was used mostly to debug virtualization in OP-TEE.
Nevertheless, it provides all features needed for OP-TEE mediation.
WARNING: This is a development patch, it does not cover all corner
cases, so, please don't use it in production.
It was tested on RCAR Salvator-M3 board.
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
---
xen/arch/arm/tee/Kconfig | 4 +
xen/arch/arm/tee/Makefile | 1 +
xen/arch/arm/tee/optee.c | 765 +++++++++++++++++++++++++++++++++++++++++++
xen/arch/arm/tee/optee_smc.h | 50 +++
4 files changed, 820 insertions(+)
create mode 100644 xen/arch/arm/tee/optee.c
diff --git a/xen/arch/arm/tee/Kconfig b/xen/arch/arm/tee/Kconfig
index e69de29..7c6b5c6 100644
--- a/xen/arch/arm/tee/Kconfig
+++ b/xen/arch/arm/tee/Kconfig
@@ -0,0 +1,4 @@
+config ARM_OPTEE
+ bool "Enable OP-TEE mediator"
+ default n
+ depends on ARM_TEE
diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile
index c54d479..9d93b42 100644
--- a/xen/arch/arm/tee/Makefile
+++ b/xen/arch/arm/tee/Makefile
@@ -1 +1,2 @@
obj-y += tee.o
+obj-$(CONFIG_ARM_OPTEE) += optee.o
diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
new file mode 100644
index 0000000..59c3600
--- /dev/null
+++ b/xen/arch/arm/tee/optee.c
@@ -0,0 +1,765 @@
+/*
+ * xen/arch/arm/tee/optee.c
+ *
+ * OP-TEE mediator
+ *
+ * Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
+ * Copyright (c) 2017 EPAM Systems.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <xen/domain_page.h>
+#include <xen/types.h>
+#include <xen/sched.h>
+
+#include <asm/p2m.h>
+#include <asm/tee.h>
+
+#include "optee_msg.h"
+#include "optee_smc.h"
+
+/*
+ * Global TODO:
+ * 1. Create per-domain context, where call and shm will be stored
+ * 2. Pin pages shared between OP-TEE and guest
+ */
+/*
+ * OP-TEE violates SMCCC when it defines own UID. So we need
+ * to place bytes in correct order.
+ */
+#define OPTEE_UID (xen_uuid_t){{
\
+ (uint8_t)(OPTEE_MSG_UID_0 >> 0), (uint8_t)(OPTEE_MSG_UID_0 >> 8),
\
+ (uint8_t)(OPTEE_MSG_UID_0 >> 16), (uint8_t)(OPTEE_MSG_UID_0 >> 24),
\
+ (uint8_t)(OPTEE_MSG_UID_1 >> 0), (uint8_t)(OPTEE_MSG_UID_1 >> 8),
\
+ (uint8_t)(OPTEE_MSG_UID_1 >> 16), (uint8_t)(OPTEE_MSG_UID_1 >> 24),
\
+ (uint8_t)(OPTEE_MSG_UID_2 >> 0), (uint8_t)(OPTEE_MSG_UID_2 >> 8),
\
+ (uint8_t)(OPTEE_MSG_UID_2 >> 16), (uint8_t)(OPTEE_MSG_UID_2 >> 24),
\
+ (uint8_t)(OPTEE_MSG_UID_3 >> 0), (uint8_t)(OPTEE_MSG_UID_3 >> 8),
\
+ (uint8_t)(OPTEE_MSG_UID_3 >> 16), (uint8_t)(OPTEE_MSG_UID_3 >> 24),
\
+ }}
+
+#define MAX_NONCONTIG_ENTRIES 8
+
+/*
+ * Call context. OP-TEE can issue mulitple RPC returns during one call.
+ * We need to preserve context during them.
+ */
+struct std_call_ctx {
+ struct list_head list;
+ struct optee_msg_arg *guest_arg;
+ struct optee_msg_arg *xen_arg;
+ void *non_contig[MAX_NONCONTIG_ENTRIES];
+ int non_contig_order[MAX_NONCONTIG_ENTRIES];
+ int optee_thread_id;
+ int rpc_op;
+ domid_t domid;
+};
+static LIST_HEAD(call_ctx_list);
+static DEFINE_SPINLOCK(call_ctx_list_lock);
+
+/*
+ * Command buffer shared between OP-TEE and guest.
+ * Warning! In the proper implementation this SHM buffer *probably* should
+ * by shadowed by XEN.
+ * TODO: Reconsider this.
+ */
+struct shm {
+ struct list_head list;
+ struct optee_msg_arg *guest_arg;
+ struct page *guest_page;
+ mfn_t guest_mfn;
+ uint64_t cookie;
+ domid_t domid;
+};
+
+static LIST_HEAD(shm_list);
+static DEFINE_SPINLOCK(shm_list_lock);
+
+static int optee_init(void)
+{
+ printk("OP-TEE mediator init done\n");
+ return 0;
+}
+
+static void optee_domain_create(struct domain *d)
+{
+ register_t resp[4];
+ call_smccc_smc(OPTEE_SMC_VM_CREATED,
+ d->domain_id + 1, 0, 0, 0, 0, 0, 0, resp);
+ if ( resp[0] != OPTEE_SMC_RETURN_OK )
+ gprintk(XENLOG_WARNING, "OP-TEE don't want to support domain: %d\n",
+ (uint32_t)resp[0]);
+ /* TODO: Change function declaration to be able to retun error */
+}
+
+static void optee_domain_destroy(struct domain *d)
+{
+ register_t resp[4];
+ call_smccc_smc(OPTEE_SMC_VM_DESTROYED,
+ d->domain_id + 1, 0, 0, 0, 0, 0, 0, resp);
+ /* TODO: Clean call contexts and SHMs associated with domain */
+}
+
+static bool forward_call(struct cpu_user_regs *regs)
+{
+ register_t resp[4];
+
+ /* TODO: Use separate registers set to prevent leakage to guest */
+ call_smccc_smc(get_user_reg(regs, 0),
+ get_user_reg(regs, 1),
+ get_user_reg(regs, 2),
+ get_user_reg(regs, 3),
+ get_user_reg(regs, 4),
+ get_user_reg(regs, 5),
+ get_user_reg(regs, 6),
+ /* VM id 0 is reserved for hypervisor itself */
+ current->domain->domain_id + 1,
This doesn't look like it would wrap around safely.
Well ordinary domain will always have a domain ID < DOMID_FIRST_RESERVER
(0x7FF0). So there are no issue here. Although, we may want a BUILD_BUG_ON()
to catch change of DOMID_FIRST_RESERVED.
+ resp);
+
+ set_user_reg(regs, 0, resp[0]);
+ set_user_reg(regs, 1, resp[1]);
+ set_user_reg(regs, 2, resp[2]);
+ set_user_reg(regs, 3, resp[3]);
+
+ return true;
+}
+
+static struct std_call_ctx *allocate_std_call_ctx(void)
+{
+ struct std_call_ctx *ret;
+
+ ret = xzalloc(struct std_call_ctx);
+ if ( !ret )
+ return NULL;
+
+ ret->optee_thread_id = -1;
You set it to -1. But no-one is checking that value. So what is the purpose
of setting to -1 and not 0?
+ ret->domid = -1;
Please use DOMID_INVALID rather than -1. You don't know whether the latter
will be used in the future for a domain.
+
+ spin_lock(&call_ctx_list_lock);
+ list_add_tail(&ret->list, &call_ctx_list);
+ spin_unlock(&call_ctx_list_lock);
+
+ return ret;
+}
+
+static void free_std_call_ctx(struct std_call_ctx *ctx)
+{
+ spin_lock(&call_ctx_list_lock);
+ list_del(&ctx->list);
+ spin_unlock(&call_ctx_list_lock);
+
+ if (ctx->xen_arg)
+ free_xenheap_page(ctx->xen_arg);
+
+ if (ctx->guest_arg)
+ unmap_domain_page(ctx->guest_arg);
+
+ for (int i = 0; i < MAX_NONCONTIG_ENTRIES; i++) {
+ if (ctx->non_contig[i])
+ free_xenheap_pages(ctx->non_contig[i], ctx->non_contig_order[i]);
+ }
+
+ xfree(ctx);
+}
+
+static struct std_call_ctx *find_ctx(int thread_id, domid_t domid)
+{
+ struct std_call_ctx *ctx;
+
+ spin_lock(&call_ctx_list_lock);
+ list_for_each_entry( ctx, &call_ctx_list, list )
+ {
+ if (ctx->domid == domid && ctx->optee_thread_id == thread_id )
+ {
+ spin_unlock(&call_ctx_list_lock);
+ return ctx;
+ }
+ }
+ spin_unlock(&call_ctx_list_lock);
+
+ return NULL;
+}
+
+#define PAGELIST_ENTRIES_PER_PAGE \
+ ((OPTEE_MSG_NONCONTIG_PAGE_SIZE / sizeof(u64)) - 1)
+
+static size_t get_pages_list_size(size_t num_entries)
+{
+ int pages = DIV_ROUND_UP(num_entries, PAGELIST_ENTRIES_PER_PAGE);
+
+ return pages * OPTEE_MSG_NONCONTIG_PAGE_SIZE;
+}
+
+static mfn_t lookup_guest_ram_addr(paddr_t gaddr)
+{
+ mfn_t mfn;
+ gfn_t gfn;
+ p2m_type_t t;
+ gfn = gaddr_to_gfn(gaddr);
+ mfn = p2m_lookup(current->domain, gfn, &t);
+ if ( t != p2m_ram_rw || mfn_eq(mfn, INVALID_MFN) ) {
+ gprintk(XENLOG_INFO, "Domain tries to use invalid gfn\n");
+ return INVALID_MFN;
+ }
+ return mfn;
+} >> +
+static struct shm *allocate_and_map_shm(paddr_t gaddr, uint64_t cookie)
+{
+ struct shm *ret;
+
+ ret = xzalloc(struct shm);
+ if ( !ret )
+ return NULL;
+
+ ret->guest_mfn = lookup_guest_ram_addr(gaddr);
+
+ if ( mfn_eq(ret->guest_mfn, INVALID_MFN) )
+ {
+ xfree(ret);
+ return NULL;
+ }
+
+ ret->guest_arg = map_domain_page(ret->guest_mfn);
map_domain_page() can never fail, but you use it the wrong way. The purpose
of this function is to map the memory for a very short lifetime, and only a
the current pCPU (when the all the RAM is not always mapped). Here, you seem
to use across SMC call (e.g for RPC).
Looking at the usage in the code, you only map it in order to copy the
arguments to/from the guest.
map_domain_page() will not take a reference on the page and prevent the page
to disappear from the guest. So this bits is unsafe.
For the arguments, the best is to use guest copy helpers (see
access_guest_memory_by_ipa). You might want to look at [1] as it improves
the use of access_guest_memory_by_ipa.
+ if ( !ret->guest_arg )
+ {
+ gprintk(XENLOG_INFO, "Could not map domain page\n");
+ xfree(ret);
+ return NULL;
+ }
+ ret->cookie = cookie;
+ ret->domid = current->domain->domain_id;
+
+ spin_lock(&shm_list_lock);
+ list_add_tail(&ret->list, &shm_list);
+ spin_unlock(&shm_list_lock);
+ return ret;
+}
+
+static void free_shm(uint64_t cookie, domid_t domid)
+{
+ struct shm *shm, *found = NULL;
+ spin_lock(&shm_list_lock);
+
+ list_for_each_entry( shm, &shm_list, list )
+ {
+ if (shm->domid == domid && shm->cookie == cookie )
+ {
+ found = shm;
+ list_del(&found->list);
+ break;
+ }
+ }
+ spin_unlock(&shm_list_lock);
+
+ if ( !found ) {
+ return;
+ }
+
+ if ( found->guest_arg )
+ unmap_domain_page(found->guest_arg);
+
+ xfree(found);
+}
+
+static struct shm *find_shm(uint64_t cookie, domid_t domid)
+{
+ struct shm *shm;
+
+ spin_lock(&shm_list_lock);
+ list_for_each_entry( shm, &shm_list, list )
+ {
+ if ( shm->domid == domid && shm->cookie == cookie )
+ {
+ spin_unlock(&shm_list_lock);
+ return shm;
+ }
+ }
+ spin_unlock(&shm_list_lock);
+
+ return NULL;
+}
+
+static bool translate_noncontig(struct std_call_ctx *ctx,
+ struct optee_msg_param *param,
+ int idx)
+{
+ /*
+ * Refer to OPTEE_MSG_ATTR_NONCONTIG description in optee_msg.h for
details.
+ *
+ * WARNING: This is test code. It works only with xen page size == 4096
That's a call for a BUILD_BUG_ON().
+ */
+ uint64_t size;
+ int page_offset;
+ int num_pages;
+ int order;
+ int entries_on_page = 0;
+ paddr_t gaddr;
+ mfn_t guest_mfn;
+ struct {
+ uint64_t pages_list[PAGELIST_ENTRIES_PER_PAGE];
+ uint64_t next_page_data;
+ } *pages_data_guest, *pages_data_xen, *pages_data_xen_start;
+
+ page_offset = param->u.tmem.buf_ptr & (OPTEE_MSG_NONCONTIG_PAGE_SIZE - 1);
+
+ size = ROUNDUP(param->u.tmem.size + page_offset,
+ OPTEE_MSG_NONCONTIG_PAGE_SIZE);
+
+ num_pages = DIV_ROUND_UP(size, OPTEE_MSG_NONCONTIG_PAGE_SIZE);
What is the limit for num_pages? I can't see anything in the code that
prevent any high number and might exhaust Xen memory.
+
+ order = get_order_from_bytes(get_pages_list_size(num_pages));
+
+ pages_data_xen_start = alloc_xenheap_pages(order, 0);
+ if (!pages_data_xen_start)
+ return false;
+
+ gaddr = param->u.tmem.buf_ptr & ~(OPTEE_MSG_NONCONTIG_PAGE_SIZE - 1);
+ guest_mfn = lookup_guest_ram_addr(gaddr);
+ if ( mfn_eq(guest_mfn, INVALID_MFN) )
+ goto err_free;
+
+ pages_data_guest = map_domain_page(guest_mfn);
Similarly here, you may want to use access_guest_by_ipa. This will do all
the safety check for copy from guest memory.
Furthermore, I think this is going to simplify a lot this code.
+ if (!pages_data_guest)
+ goto err_free;
+
+ pages_data_xen = pages_data_xen_start;
+ while ( num_pages ) {
+ mfn_t entry_mfn = lookup_guest_ram_addr(
+ pages_data_guest->pages_list[entries_on_page]);
+
+ if ( mfn_eq(entry_mfn, INVALID_MFN) )
+ goto err_unmap;
You would need to get a reference on each page, and release it in err_unmap
or when the command is done. get_page_from_gfn could do it for you.
+
+ pages_data_xen->pages_list[entries_on_page] = mfn_to_maddr(entry_mfn);
+ entries_on_page++;
+
+ if ( entries_on_page == PAGELIST_ENTRIES_PER_PAGE ) {
+ pages_data_xen->next_page_data = virt_to_maddr(pages_data_xen + 1);
+ pages_data_xen++;
+ gaddr = pages_data_guest->next_page_data;
+ unmap_domain_page(pages_data_guest);
+ guest_mfn = lookup_guest_ram_addr(gaddr);
+ if ( mfn_eq(guest_mfn, INVALID_MFN) )
+ goto err_free;
+
+ pages_data_guest = map_domain_page(guest_mfn);
+ if ( !pages_data_guest )
+ goto err_free;
+ /* Roll over to the next page */
+ entries_on_page = 0;
+ }
+ num_pages--;
+ }
+
+ unmap_domain_page(pages_data_guest);
+
+ param->u.tmem.buf_ptr = virt_to_maddr(pages_data_xen_start) | page_offset;
I am not sure to understand why you are apply the offset of the guest buffer
to xen buffer.
+
+ ctx->non_contig[idx] = pages_data_xen_start;
+ ctx->non_contig_order[idx] = order;
+
+ unmap_domain_page(pages_data_guest);
+ return true;
+
+err_unmap:
+ unmap_domain_page(pages_data_guest);
+err_free:
+ free_xenheap_pages(pages_data_xen_start, order);
+ return false;
+}
+
+static bool translate_params(struct std_call_ctx *ctx)
+{
+ unsigned int i;
+ uint32_t attr;
+
+ for ( i = 0; i < ctx->xen_arg->num_params; i++ ) {
+ attr = ctx->xen_arg->params[i].attr;
+
+ switch ( attr & OPTEE_MSG_ATTR_TYPE_MASK ) {
+ case OPTEE_MSG_ATTR_TYPE_TMEM_INPUT:
+ case OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT:
+ case OPTEE_MSG_ATTR_TYPE_TMEM_INOUT:
+ if ( attr & OPTEE_MSG_ATTR_NONCONTIG ) {
+ if ( !translate_noncontig(ctx, ctx->xen_arg->params + i, i) )
+ return false;
+ }
+ else {
+ gprintk(XENLOG_WARNING, "Guest tries to use old tmem arg\n");
+ return false;
+ }
+ break;
+ case OPTEE_MSG_ATTR_TYPE_NONE:
+ case OPTEE_MSG_ATTR_TYPE_VALUE_INPUT:
+ case OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT:
+ case OPTEE_MSG_ATTR_TYPE_VALUE_INOUT:
+ case OPTEE_MSG_ATTR_TYPE_RMEM_INPUT:
+ case OPTEE_MSG_ATTR_TYPE_RMEM_OUTPUT:
+ case OPTEE_MSG_ATTR_TYPE_RMEM_INOUT:
+ continue;
+ }
+ }
+ return true;
+}
+
+/*
+ * Copy command buffer into xen memory to:
+ * 1) Hide translated addresses from guest
+ * 2) Make sure that guest wouldn't change data in command buffer during call
+ */
+static bool copy_std_request(struct cpu_user_regs *regs,
+ struct std_call_ctx *ctx)
+{
+ paddr_t cmd_gaddr, xen_addr;
+ mfn_t cmd_mfn;
+
+ cmd_gaddr = (paddr_t)get_user_reg(regs, 1) << 32 |
+ get_user_reg(regs, 2);
+
+ /*
+ * Command buffer should start at page boundary.
+ * This is OP-TEE ABI requirement.
+ */
+ if ( cmd_gaddr & (OPTEE_MSG_NONCONTIG_PAGE_SIZE - 1) )
+ return false;
+
+ cmd_mfn = lookup_guest_ram_addr(cmd_gaddr);
+ if ( mfn_eq(cmd_mfn, INVALID_MFN) )
+ return false;
+
+ ctx->guest_arg = map_domain_page(cmd_mfn);
+ if ( !ctx->guest_arg )
+ return false;
+
+ ctx->xen_arg = alloc_xenheap_page();
+ if ( !ctx->xen_arg )
+ return false;
+
+ memcpy(ctx->xen_arg, ctx->guest_arg, OPTEE_MSG_NONCONTIG_PAGE_SIZE);
Have a look a guest copy helpers.
Cheers,
[1]
https://lists.xenproject.org/archives/html/xen-devel/2017-11/msg01481.html
--
Julien Grall
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
