diff mbox

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

Message ID 57762D54.3030302@denx.de
State RFC
Delegated to: Tom Rini
Headers show

Commit Message

Stefano Babic July 1, 2016, 8:44 a.m. UTC 
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:


What do you think ? If this could be a solution for release, I send a
formal patch.

Best regards,
Stefano

> This series tidies this up a little, to make mkimage more friendly.
> 
> 
> Simon Glass (14):
>   mkimage: Honour the default image type with auto-fit
>   mkimage: Explain the auto-fit imagefile special case
>   mkimage: Require a data file when auto-fit is used
>   mkimage: Drop premature setting of params.fit_image_type
>   mkimage: Drop blank line before main()
>   image: Correct auto-fit architecture property name
>   image: Convert the IH_... values to enums
>   image: Create a table of information for each category
>   image: Add a name for invalid types
>   image: Add functions to obtain category information
>   mkimage: Allow display of a list of any image header category
>   mkimage: Use generic code for showing an 'image type' error
>   mkimage: Show item lists for all categories
>   tools: Allow building with debug enabled
> 
>  Kconfig           |   9 +++
>  Makefile          |   3 +-
>  common/image.c    |  87 ++++++++++++++++++++-
>  include/image.h   | 230 ++++++++++++++++++++++++++++++++++--------------------
>  tools/fit_image.c |   3 +-
>  tools/mkimage.c   |  69 +++++++++-------
>  6 files changed, 280 insertions(+), 121 deletions(-)
>

Comments

Tom Rini July 1, 2016, 3:48 p.m. UTC | #1 
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.
Simon Glass July 1, 2016, 4:15 p.m. UTC | #2 
Hi,

On 1 July 2016 at 08:48, Tom Rini <trini@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
Tom Rini July 1, 2016, 4:45 p.m. UTC | #3 
On Fri, Jul 01, 2016 at 09:15:11AM -0700, Simon Glass wrote:
> Hi,
> 
> On 1 July 2016 at 08:48, Tom Rini <trini@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@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.
Stefano Babic July 1, 2016, 6:33 p.m. UTC | #4 
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@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@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.

>  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,
Stefano
Simon Glass July 1, 2016, 6:38 p.m. UTC | #5 
Hi Stefano,

On 1 July 2016 at 11:33, Stefano Babic <sbabic@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@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@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
diff mbox

Patch

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