Re: [Xen-devel] [PATCH v1 2/6] arm: add generic TEE mediator framework

[Julien Grall <julien.grall@xxxxxxx>]

Hi Volodymyr,

On 22/08/18 15:11, Volodymyr Babchuk wrote:
> This patch adds basic framework for TEE mediators. Guests can't talk
> to TEE directly, we need some entity that will intercept request
> and decide what to do with them. "TEE mediator" is a such entity.
>
> This is how it works: user can build XEN with multiple TEE mediators
> (see the next patches, where OP-TEE mediator is introduced).
> TEE mediator register self with REGISTER_TEE_MEDIATOR() macro in the
> same way, as device drivers use DT_DEVICE_START()/DT_DEVICE_END()
> macros.
> In runtime, during initialization, framework calls probe() function
> for each available mediator driver to find which TEE is installed
> on the platform. Then generic vSMC handler will call selected mediator
> when it intercept SMC that belongs to TEE OS or TEE application.
>
> Also, there are hooks for domain construction and destruction, so
> TEE mediator can inform TEE about VM lifecycle.
>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> ---
>
> changes from "RFC" version:
>   - renamed CONFIG_ARM_TEE to CONFIG_TEE
>   - changed discovery mechanism: instead of UUID mathing, TEE-specific
>     probing is used
>
>   MAINTAINERS                   |   5 ++
>   xen/arch/arm/Kconfig          |  10 ++++
>   xen/arch/arm/Makefile         |   1 +
>   xen/arch/arm/domain.c         |   7 +++
>   xen/arch/arm/setup.c          |   4 ++
>   xen/arch/arm/shutdown.c       |   2 +
>   xen/arch/arm/tee/Kconfig      |   0
>   xen/arch/arm/tee/Makefile     |   1 +
>   xen/arch/arm/tee/tee.c        |  79 ++++++++++++++++++++++++++++++++
>   xen/arch/arm/vsmc.c           |   5 ++
>   xen/arch/arm/xen.lds.S        |   7 +++
>   xen/include/asm-arm/tee/tee.h | 103 ++++++++++++++++++++++++++++++++++++++++++
>   12 files changed, 224 insertions(+)
>   create mode 100644 xen/arch/arm/tee/Kconfig
>   create mode 100644 xen/arch/arm/tee/Makefile
>   create mode 100644 xen/arch/arm/tee/tee.c
>   create mode 100644 xen/include/asm-arm/tee/tee.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 15d45d0..b0034a9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -365,6 +365,11 @@ F: config/Stubdom.mk.in
>   F:    m4/stubdom.m4
>   F:    stubdom/
>   
> +TEE MEDIATORS
> +M:     Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> +S:     Supported
> +F:     xen/arch/arm/tee/*
The star is not necessary here.

> +
>   TOOLSTACK
>   M:    Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
>   M:    Wei Liu <wei.liu2@xxxxxxxxxx>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 2782ee6..09bfc9b 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -57,6 +57,14 @@ config SBSA_VUART_CONSOLE
>           Allows a guest to use SBSA Generic UART as a console. The
>           SBSA Generic UART implements a subset of ARM PL011 UART.
>   
> +config TEE
> +       bool "Enable TEE mediators support"
> +       default n
I think we want this to depends on EXPERT for a few releases. We can Xen 
discuss how we are going to security support this.
> +       depends on ARM
No need to depend on Arm here.

> +       help
> +         This option enables generic TEE mediators support. It allows guests
> +         to access real TEE via one of TEE mediators implemented in XEN.
> +
>   endmenu
>   
>   menu "ARM errata workaround via the alternative framework"
> @@ -197,3 +205,5 @@ config ARM32_HARDEN_BRANCH_PREDICTOR
>   source "common/Kconfig"
>   
>   source "drivers/Kconfig"
> +
> +source "arch/arm/tee/Kconfig"
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 30a2a65..af29aa7 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -3,6 +3,7 @@ subdir-$(CONFIG_ARM_64) += arm64
>   subdir-y += platforms
>   subdir-$(CONFIG_ARM_64) += efi
>   subdir-$(CONFIG_ACPI) += acpi
> +subdir-$(CONFIG_TEE) += tee
>   
>   obj-$(CONFIG_HAS_ALTERNATIVE) += alternative.o
>   obj-y += bootfdt.init.o
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index a74ff1c..41e41b9 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -31,6 +31,7 @@
>   #include <asm/platform.h>
>   #include <asm/procinfo.h>
>   #include <asm/regs.h>
> +#include <asm/tee/tee.h>
>   #include <asm/vfp.h>
>   #include <asm/vgic.h>
>   #include <asm/vtimer.h>
> @@ -671,6 +672,9 @@ int arch_domain_create(struct domain *d, unsigned int 
> domcr_flags,
>       if ( is_hardware_domain(d) && (rc = domain_vuart_init(d)) )
>           goto fail;
>   
> +    /* Notify TEE that new domain was created */
> +    tee_domain_create(d);
My concern about domain creation is still not addressed. I would expect 
the toolstack to decide whether TEE should be initialized for a given 
guest and potentially return an error on failure (e.g maximum client ID 
has been reached).
But very likely, you don't need to initialize TEE that early. This could 
be done in a separate DOMCTL as we did for VPL011.
> +
>       return 0;
>   
>   fail:
> @@ -878,6 +882,9 @@ int domain_relinquish_resources(struct domain *d)
>            */
>           domain_vpl011_deinit(d);
>   
> +        /* Free TEE mediator resources */
> +        tee_domain_destroy(d);
> +
>           d->arch.relmem = RELMEM_xen;
>           /* Fallthrough */
>   
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 032a6a8..4d31d94 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -47,6 +47,7 @@
>   #include <asm/platform.h>
>   #include <asm/procinfo.h>
>   #include <asm/setup.h>
> +#include <asm/tee/tee.h>
>   #include <xsm/xsm.h>
>   #include <asm/acpi.h>
>   
> @@ -851,6 +852,9 @@ void __init start_xen(unsigned long boot_phys_offset,
>       apply_alternatives_all();
>       enable_errata_workarounds();
>   
> +    /* Initialize TEE mediator */
> +    tee_init();
Does this really need to be called directly? Can't this be using an init 
call?
> +
>       /* Create initial domain 0. */
>       /* The vGIC for DOM0 is exactly emulating the hardware GIC */
>       config.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
> diff --git a/xen/arch/arm/shutdown.c b/xen/arch/arm/shutdown.c
> index b32f07e..e1ace18 100644
> --- a/xen/arch/arm/shutdown.c
> +++ b/xen/arch/arm/shutdown.c
> @@ -5,6 +5,7 @@
>   #include <xen/smp.h>
>   #include <asm/platform.h>
>   #include <asm/psci.h>
> +#include <asm/tee/tee.h>
>   
>   static void noreturn halt_this_cpu(void *arg)
>   {
> @@ -15,6 +16,7 @@ void machine_halt(void)
>   {
>       int timeout = 10;
>   
> +    tee_remove();
This callback does not seem to be implemented for OP-TEE. I would rather 
only introduce callback that are only necessary at the moment. The other 
can be added later on.
>       watchdog_disable();
>       console_start_sync();
>       local_irq_enable();
> diff --git a/xen/arch/arm/tee/Kconfig b/xen/arch/arm/tee/Kconfig
> new file mode 100644
> index 0000000..e69de29
> diff --git a/xen/arch/arm/tee/Makefile b/xen/arch/arm/tee/Makefile
> new file mode 100644
> index 0000000..c54d479
> --- /dev/null
> +++ b/xen/arch/arm/tee/Makefile
> @@ -0,0 +1 @@
> +obj-y += tee.o
> diff --git a/xen/arch/arm/tee/tee.c b/xen/arch/arm/tee/tee.c
> new file mode 100644
> index 0000000..6df3b09
> --- /dev/null
> +++ b/xen/arch/arm/tee/tee.c
> @@ -0,0 +1,79 @@
> +/*
> + * xen/arch/arm/tee/tee.c
> + *
> + * Generic part of TEE mediator subsystem
> + *
> + * 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/errno.h>
> +#include <xen/types.h>
> +#include <asm/tee/tee.h>
> +
> +extern const struct tee_mediator_desc _steemediator[], _eteemediator[];
> +static const struct tee_mediator_ops *mediator_ops;
> +
> +void tee_init(void)
> +{
> +    const struct tee_mediator_desc *desc;
> +
> +    for ( desc = _steemediator; desc != _eteemediator; desc++ )
{

> +        if ( desc->ops->probe() == 0 )
NIT: !desc->ops->probe()

> +        {
> +            printk(XENLOG_INFO "Using TEE mediator for %s\n", desc->name);
> +            mediator_ops = desc->ops;
> +            return;
> +        }
}

> +    printk(XENLOG_WARNING "No TEE mediator found\n");
Not having a TEE is a valid use case. So printing a warning seems a bit 
too much.
> +}
> +
> +bool tee_handle_smc(struct cpu_user_regs *regs)
How about HVC?

> +{
> +    if ( !mediator_ops )
> +        return false;
> +
> +    return mediator_ops->handle_smc(regs);
> +}
> +
> +int tee_domain_create(struct domain *d)
> +{
> +    if ( !mediator_ops )
> +        return -ENODEV;
> +
> +    return mediator_ops->domain_create(d);
> +}
> +
> +void tee_domain_destroy(struct domain *d)
> +{
> +    if ( !mediator_ops )
> +        return;
> +
> +    return mediator_ops->domain_destroy(d);
> +}
> +
> +void tee_remove(void)
> +{
> +    if ( !mediator_ops )
> +        return;
> +
> +    return mediator_ops->remove();
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> index 40a80d5..aacebf9 100644
> --- a/xen/arch/arm/vsmc.c
> +++ b/xen/arch/arm/vsmc.c
> @@ -22,6 +22,7 @@
>   #include <asm/monitor.h>
>   #include <asm/regs.h>
>   #include <asm/smccc.h>
> +#include <asm/tee/tee.h>
>   #include <asm/traps.h>
>   #include <asm/vpsci.h>
>   
> @@ -235,6 +236,10 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs)
>           case ARM_SMCCC_OWNER_STANDARD:
>               handled = handle_sssc(regs);
>               break;
> +        case ARM_SMCCC_OWNER_TRUSTED_APP ... ARM_SMCCC_OWNER_TRUSTED_APP_END:
> +        case ARM_SMCCC_OWNER_TRUSTED_OS ... ARM_SMCCC_OWNER_TRUSTED_OS_END:
> +            handled = tee_handle_smc(regs);
I think you want to rename that function "tee_handle_call".

> +            break;
>           }
>       }
>   
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index c9b9546..b78b7f1 100644
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -126,6 +126,13 @@ SECTIONS
>         _aedevice = .;
>     } :text
>   
> +  . = ALIGN(8);
> +  .teemediator.info : {
> +      _steemediator = .;
> +      *(.teemediator.info)
> +      _eteemediator = .;
> +  } :text
> +
>     . = ALIGN(PAGE_SIZE);             /* Init code and data */
>     __init_begin = .;
>     .init.text : {
> diff --git a/xen/include/asm-arm/tee/tee.h b/xen/include/asm-arm/tee/tee.h
> new file mode 100644
> index 0000000..ffd25b0
> --- /dev/null
> +++ b/xen/include/asm-arm/tee/tee.h
> @@ -0,0 +1,103 @@
> +/*
> + * xen/include/asm-arm/tee.h
> + *
> + * Generic part of TEE mediator subsystem
> + *
> + * 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 as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
Xen is GPLv2 only.

> + *
> + * 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.
> + */
> +
> +#ifndef __ARCH_ARM_TEE_TEE_H__
> +#define __ARCH_ARM_TEE_TEE_H__
> +
> +#include <xen/lib.h>
> +#include <xen/types.h>
> +#include <asm/regs.h>
> +
> +#ifdef CONFIG_TEE
> +
> +struct tee_mediator_ops {
> +    /*
> +     * Probe for TEE. Should return 0 if TEE found and
> +     * mediator is initialized.
> +     */
> +    int (*probe)(void);
The caller does not care about the value but knowing there was an error. 
So it looks like to me you want to return a boolean instead.
> +
> +    /*
> +     * Called during domain construction so mediator can inform TEE about new
> +     * guest and create own structures for the new domain.
> +     */
> +    int (*domain_create)(struct domain *d);
> +
> +    /*
> +     * Called during domain destruction to inform TEE that guest is now dead
> +     * and to destroy all resources allocated for the domain being destroyed.
> +     */
> +    void (*domain_destroy)(struct domain *d);
> +
> +    /* Handle SMC call for current domain. */
> +    bool (*handle_smc)(struct cpu_user_regs *regs);
> +
> +    /* Called during XEN shutdown to free remaining resources. */
> +    void (*remove)(void);
> +};
> +
> +struct tee_mediator_desc {
> +    /* Name of the TEE. Just for debugging purposes. */
> +    const char *name;
> +
> +    /* Mediator callbacks as described above. */
> +    const struct tee_mediator_ops *ops;
> +};
> +
> +void tee_init(void);
> +bool tee_handle_smc(struct cpu_user_regs *regs);
> +int tee_domain_create(struct domain *d);
> +void tee_domain_destroy(struct domain *d);
> +void tee_remove(void);
> +
> +#define REGISTER_TEE_MEDIATOR(_name, _namestr, _ops)          \
> +static const struct tee_mediator_desc __tee_desc_##_name __used     \
> +__section(".teemediator.info") = {                                  \
> +    .name = _namestr,                                               \
> +    .ops = _ops                                                     \
> +}
> +
> +#else
> +
> +static inline void tee_init(void) {}
> +static inline bool tee_handle_smc(struct cpu_user_regs *regs)
> +{
> +    return false;
> +}
> +
> +static inline int tee_domain_create(struct domain *d)
> +{
> +    return -ENODEV;
> +}
> +
> +static inline void tee_domain_destroy(struct domain *d) {}
> +static inline void tee_remove(void) {}
> +
> +#endif  /* CONFIG_TEE */
> +
> +#endif /* __ARCH_ARM_TEE_TEE_H__ */
> +
> +/*
> + * 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel