[U-Boot] [RFC 1/1] image: Add TEE loading to FIT loadable processing

[Simon Glass]

Hi Andrew,

On 14 November 2016 at 12:49, Andrew F. Davis <afd at ti.com> wrote:
> To help automate the loading of a TEE image during the boot we add a new
> FIT section type 'tee', when we see this type while loading the loadable
> sections we automatically call the platforms TEE processing function on
> this image section.
>
> Signed-off-by: Andrew F. Davis <afd at ti.com>
> ---
>  Kconfig         | 10 ++++++++++
>  common/image.c  | 18 ++++++++++++++++++
>  include/image.h | 15 +++++++++++++++
>  3 files changed, 43 insertions(+)
>
> diff --git a/Kconfig b/Kconfig
> index 1263d0b..97cf7c8 100644
> --- a/Kconfig
> +++ b/Kconfig
> @@ -291,6 +291,16 @@ config FIT_IMAGE_POST_PROCESS
>           injected into the FIT creation (i.e. the blobs would have been pre-
>           processed before being added to the FIT image).
>
> +config FIT_IMAGE_TEE_PROCESS
> +       bool "Enable processing of TEE images during FIT loading by U-Boot"
> +       depends on FIT && TI_SECURE_DEVICE

This is a generic option so I don't think it should depend on TI.

> +       help
> +         Allows platforms to perform processing, such as authentication and
> +         installation, on TEE images extracted from FIT images in a platform
> +         or board specific way. In order to use this feature a platform or
> +         board-specific implementation of board_tee_image_process() must be
> +         provided.
> +
>  config SPL_DFU_SUPPORT
>         bool "Enable SPL with DFU to load binaries to memory device"
>         depends on USB
> diff --git a/common/image.c b/common/image.c
> index 7604494..4552ca5 100644
> --- a/common/image.c
> +++ b/common/image.c
> @@ -165,6 +165,7 @@ static const table_entry_t uimage_type[] = {
>         {       IH_TYPE_ZYNQIMAGE,  "zynqimage",  "Xilinx Zynq Boot Image" },
>         {       IH_TYPE_ZYNQMPIMAGE, "zynqmpimage", "Xilinx ZynqMP Boot Image" },
>         {       IH_TYPE_FPGA,       "fpga",       "FPGA Image" },
> +       {       IH_TYPE_TEE,        "tee",        "TEE OS Image",},

Perhaps write out TEE in full? It's a bit cryptic.

>         {       -1,                 "",           "",                   },
>  };
>
> @@ -1408,6 +1409,8 @@ int boot_get_loadable(int argc, char * const argv[], bootm_headers_t *images,
>         int fit_img_result;
>         const char *uname;
>
> +       uint8_t img_type;
> +
>         /* Check to see if the images struct has a FIT configuration */
>         if (!genimg_has_config(images)) {
>                 debug("## FIT configuration was not specified\n");
> @@ -1447,6 +1450,21 @@ int boot_get_loadable(int argc, char * const argv[], bootm_headers_t *images,
>                                 /* Something went wrong! */
>                                 return fit_img_result;
>                         }
> +
> +                       fit_img_result = fit_image_get_node(buf, uname);
> +                       if (fit_img_result < 0) {
> +                               /* Something went wrong! */
> +                               return fit_img_result;
> +                       }
> +                       fit_img_result = fit_image_get_type(buf, fit_img_result, &img_type);
> +                       if (fit_img_result < 0) {
> +                               /* Something went wrong! */
> +                               return fit_img_result;
> +                       }
> +#if defined(CONFIG_FIT_IMAGE_TEE_PROCESS)
> +                       if (img_type == IH_TYPE_TEE)
> +                               board_tee_image_process(img_data, img_len);
> +#endif

Instead of putting this here, I think it would be better for
boot_get_loadable() to return the correct values for ld_start and
ld_len. Perhaps you need to pass it the loadable index to load, so it
is called multiple times? The only caller is bootm_find_images().

It is too ugly, I think, to check the image type in the 'load'
function, and do special things.

>                 }
>                 break;
>         default:
> diff --git a/include/image.h b/include/image.h
> index 2b1296c..57084c8 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -279,6 +279,7 @@ enum {
>         IH_TYPE_ZYNQMPIMAGE,            /* Xilinx ZynqMP Boot Image */
>         IH_TYPE_FPGA,                   /* FPGA Image */
>         IH_TYPE_VYBRIDIMAGE,    /* VYBRID .vyb Image */
> +       IH_TYPE_TEE,            /* Trusted Execution Environment OS Image */
>
>         IH_TYPE_COUNT,                  /* Number of image types */
>  };
> @@ -1263,4 +1264,18 @@ int board_fit_config_name_match(const char *name);
>  void board_fit_image_post_process(void **p_image, size_t *p_size);
>  #endif /* CONFIG_SPL_FIT_IMAGE_POST_PROCESS */
>
> +#ifdef CONFIG_FIT_IMAGE_TEE_PROCESS

I don't think you should have this #ifdef in the header file.

> +/**
> + * board_fit_tee_process() - Do any needed processing on a loaded TEE image
> + *
> + * This is used to verify, decrypt, and/or install a TEE in a platform or
> + * board specific way.

nit: board-specific


> + *
> + * @tee_image: pointer to the image

What format is the image?

> + * @tee_size: the image size
> + * @return no return value (failure should be handled internally)
> + */
> +void board_tee_image_process(void *tee_image, size_t tee_size);

I think it's a good idea to return an error code here, since the
function may fail.

> +#endif /* CONFIG_FIT_IMAGE_TEE_PROCESS */
> +
>  #endif /* __IMAGE_H__ */
> --
> 2.10.1
>

Regards,
SImon