|
[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
On Mon, Oct 16, 2017 at 03:36:38PM +0100, Julien Grall wrote:
> Hi Volodymyr,
Hi Julien,
> 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.
Which spec? OP-TEE protocol? It was added in previous commit.
> >
> >It was tested on RCAR Salvator-M3 board.
>
> Is it with the stock op-tee? Or an updated version?
Static SHM was tested with stock OP-TEE. Dynamic SHM was tested with
my build. But my patches are already merged. OP-TEE 2.6.0 will support
dynamic SHM out of the box.
> >
> >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?
Sure.
> >+ */
> >+#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?
Because currently OP-TEE does not support such notification.
> >+ */
> >+}
> >+
> >+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?
The same answer. OP-TEE currently can work with only one domain. I selected
Dom0 for this.
> >+}
> >+
> >+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?
Hm, as I remember, I never mentioned that I modified OP-TEE to support
multiple client IDs. This is my current task.
But, answering your question, optee_domain_create() will tell OP-TEE about
new client ID.
> Similarly, do you expect OP-TEE to support 16-bit of client identifier?
Actually, yes. But I don't expect it to work with all 65535 domains at the
same time.
> >+ 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.
Maybe there will be cases when call will be forwarded straight to OP-TEE and
nobody in hypervisor will examine returned result. At least, at this moment
there are such cases. Probably, in full-scalle mediator this will no longer
be true.
> This would avoid to mistakenly expose unwanted data to a domain.
Correct me, but set_user_reg() modifies data that will be stored in general
purpose registers during return from trap handler. This can't expose any
additional 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.
Sorry, this is a debug print. I'll remove it at all.
> >+ /* Give all static SHM region to the Dom0 */
>
> s/Dom0/Hardware Domain/
Hm, looks like Dom0 != hardware domain. At least I see code that replaces
contents of hardware_domain variable. If it is possible, then there will
be a problem with static SHM buffer.
Looks like it is better to check for is_domain_direct_mapped(d), as you
mentioned below.
> 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.
Because I will remove outer check. But this check will remain. In this way
older OP-TEEs (without virtualization support) will still be accessible
from Dom0/HWDom.
> >+ 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.
Good idea, thanks.
> >+
> >+ 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));
Thanks. I'll check this in runtime, as I mentioned above.
> >+ 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.
Okay.
> >+ 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.
Linux kernel driver does memremap() in such place. OP-TEE maps it as non-secure
RAM. This shared memory is used to pass information between OP-TEE OS
and OP-TEE client. About which safety check you are talking?
>
> >+ 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.
I just wanted to emphasis that we mappaed memory for Dom0. Will remove.
> >+ 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.
Sorry, this is another debug print. Missed it when formatted patches.
> >+ /* 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?
This is a RFC. Will remove this parts of the code in a proper patch series.
I just wanted to ensure that community is okay with proposed approach and
to show how minimalistic mediator can look.
If you are generally okay with that, I'll address all comments, rework
second patch and push proper patch series.
WBR, Volodymyr
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |