[U-Boot] [RFC 1/1] image: Add TEE loading to FIT loadable processing
Simon Glass
sjg at chromium.org
Tue Nov 15 01:34:08 CET 2016
Hi Andrew,
On 14 November 2016 at 14:55, Andrew F. Davis <afd at ti.com> wrote:
> On 11/14/2016 02:44 PM, Simon Glass wrote:
>> 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.
>>
>
> This was based on FIT_IMAGE_POST_PROCESS which is also generic but
> depends on TI as we currently are the only users.
>
> I think it should be removed from both, so I'll remove it here at least.
>
>>> + 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.
>>
>
> Will do.
>
>>> { -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().
>>
>
> Something like how boot_get_fpga() does it? This seems like a lot of
> code duplication between boot_get_fpga() and boot_get_loadable(), and
> both ignore ld_start and ld_len.
Yes it is. In fact I think we could rationalise these to some extent.
But it's not a big deal and could be done later.
>
> Does it not make more sense to have a single function for loadable
> components like we have now? The loadables themselves have a type we can
> switch on and then call a platform specific hook for that loadable type.
So with your patch we have two special processing things, right? One
for FPGA and one for TEE.
I wonder if we should have a linker list with handlers for each type.
We can search that list and call the provided function if there is
one. We could have handlers for FPGA and TEE, for now.
I spent a while refactoring the loading code. It's a tricky thing, but
I hope we can avoid filling it up with special cases again...
>
> I don't want a big TI specific function in common/image.c, but if this
> is okay I'll move it out of platform and into here.
Agreed we don't want that, and in fact the xilinux stuff in image.c isn't nice.
>
>> It is too ugly, I think, to check the image type in the 'load'
>> function, and do special things.
>>
>
> The alternative is a boot_get_<type> function for each type. All called
> from bootm_find_images().
>
>>> }
>>> 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.
>>
>
> Then board_tee_image_process would always have a deceleration, even on
> boards without a definition of it.
Right. But if someone uses it when they should not, they'll get an error.
>
>>> +/**
>>> + * 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?
>>
>
> Platform specific. Different TEEs have different header types and our
> platforms require different signing headers.
OK - can you please add that info here?
>
>>> + * @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
More information about the U-Boot
mailing list