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

[Simon Glass]

Hi Stefano,

On 1 July 2016 at 11:33, Stefano Babic <sbabic at denx.de> wrote:
> Hi Tom, Simon,
>
> On 01/07/2016 18:45, Tom Rini wrote:
>> 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
>>
>
> Yes, I had bisected myself and I get this commit - but the patch is not
> the readon of the breakage.
>
>> And yes, perhaps some way to say to say "maybe" is enough.
>
> Right, but there is a side effect. Even if a "maybe" is returned (or
> let's say, a "I can't check"), the effect is that "GP Header: Size XXXX"
> is still printed, because for the gpimage this *could* be a correct image.

Well, verifying whether this is 'your' image should not print
anything. We probably need to clarify the API a little. The comment
for verify_header() could say that printing is not allowed.

>
>>  Or maybe we
>> just need to allow some to say that they can't verify?  I think we're thinking
>> along the same lines.

Regards,
Simon