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

Hi,

On 26/02/18 10:00, Jun Nie wrote:

2018-02-26 17:56 GMT+08:00 Andre Przywara andre.przywara@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@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);