|
[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 Tue, Oct 17, 2017 at 06:30:13PM +0100, Julien Grall wrote:
> >>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.
>
> So basically you are saying the header is the documentation of the API?
> There are not external documentation making easier to follow the version...?
There are high-level documentation at [1]. All details are covered in headers.
> >
> >>>
> >>>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.
>
> "On OP-TEE side:
> 1. Shared memory redesign which is almost complete.
> 2. Implement new SMCs from hypervsiror to TEE to track VM lifecycle.
> 3. Track VM IDs to isolated VM data.
> 4. RPCs to sleeping guests."
Yes, this are my plans. First item is done. I'm currently working on
others. Sorry, looks like I didn't clearly showed, that this is what should
be done. It is not done yet.
> I was kind of expecting that was done given you put a client ID here. If it
> is not done, then why are you passing a ID that we are not even sure OP-TEE
> will be able to understand?
OP-TEE has very rudimentary support of client ID [2]. So it will understand it.
It uses client ID to ensure, that right VM does return from a RPC. There are
no other uses for it right now.
> >But, answering your question, optee_domain_create() will tell OP-TEE about
> >new client ID.
>
> And I guess you would expect that the registration may fail, hence an error
> return is mandatory in optee_domain_create.
Agree.
> >
> >>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.
>
> So how many client do you expect to be supported? And how do you expect to
> get that information back?
I can't answer to your question right now. OP-TEE has limited memory
resources. It need to share them among multiple guests. This is not
done yet. So I can't make assumptions yet.
> >
> >>>+ 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.
>
> Which set_user_reg()? The helper does not do any modification... If you
> speak about the code below, then it is very confusing and error-prone.
No, I was speaking about code above. The one that calls set_user_reg().
You leave your comment there, so I assumed you are talking about that part.
> If you separate the call from setting the guest registers then the you give
> a hint to the caller that maybe something has to be down and he can't
> blindly trust the result...
Let me describe how this works right now. XEN traps SMC to OP-TEE and forwards
it to the mediator. Mediator examines registers to determine type of the call.
Then it either:
* Forwards it to OP-TEE as is. This does forward_call(). forward_call()
executes real SMC and then writes return data to guest registers
* Forwards it to OP-TEE and then examines result. Again, it uses
forward_call() to execute SMC and then it checks returned values.
For example, if guest wanted to exchange capabilities with OP-TEE,
mediator checks if OP-TEE support dynamic SHM. If it is not supported
and guest is not hwdom, then mediator injects an error to guest.
This prevents further initialization of OP-TEE driver in client.
Another example is static SHM configuration. If this request was sent
from hwdom, then upon return from OP-TEE, mediator maps static
shm into hwdom address space. Else it returns an error. DomU sees
that static SHM is not available and relies only on dynamic SHM.
Idea is to make this transparent for OP-TEE client driver. It does not
need to know that it is running in virtualized environment (one
backward-compatible change will be needed anyways, but this is
another story).
Static SHM is a predefined region, that is shared by OP-TEE OS and OP-TEE
client.
Both sides expect that it is a physically contiguous memory region.
Dynamic SHM is never thing. It allows OP-TEE client to use any portion of own
memory as SHM. It was designed with virtualization in mind, so it support
non-contiguous memory regions. Thus it can be used by DomU clients.
>
> >
> >>>+
> >>>+ 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.
>
> On Arm Dom0 == Hardware Domain. If Hardware Domain were introduced, then I
> would expect OP-TEE to be handled by the it and not Dom0.
Oh, I see. Thank you for explanation.
> >
> >Looks like it is better to check for is_domain_direct_mapped(d), as you
> >mentioned below.
>
> is_domain_direct_mapped(d) != hwdom. Please don't mix the both. The former
> is here to proctect you gfn == mfn. The latter is here to make sure no other
> domain than the hardware domain is going to use the shared memory.
Yes, I see. As I said earlier, only 1:1 mapped domain can use static SHM
mechanism. So I think I need to use is_domain_direct_mapped(d).
> >
> >>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?
>
> Well, does OP-TEE validate the data read from that shared region? But it
> seems that you don't plan to give part of the SHM to a guest, so it might be
> ok.
OP-TEE surely validate all data from NW. Also OP-TEE is written in such way,
that it reads from shared memory only once, to ensure that NW will not change
data after validation. Mediator will do the same.
> Also how OP-TEE will map this region? Cacheable...?
Yes, cacheable, PR, PW, non-secure.
> >
> >>
> >>>+ 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.
>
> gprintk will printk d0. So there are no point to say it a second time...
> >
> >>>+ 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.
> I don't think this is true. You only show how easy it is to let Dom0
> accessing TEE. And as I said in the cover letter, this is not the
> controversial part.
Actually I wanted to show approach when mediator resides right in xen.
I got valuable input from you. Now I see that I must completely rework the
first patch. And, probably, show more comprehensive support from OP-TEE side.
> The more controversial one is the guest support that you completely left
> aside. I believe this part will not be as minimalistic as you think because
> you need to translate buffer address and prevent those buffers to disappear
> under your feet.
Yes. I plan to copy all buffers where IPAs presented to another place,
so DomU will not be able to see PAs during translation. And I plan to
pin all DomU pages with a data. Also I'll read from guest pages only
once. I think, this will be enough.
> There are probably other problem to fix...
Probably yes...
I think, I'll focus on OP-TEE side right now and come back when there will
be more more to show.
[1] https://github.com/OP-TEE/optee_os/blob/master/documentation/optee_design.md
[2]
https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/kernel/thread.c#L459
WBR, Volodymyr.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |