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

Hi Andrew,

On 14 November 2016 at 12:49, Andrew F. Davis afd@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@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