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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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