[U-Boot] [PATCH] SPL: Add signature verification when loading image
Jun Nie
jun.nie at linaro.org
Mon Feb 26 13:16:12 UTC 2018
2018-02-26 19:02 GMT+08:00 Andre Przywara <andre.przywara at arm.com>:
> Hi,
>
> On 26/02/18 10:00, Jun Nie wrote:
>> 2018-02-26 17:56 GMT+08:00 Andre Przywara <andre.przywara at arm.com>:
>>> Hi,
>>>
>>> On 11/02/18 11:56, Jun Nie wrote:
>>>> Add signature verification when loading FIT image in SPL.
>>>
>>> this message is very thin. Can you explain WHY you did this change and
>>> what it's supposed to do and what it improves?
>>> For my part I am completely clueless what you are after.
>>>
>>> Cheers,
>>> Andre.
>>
>> There is no support for signature check to u-boot proper in SPL,
>> except platform specific implementation. This patch add signature
>> check to u-boot in SPL, that can be shared on most platforms, if not
>> all.
>
> Thanks, that's a good *first* part of the commit message!
>
> Now it would be good to know how you achieve this, because this is not
> immediately obvious from looking at the patch.
> I guess you make the existing FIT image signature check used in U-Boot
> proper fit for being used in SPL as well? If yes, please state this.
>
> In general for this kind of patch I would structure a commit message
> like this:
> - What is the current situation, and why is it not good enough?
> - What is the change you propose, and how does it overcome the limitation?
> - What does that fix, specifically? (e.g. "Allows SPL signature checks
> on board xyz.")
>
> The last part is a clue to the maintainer why she should merge this patch.
>
> Cheers,
> Andre.
Thanks for sharing experience on this! If you do not have more
comments on these code,
I will prepare version 2 with these commit message.
Jun
>
>>>
>>>> Signed-off-by: Jun Nie <jun.nie at linaro.org>
>>>> ---
>>>> common/image-fit.c | 56 +++++++++++++++++++++++++++++++---------------------
>>>> common/spl/spl_fit.c | 12 +++++++++++
>>>> include/image.h | 2 ++
>>>> 3 files changed, 48 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/common/image-fit.c b/common/image-fit.c
>>>> index f6e956a..94d9d4b 100644
>>>> --- a/common/image-fit.c
>>>> +++ b/common/image-fit.c
>>>> @@ -1068,34 +1068,14 @@ static int fit_image_check_hash(const void *fit, int noffset, const void *data,
>>>> return 0;
>>>> }
>>>>
>>>> -/**
>>>> - * fit_image_verify - verify data integrity
>>>> - * @fit: pointer to the FIT format image header
>>>> - * @image_noffset: component image node offset
>>>> - *
>>>> - * fit_image_verify() goes over component image hash nodes,
>>>> - * re-calculates each data hash and compares with the value stored in hash
>>>> - * node.
>>>> - *
>>>> - * returns:
>>>> - * 1, if all hashes are valid
>>>> - * 0, otherwise (or on error)
>>>> - */
>>>> -int fit_image_verify(const void *fit, int image_noffset)
>>>> +int fit_image_verify_with_data(const void *fit, int image_noffset,
>>>> + const void *data, size_t size)
>>>> {
>>>> - const void *data;
>>>> - size_t size;
>>>> int noffset = 0;
>>>> char *err_msg = "";
>>>> int verify_all = 1;
>>>> int ret;
>>>>
>>>> - /* Get image data and data length */
>>>> - if (fit_image_get_data(fit, image_noffset, &data, &size)) {
>>>> - err_msg = "Can't get image data/size";
>>>> - goto error;
>>>> - }
>>>> -
>>>> /* Verify all required signatures */
>>>> if (IMAGE_ENABLE_VERIFY &&
>>>> fit_image_verify_required_sigs(fit, image_noffset, data, size,
>>>> @@ -1153,6 +1133,38 @@ error:
>>>> }
>>>>
>>>> /**
>>>> + * fit_image_verify - verify data integrity
>>>> + * @fit: pointer to the FIT format image header
>>>> + * @image_noffset: component image node offset
>>>> + *
>>>> + * fit_image_verify() goes over component image hash nodes,
>>>> + * re-calculates each data hash and compares with the value stored in hash
>>>> + * node.
>>>> + *
>>>> + * returns:
>>>> + * 1, if all hashes are valid
>>>> + * 0, otherwise (or on error)
>>>> + */
>>>> +int fit_image_verify(const void *fit, int image_noffset)
>>>> +{
>>>> + const void *data;
>>>> + size_t size;
>>>> + int noffset = 0;
>>>> + char *err_msg = "";
>>>> +
>>>> + /* Get image data and data length */
>>>> + if (fit_image_get_data(fit, image_noffset, &data, &size)) {
>>>> + err_msg = "Can't get image data/size";
>>>> + printf("error!\n%s for '%s' hash node in '%s' image node\n",
>>>> + err_msg, fit_get_name(fit, noffset, NULL),
>>>> + fit_get_name(fit, image_noffset, NULL));
>>>> + return 0;
>>>> + }
>>>> +
>>>> + return fit_image_verify_with_data(fit, image_noffset, data, size);
>>>> +}
>>>> +
>>>> +/**
>>>> * fit_all_image_verify - verify data integrity for all images
>>>> * @fit: pointer to the FIT format image header
>>>> *
>>>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>>>> index cc07fbc..8d382eb 100644
>>>> --- a/common/spl/spl_fit.c
>>>> +++ b/common/spl/spl_fit.c
>>>> @@ -174,6 +174,9 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
>>>> uint8_t image_comp = -1, type = -1;
>>>> const void *data;
>>>> bool external_data = false;
>>>> +#ifdef CONFIG_SPL_FIT_SIGNATURE
>>>> + int ret;
>>>> +#endif
>>>>
>>>> if (IS_ENABLED(CONFIG_SPL_OS_BOOT) && IS_ENABLED(CONFIG_SPL_GZIP)) {
>>>> if (fit_image_get_comp(fit, node, &image_comp))
>>>> @@ -252,7 +255,16 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector,
>>>> image_info->entry_point = fdt_getprop_u32(fit, node, "entry");
>>>> }
>>>>
>>>> +#ifdef CONFIG_SPL_FIT_SIGNATURE
>>>> + printf("## Checking hash(es) for Image %s ...\n",
>>>> + fit_get_name(fit, node, NULL));
>>>> + ret = fit_image_verify_with_data(fit, node,
>>>> + (const void *)load_addr, length);
>>>> + printf("\n");
>>>> + return !ret;
>>>> +#else
>>>> return 0;
>>>> +#endif
>>>> }
>>>>
>>>> static int spl_fit_append_fdt(struct spl_image_info *spl_image,
>>>> diff --git a/include/image.h b/include/image.h
>>>> index 325b014..77c11f8 100644
>>>> --- a/include/image.h
>>>> +++ b/include/image.h
>>>> @@ -1013,6 +1013,8 @@ int fit_add_verification_data(const char *keydir, void *keydest, void *fit,
>>>> const char *comment, int require_keys,
>>>> const char *engine_id);
>>>>
>>>> +int fit_image_verify_with_data(const void *fit, int image_noffset,
>>>> + const void *data, size_t size);
>>>> int fit_image_verify(const void *fit, int noffset);
>>>> int fit_config_verify(const void *fit, int conf_noffset);
>>>> int fit_all_image_verify(const void *fit);
>>>>
More information about the U-Boot
mailing list