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

Re: [Xen-devel] [PATCH v4 03/10] xen/arm: optee: add OP-TEE mediator skeleton



Hi,

On 07/03/2019 21:04, Volodymyr Babchuk wrote:
From: Volodymyr Babchuk <vlad.babchuk@xxxxxxxxx>

Add very basic OP-TEE mediator. It can probe for OP-TEE presence,
tell it about domain creation/destruction and forward all known
calls.

This is all what is needed for Dom0 to work with OP-TEE as long as
Dom0 shares 1:1 mapped pages with OP-TEE. Any attempt to call OP-TEE
from DomU will fail and will lead to random memory corruption. But
this is impossible, because there is no means to enable TEE support
for DomUs right now. Also, problems can arise if Dom0 uses pages
mapped from other domains. This will be fixed in the following
patches, as more of the mediator functionality will be added.

This code issues two non-preemptible calls to OP-TEE: to create and
to destroy client context. They can't block in OP-TEE, as they are
considered "fast calls" in terms of ARM SMCCC.

Signed-off-by: Volodymyr Babchuk <vlad.babchuk@xxxxxxxxx>

---

  All the patches to optee.c should be merged together. They were
  split to ease up review. But they depend heavily on each other.

Changes from v3:
   - Introduced optee_relinquish_resources() function to free
     mediator resources in a more controllable way

Changes from v2:
   - Fixed coding style
   - Introduced tee/Kconfig
   - Fixed error messages
---
  xen/arch/arm/Kconfig         |   2 +
  xen/arch/arm/tee/Kconfig     |   4 +
  xen/arch/arm/tee/Makefile    |   1 +
  xen/arch/arm/tee/optee.c     | 170 +++++++++++++++++++++++++++++++++++
  xen/include/asm-arm/domain.h |   3 +
  5 files changed, 180 insertions(+)
  create mode 100644 xen/arch/arm/tee/Kconfig
  create mode 100644 xen/arch/arm/tee/optee.c

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index e527b2f885..99e6f0ebb2 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -237,3 +237,5 @@ source "arch/arm/platforms/Kconfig"
  source "common/Kconfig"
source "drivers/Kconfig"
+
+source "arch/arm/tee/Kconfig"
diff --git a/xen/arch/arm/tee/Kconfig b/xen/arch/arm/tee/Kconfig
new file mode 100644
index 0000000000..5b829db2e9
--- /dev/null
+++ b/xen/arch/arm/tee/Kconfig
@@ -0,0 +1,4 @@
+config OPTEE
+       bool "Enable OP-TEE mediator"
+       default n
+       depends on TEE
diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile
index c54d4796ff..982c879684 100644
--- a/xen/arch/arm/tee/Makefile
+++ b/xen/arch/arm/tee/Makefile
@@ -1 +1,2 @@
  obj-y += tee.o
+obj-$(CONFIG_OPTEE) += optee.o
diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
new file mode 100644
index 0000000000..b50c581aaf
--- /dev/null
+++ b/xen/arch/arm/tee/optee.c
@@ -0,0 +1,170 @@
+/*
+ * xen/arch/arm/tee/optee.c
+ *
+ * OP-TEE mediator
+ *
+ * Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
+ * Copyright (c) 2018 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.

I think you are missing parts of the GPLv2 here:

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/device_tree.h>
+#include <xen/sched.h>
+
+#include <asm/smccc.h>
+#include <asm/tee/tee.h>
+#include <asm/tee/optee_msg.h>
+#include <asm/tee/optee_smc.h>
+
+#define OPTEE_ENABLED ((void*)0x1)

Please don't do that. Introduce a dummy structure instead and fill it up when needed.

+
+/* Client ID 0 is reserved for hypervisor itself */

s/for/for the/

