[U-Boot] [RFC 1/1] image: Add TEE loading to FIT loadable processing
Andrew F. Davis
afd at ti.com
Tue Nov 15 18:07:26 CET 2016
On 11/14/2016 06:34 PM, Simon Glass wrote:
> 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.
>
My plan would be to have only the basic image types handled in
common/image.c (ramdisk, dtb, loadables, etc..), then we have a switch
of optional callbacks for different "loadable" types. FPGA, DSP, TEE,
are all images that need to be "loaded" with platform specific handlers.
So my proposal with this patch is to *not* add a new image type loader,
but to use the existing "loadable" type.
Right now a given FIT configuration can have
kernel=
ramdisk=
dtb=
loadables=
When for instance when kernel type is parsed we look at what type of
kernel it is by looking at the node it points to (we also get the
data/arch/etc.. from that node). When we parse loadables we should do
the same, when the loadable type is a recognized type, we load in the
data and call a platform hook to further process it.
> 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.
>
This is almost what I have now, but with an ifdef'able switch block over
the types.
> 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...
>
My hope as well, what I'm trying to avoid is doing it like this new
Xilinx FPGA loader where we have a custom type loader in common code.
>>
>> 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.
>
So moving forward, is this a better solution, should I un-RFC this
patch? Then we can move over the Xilinx loader to this style.
>>
>>> 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.
>
A hard to track down link-time error. With this ifdef'd off, they will
also get an error, but the compiler will complain and point them right
to the offending call.
>>
>>>> +/**
>>>> + * 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?
>
Will do.
>>
>>>> + * @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