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

[Simon Glass]

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

Regards,
Simon