+#define OPTEE_CLIENT_ID(domain) ((domain)->domain_id + 1)
+
+static bool optee_probe(void)
+{
+    struct dt_device_node *node;
+    struct arm_smccc_res resp;
+
+    /* Check for entry in dtb */
+    node = dt_find_compatible_node(NULL, NULL, "linaro,optee-tz");
+    if ( !node )
+        return false;
+
+    /* Check UID */
+    arm_smccc_smc(ARM_SMCCC_CALL_UID_FID(TRUSTED_OS_END), &resp);
+
+    if ( (uint32_t)resp.a0 != OPTEE_MSG_UID_0 ||
+         (uint32_t)resp.a1 != OPTEE_MSG_UID_1 ||
+         (uint32_t)resp.a2 != OPTEE_MSG_UID_2 ||
+         (uint32_t)resp.a3 != OPTEE_MSG_UID_3 )
+        return false;
+
+    return true;
+}
+
+static int optee_domain_init(struct domain *d)
+{
+    struct arm_smccc_res resp;
+
+    /*
+     * Inform OP-TEE about a new guest.
+     * This is a "Fast" call in terms of OP-TEE. This basically
+     * means that it can't be preempted, because there is no
+     * thread allocated for it in OP-TEE. It is close to atomic
+     * context in linux kernel: E.g. no blocking calls can be issued.

This does not really make sense to describe Linux here. Can't you just make the wording OS agnostic?

+     * Also, interrupts are disabled.
+     *
+     * a7 should be 0, so we can't skip last 6 parameters of arm_smccc_smc()
+     */
+    arm_smccc_smc(OPTEE_SMC_VM_CREATED, OPTEE_CLIENT_ID(d), 0, 0, 0, 0, 0, 0,
+                  &resp);
+    if ( resp.a0 != OPTEE_SMC_RETURN_OK )
+    {
+        gprintk(XENLOG_WARNING, "Unable to create OPTEE client: rc = 0x%X\n",

gprintk will print the current vCPU and not the domain created. This is not very useful to know the current domain. So it would be better to use:

printk(XENLOG_G_WARNING, "%pd: Unable to create OPTEE client: rc ...", d);

+                (uint32_t)resp.a0);
+
+        return -ENODEV;
+    }
+
+    d->arch.tee = OPTEE_ENABLED;
+
+    return 0;
+}
+
+static void forward_call(struct cpu_user_regs *regs)
+{
+    struct arm_smccc_res resp;
+
+    arm_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),
+                  OPTEE_CLIENT_ID(current->domain),
+                  &resp);
+
+    set_user_reg(regs, 0, resp.a0);
+    set_user_reg(regs, 1, resp.a1);
+    set_user_reg(regs, 2, resp.a2);
+    set_user_reg(regs, 3, resp.a3);
+    set_user_reg(regs, 4, 0);
+    set_user_reg(regs, 5, 0);
+    set_user_reg(regs, 6, 0);
+    set_user_reg(regs, 7, 0);
+}
+
+static int optee_relinquish_resources(struct domain *d)
+{
+    return 0;
+}
+
+static void optee_domain_destroy(struct domain *d)
+{
+    struct arm_smccc_res resp;
+
+    if ( !d->arch.tee )
+        return;
+
+    /*
+     * Inform OP-TEE that domain is shutting down. This is
+     * also a fast SMC call, like OPTEE_SMC_VM_CREATED, so
+     * it is also non-preemptible.
+     * At this time all domain VCPUs should be stopped. OP-TEE
+     * relies on this.
+     *
+     * a7 should be 0, so we can't skip last 6 parameters od arm_smccc_smc()

NIT: s/od/of/

+     */
+    arm_smccc_smc(OPTEE_SMC_VM_DESTROYED, OPTEE_CLIENT_ID(d), 0, 0, 0, 0, 0, 0,
+                  &resp);

Your split between domain_destroy and relinquish_resources looks wrong. If you relinquish resources before telling OP-TEE then you are at risk that OP-TEE will use those resources.

Instead you should first tell OP-TEE the domain is shutting down, then release the resources.

+}
+
+static bool optee_handle_call(struct cpu_user_regs *regs)
+{
+    if ( !current->domain->arch.tee )
+        return false;
+
+    switch ( get_user_reg(regs, 0) )
+    {
+    case OPTEE_SMC_CALLS_COUNT:
+    case OPTEE_SMC_CALLS_UID:
+    case OPTEE_SMC_CALLS_REVISION:
+    case OPTEE_SMC_CALL_GET_OS_UUID:
+    case OPTEE_SMC_FUNCID_GET_OS_REVISION:
+    case OPTEE_SMC_ENABLE_SHM_CACHE:
+    case OPTEE_SMC_DISABLE_SHM_CACHE:
+    case OPTEE_SMC_GET_SHM_CONFIG:
+    case OPTEE_SMC_EXCHANGE_CAPABILITIES:
+    case OPTEE_SMC_CALL_WITH_ARG:
+    case OPTEE_SMC_CALL_RETURN_FROM_RPC:
+        forward_call(regs);
+        return true;
+    default:
+        return false;
+    }
+}
+
+static const struct tee_mediator_ops optee_ops =
+{
+    .probe = optee_probe,
+    .domain_init = optee_domain_init,
+    .domain_destroy = optee_domain_destroy,
+    .relinquish_resources = optee_relinquish_resources,
+    .handle_call = optee_handle_call,
+};
+
+REGISTER_TEE_MEDIATOR(optee, "OP-TEE", &optee_ops);
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 0f15372098..2960a53e69 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -98,6 +98,9 @@ struct arch_domain
      struct vpl011 vpl011;
  #endif
+#ifdef CONFIG_TEE
+    void *tee;
+#endif
  }  __cacheline_aligned;
struct arch_vcpu


Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

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