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

[Julien Grall <julien.grall@linaro.org>]

Hi Volodymyr,

On 17/10/17 19:57, Volodymyr Babchuk wrote:
> 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.

Thanks.

> 
>>>
>>>>>
>>>>> 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@epam.com>
>>>>> ---
>>>>>   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@epam.com>
>>>>> + * 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.

I am not sure to understand what you mean here. Do you expect OP-TEE to 
block? Or send an interrupt later on to say the work is finish?

[...]

>>>
>>>>> +                   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.
Thank you for the explanation, but I don't think this is addressing how 
this would prevent leaking data to the guest.

My request is to move the set_user_reg(...) calls outside of 
call_forward. So this would make clear the mediator needs to examine the 
result values.

To give you an example:

call_forward(....)
/* No need to sanitize value because... */
set_user_reg(...)
set_user_reg(...)

The caller may not need to examine the results. But at least it is clear 
compare to an helper hiding that.

Note that the set_user_reg(...) calls could in a another helper.

>>
>>>
>>>>> +
>>>>> +    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 if you use is_domain_direct_mapped(d) here, what will happen if two 
guests asked for shared memory?

> 
>>>
>>>> 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.

What do you mean by the last bit?

> 
>> 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.

To clarify my view. I am not against a temporary support of OP-TEE for 
the hardware domain in Xen. But it does not mean I would be ready to see 
  the a full OP-TEE support for guests in Xen.

Cheers,

-- 
Julien Grall

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