[U-Boot] [PATCH 00/14] mkimage: Tidy up error handling

[Tom Rini]

On Fri, Jul 01, 2016 at 09:15:11AM -0700, Simon Glass wrote:
> Hi,
> 
> On 1 July 2016 at 08:48, Tom Rini <trini at konsulko.com> wrote:
> > On Fri, Jul 01, 2016 at 10:44:04AM +0200, Stefano Babic wrote:
> >> Hi Simon,
> >>
> >> On 30/06/2016 18:52, Simon Glass wrote:
> >> > There are a few problems when mkimage is provided with invalid arguments.
> >> > In one case it crashes. When an invalid image type it is provided it lists
> >> > the valid types, but this is not implemented for compression, architecture
> >> > or OS.
> >> >
> >>
> >> There is another issue related to mkimage. It looks like it is broken
> >> since a lot of time, but it appears it was not noted.
> >>
> >> mkimage -l is broken. Testing with i.MX images (imximage), it does not
> >> show the header, but it reports the output as it as a "gpimage".
> >>
> >> In fact:
> >>
> >> ./tools/mkimage -l test.imx
> >> GP Header: Size d1002040 LoadAddr 8017
> >>
> >> It should be:
> >>
> >> ./tools/mkimage -l test.imx
> >> Image Type:   Freescale IMX Boot Image
> >> Image Ver:    2 (i.MX53/6/7 compatible)
> >> Data Size:    331776 Bytes = 324.00 kB = 0.32 MB
> >> Load Address: 177ff420
> >> Entry Point:  17800000
> >>
> >> The reason is due to the format of the gpimage itself. It has no magic
> >> number, and checking for its correctness means in gph_verify_header()
> >> just check that size and address are not NULL. That let think that the
> >> image is a gpimage, it is not. gpimage simply uses the image and does
> >> not let to check for further image headers that are put int the (sorted)
> >> list with the U_BOOT_IMAGE_TYPE. Image types that are in the list
> >> *before* gpimage are correctly recognized and printed, the following (as
> >> imximage) not.
> >>
> >> A quick fix (maybe just for the release ?) should be to let gpimage (but
> >> I do not know if there is another image type that misbehaves as it does)
> >> as the last one in the list: if all detections fail, the last one
> >> without any possibility for detection (gpimage) runs. The following
> >> patch works for me:
> >>
> >> diff --git a/tools/Makefile b/tools/Makefile
> >> index 63355aa..f72294a 100644
> >> --- a/tools/Makefile
> >> +++ b/tools/Makefile
> >> @@ -76,8 +76,6 @@ dumpimage-mkimage-objs := aisimage.o \
> >>                       lib/fdtdec.o \
> >>                       fit_common.o \
> >>                       fit_image.o \
> >> -                     gpimage.o \
> >> -                     gpimage-common.o \
> >>                       common/image-fit.o \
> >>                       image-host.o \
> >>                       common/image.o \
> >> @@ -100,6 +98,8 @@ dumpimage-mkimage-objs := aisimage.o \
> >>                       zynqimage.o \
> >>                       zynqmpimage.o \
> >>                       $(LIBFDT_OBJS) \
> >> +                     gpimage.o \
> >> +                     gpimage-common.o \
> >>                       $(RSA_OBJS-y)
> >>
> >>  dumpimage-objs := $(dumpimage-mkimage-objs) dumpimage.o
> >>
> >> What do you think ? If this could be a solution for release, I send a
> >> formal patch.
> >
> > Ug, I think we need what you're saying at least for release.  I'm
> > kicking off a big bisect now to see just when this last worked right.
> 
> That patch seems reasonable to me for now. My series is intended for
> the next release.
> 
> Perhaps verify_header() should return a value meaning 'possibly'?

So, this was broken by:
commit 0ca6691c2ec20ff264d882854c339795579991f5
Author: Guilherme Maciel Ferreira <guilherme.maciel.ferreira at gmail.com>
Date:   Thu Jan 15 02:48:05 2015 -0200

    imagetool: move common code to imagetool module

And yes, perhaps some way to say to say "maybe" is enough.  Or maybe we
just need to allow some to say that they can't verify?  I think we're thinking
along the same lines.


-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160701/f5c39d49/attachment.sig>