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

[Julien Grall <julien.grall@arm.com>]

Hi,

On 07/03/2019 21:04, Volodymyr Babchuk wrote:
> From: Volodymyr Babchuk <vlad.babchuk@gmail.com>
> 
> 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@gmail.com>
> 
> ---
> 
>   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@epam.com>
> + * 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel