[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [RFC 4/4] arm: tee: add basic OP-TEE mediator



Hi Volodymyr,

On 11/10/17 20:01, Volodymyr Babchuk wrote:
Add basic OP-TEE mediator as an example how TEE mediator framework
works.

Currently it support only calls from Dom0. Calls from other guests
will be declined. It maps OP-TEE static shared memory region into
Dom0 address space, so Dom0 is the only domain which can work with
older versions of OP-TEE.

Also it alters SMC requests by\ adding domain id to request. OP-TEE
can use this information to track requesters.

Albeit being in early development stages, this mediator already can
be used on systems where only Dom0 interacts with OP-TEE.

A link to the spec would be useful here to be able to fully review this patch.


It was tested on RCAR Salvator-M3 board.

Is it with the stock op-tee? Or an updated version?


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  | 178 ++++++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 183 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..0220691
--- /dev/null
+++ b/xen/arch/arm/tee/optee.c
@@ -0,0 +1,178 @@
+/*
+ * 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/types.h>
+#include <xen/sched.h>
+
+#include <asm/p2m.h>
+#include <asm/tee.h>
+
+#include "optee_msg.h"
+#include "optee_smc.h"
+
+/*
+ * OP-TEE violates SMCCC when it defines own UID. So we need
+ * to place bytes in correct order.

Can you please point the paragraph in the spec where it says that?

+ */
+#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),        
 \
+    }}
+
+static int optee_init(void)
+{
+    printk("OP-TEE mediator init done\n");
+    return 0;
+}
+
+static void optee_domain_create(struct domain *d)
+{
+    /*
+     * Do nothing at this time.
+     * In the future this function will notify that new VM is started.

You already have a new client with the hardware domain. So don't you already need to notifity OP-TEE?

+     */
+}
+
+static void optee_domain_destroy(struct domain *d)
+{
+    /*
+     * Do nothing at this time.
+     * In the future this function will notify that VM is being destroyed.
+     */

Same for the destruction?

+}
+
+static bool forward_call(struct cpu_user_regs *regs)
+{
+    register_t resp[4];
+
+    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 */

s/VM/client/. Also, on your design document you mentioned that you did modify OP-TEE to support multiple client ID. So how do you know whether the TEE supports client ID?

Similarly, do you expect OP-TEE to support 16-bit of client identifier?

+                   current->domain->domain_id +1,
+                   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]);

Who will do the sanity check of the return values? Each callers? If so, I would prefer that the results are stored in a temporary array and a separate helpers will write them into the domain once the sanity is done.

This would avoid to mistakenly expose unwanted data to a domain.

+
+    return true;
+}
+
+static bool handle_get_shm_config(struct cpu_user_regs *regs)
+{
+    paddr_t shm_start;
+    size_t shm_size;
+    int rc;
+
+    printk("handle_get_shm_config\n");

No plain printk in code accessible by the guest. You should use gprintk or ratelimit it.

+    /* Give all static SHM region to the Dom0 */

s/Dom0/Hardware Domain/

But I am not sure what's the point of this check given OP-TEE is only supported for the Hardware Domain and you already have a check for that.

+    if ( current->domain->domain_id != 0 )

Please use is_hardware_domain(current->domain) and not open-code the check.

+        return false;
+
+    forward_call(regs);
+
+    /* Return error back to the guest */
+    if ( get_user_reg(regs, 0) != OPTEE_SMC_RETURN_OK)
+        return true;

This is quite confusing to read, I think it would make sense that forward_call return the error.

+
+    shm_start = get_user_reg(regs, 1);
+    shm_size = get_user_reg(regs, 2);
+
+    /* Dom0 is mapped 1:1 */

Please don't make this assumption or at least add ASSERT(is_domain_direct_mapped(d));

+    rc = map_regions_p2mt(current->domain, gaddr_to_gfn(shm_start),

Rather than using current->domain everywhere, I would prefer if you introduce a temporary variable for the domain.

+                          shm_size / PAGE_SIZE,

Please PFN_DOWN(...).

+                          maddr_to_mfn(shm_start),
+                          p2m_ram_rw);

What is this shared memory for? I know this is the hardware domain, so using p2m_ram_rw would be ok. But I don't think this would be safe unless TEE do proper safety check.

+    if ( rc < 0 )
+    {
+        gprintk(XENLOG_INFO, "OP-TEE: Can't map static shm for Dom0: %d", rc);

gprintk already dump the domid. So no need to say Dom0.

+        set_user_reg(regs, 0, OPTEE_SMC_RETURN_ENOTAVAIL);
+    }
+
+    return true;
+}
+
+static bool handle_exchange_capabilities(struct cpu_user_regs *regs)
+{
+        forward_call(regs);
+
+        printk("handle_exchange_capabilities\n");

Same here, no plain prink.

+        /* Return error back to the guest */
+        if ( get_user_reg(regs, 0) != OPTEE_SMC_RETURN_OK)
+            return true;
+
+        /* Don't allow guests to work without dynamic SHM */

Hmmm? But you don't support guests today. So why are you checking that?

I would prefer if you either support guest or not. But not half of it as it is hard to know how this will end up.

+        if (current->domain->domain_id != 0 &&
+            !(get_user_reg(regs, 1) & OPTEE_SMC_SEC_CAP_DYNAMIC_SHM) )
+            set_user_reg(regs, 0, OPTEE_SMC_RETURN_ENOTAVAIL);

Missing newline.

+        return true;
+}
+
+static bool optee_handle_smc(struct cpu_user_regs *regs)
+{
+    /* At this time we support only calls from Dom0. */
+    if ( current->domain->domain_id != 0 )

is_hardware_domain(d)

+        return false;
+
+    switch ( get_user_reg(regs, 0) )
+    {
+    case OPTEE_SMC_GET_SHM_CONFIG:
+        return handle_get_shm_config(regs);
+    case OPTEE_SMC_EXCHANGE_CAPABILITIES:
+        return handle_exchange_capabilities(regs);
+    default:
+        return forward_call(regs);
+    }
+    return true;

The return here is not necessary.

+}
+
+static void optee_remove(void)
+{
+}
+
+static const struct tee_mediator_ops optee_ops =
+{
+    .init = optee_init,
+    .domain_create = optee_domain_create,
+    .domain_destroy = optee_domain_destroy,
+    .handle_smc = optee_handle_smc,
+    .remove = optee_remove,
+};
+
+REGISTER_TEE_MEDIATOR(optee, "OP-TEE", OPTEE_UID, &optee_ops);
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */


Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.