[U-Boot] [PATCH] SPL: Add signature verification when loading image

Andre Przywara andre.przywara at arm.com
Mon Feb 26 11:02:41 UTC 2018


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.

>>
>>> 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