2018-02-26 19:02 GMT+08:00 Andre Przywara <andre.przyw...@arm.com>: > Hi, > > On 26/02/18 10:00, Jun Nie wrote: >> 2018-02-26 17:56 GMT+08:00 Andre Przywara <andre.przyw...@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....@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); >>>> _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot