Hi Stefano, On 1 July 2016 at 11:33, Stefano Babic <sba...@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 <tr...@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.ferre...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
