[U-Boot] [PATCH] fdt: Fix FIT header verification in mkimage and conduct same checks as bootm

* [U-Boot] [PATCH] fdt: Fix FIT header verification in mkimage and conduct same checks as bootm
       [not found] <20190219234004.29235-1-jorhand@microsoft.com>
@ 2019-02-21  2:47 ` Simon Glass
  2019-02-21 17:26   ` [U-Boot] [PATCH v2] " Jordan Hand
  0 siblings, 1 reply; 4+ messages in thread
From: Simon Glass @ 2019-02-21  2:47 UTC (permalink / raw)
  To: u-boot

Hi Jordan,

On Tue, 19 Feb 2019 at 16:40, Jordan Hand <jordanhand22@gmail.com> wrote:
>
> Signed-off-by: Jordan Hand <jorhand@microsoft.com>
> ---
> FIT header verification in mkimage was treating a return code as a boolean,
> which meant that failures in validating the fit were seen as successes.
>
> Additionally, mkimage was checking all formats to find a header which
> passes validation, rather than using the image type specified to
> mkimage.
>
> checkpatch.pl checks for lines ending with '(' and alignment matching
> open parentheses are ignored to keep with existing coding style.
>
>  tools/fit_common.c |  5 ++++-
>  tools/imagetool.c  | 31 +++++++++++++++++++++++++++++++
>  tools/imagetool.h  | 15 +++++++++++++++
>  tools/mkimage.c    |  2 +-
>  4 files changed, 51 insertions(+), 2 deletions(-)
>
> diff --git a/tools/fit_common.c b/tools/fit_common.c
> index d96085eaad..622a9e2a21 100644
> --- a/tools/fit_common.c
> +++ b/tools/fit_common.c
> @@ -26,7 +26,10 @@
>  int fit_verify_header(unsigned char *ptr, int image_size,
>                         struct image_tool_params *params)
>  {
> -       return fdt_check_header(ptr);
> +       if (fdt_check_header(ptr) != EXIT_SUCCESS || !fit_check_format(ptr))
> +               return -FDT_ERR_BADSTRUCTURE;

So not a standard error code from ernno.h?

Can you please add a comment to fit_verify_header() to indicate what
it returns? The use of EXIT_SUCCESS suggests that this value is
returned as the program's exit code, and we don't want to return a -ve
exit code.

> +
> +       return EXIT_SUCCESS;
>  }
>
>  int fit_check_image_types(uint8_t type)
> diff --git a/tools/imagetool.c b/tools/imagetool.c
> index b3e628f612..42c6a73925 100644
> --- a/tools/imagetool.c
> +++ b/tools/imagetool.c
> @@ -65,6 +65,37 @@ int imagetool_verify_print_header(
>         return retval;
>  }
>
> +int imagetool_verify_print_header_by_type(
> +       void *ptr,
> +       struct stat *sbuf,
> +       struct image_type_params *tparams,
> +       struct image_tool_params *params)
> +{
> +       int retval = tparams->verify_header((unsigned char *)ptr,
> +                                        sbuf->st_size, params);

Can you do:

   int retval;

   retval = ...

because it is nicer to separate declarations from non-trivial code.

> +
> +       if (retval == 0) {
> +               /*
> +                * Print the image information  if verify is

Single space between 'information' and 'if'

> +                * successful
> +                */
> +               if (tparams->print_header) {
> +                       if (!params->quiet)
> +                               tparams->print_header(ptr);
> +               } else {
> +                       fprintf(stderr,
> +                               "%s: print_header undefined for %s\n",
> +                               params->cmdname, tparams->name);
> +               }
> +       } else {
> +               fprintf(stderr,
> +                       "%s: verify_header failed for %s with exit code %d\n",
> +                       params->cmdname, tparams->name, retval);
> +       }
> +
> +       return retval;
> +}
> +
>  int imagetool_save_subimage(
>         const char *file_name,
>         ulong file_data,
> diff --git a/tools/imagetool.h b/tools/imagetool.h
> index 71471420f9..81f2f60727 100644
> --- a/tools/imagetool.h
> +++ b/tools/imagetool.h
> @@ -179,6 +179,21 @@ int imagetool_verify_print_header(
>         struct image_type_params *tparams,
>         struct image_tool_params *params);
>
> +/*
> + * imagetool_verify_print_header_by_type() - verifies the image header
> + *
> + * Verify the image_header for the image type given by tparams.
> + * If verification is successful, this prints the respective header.
> + *

Please add docs for the parameters as well.

> + * @return 0 on success, negative if input image format does not match with
> + * the given image type
> + */
> +int imagetool_verify_print_header_by_type(
> +       void *ptr,
> +       struct stat *sbuf,
> +       struct image_type_params *tparams,
> +       struct image_tool_params *params);
> +
>  /**
>   * imagetool_save_subimage - store data into a file
>   * @file_name: name of the destination file
> diff --git a/tools/mkimage.c b/tools/mkimage.c
> index ea5ed542ab..2899adff81 100644
> --- a/tools/mkimage.c
> +++ b/tools/mkimage.c
> @@ -409,7 +409,7 @@ int main(int argc, char **argv)
>                  * Print the image information for matched image type
>                  * Returns the error code if not matched
>                  */
> -               retval = imagetool_verify_print_header(ptr, &sbuf,
> +               retval = imagetool_verify_print_header_by_type(ptr, &sbuf,
>                                 tparams, &params);
>
>                 (void) munmap((void *)ptr, sbuf.st_size);
> --
> 2.17.1
>

It would be great to have a test for imagetool in pytest. Not sure if
you want to take that on :-)

Regards,
Simon

^ permalink raw reply	[flat|nested] 4+ messages in thread