[U-Boot] [PATCH] tools: default image: use ih_size for checking data size

[U-Boot] [PATCH] tools: default image: use ih_size for checking data size

[Jonas Gorski]

Common image usage is uImage + appended rootfs, so the the uImage data
is only part of the total image. So read out and use the header's
ih_size field instead of the total file size.

To prevent reading over the end of the buffer, check that the image file
is big enough to contain the data before calculating its checksum.

Before:
~# mkimage -l dir665_fw_100NA.bin
mkimage: ERROR: "dir665_fw_100NA/dir665_fw_100NA.bin" has corrupted data!

After:
~# mkimage -l dir665_fw_100NA.bin
Image Name:   Linux Kernel Image
Created:      Fri Feb 12 03:38:36 2010
Image Type:   ARM Linux Kernel Image (lzma compressed)
Data Size:    1107781 Bytes = 1081.82 kB = 1.06 MB
Load Address: 00008000
Entry Point:  00008000

Signed-off-by: Jonas Gorski <jogo@openwrt.org>
---
 tools/default_image.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/default_image.c b/tools/default_image.c
index e9d0729..db20e53 100644
--- a/tools/default_image.c
+++ b/tools/default_image.c
@@ -86,10 +86,11 @@ static int image_verify_header(unsigned char *ptr, int image_size,
 	}
 
 	data = (const unsigned char *)ptr + sizeof(image_header_t);
-	len  = image_size - sizeof(image_header_t) ;
+	len  = be32_to_cpu(hdr->ih_size);
 
 	checksum = be32_to_cpu(hdr->ih_dcrc);
-	if (crc32(0, data, len) != checksum) {
+	if ((image_size - sizeof(image_header_t)) < len ||
+	    crc32(0, data, len) != checksum) {
 		fprintf(stderr,
 			"%s: ERROR: \"%s\" has corrupted data!\n",
 			params->cmdname, params->imagefile);
-- 
1.7.10.4

[U-Boot] [PATCH] tools: default image: use ih_size for checking data size

[Wolfgang Denk]

Dear Jonas Gorski,

In message <1367581025-26740-1-git-send-email-jogo@openwrt.org> you wrote:
> Common image usage is uImage + appended rootfs, so the the uImage data

No, this is not at all "common usage".  Actually this something you
should never do.

> is only part of the total image. So read out and use the header's
> ih_size field instead of the total file size.
> 
> To prevent reading over the end of the buffer, check that the image file
> is big enough to contain the data before calculating its checksum.
> 
> Before:
> ~# mkimage -l dir665_fw_100NA.bin
> mkimage: ERROR: "dir665_fw_100NA/dir665_fw_100NA.bin" has corrupted data!

Sorry, I don't know how you create your image files, but you must be
doing something fundamentally wrong.  If mkimage reports a bug here,
it is probably right.  If the actual payload size is different from
the content of the  ih_size  field, then your image _is_ broken.

NAK.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The aim of science is not to open the door to everlasting wisdom  but
to set a limit on everlasting error.                 - Bertolt Brecht

[U-Boot] [PATCH] tools: default image: use ih_size for checking data size

[Jonas Gorski]

On Fri, 03 May 2013 17:04:44 +0200
Wolfgang Denk <wd@denx.de> wrote:

> Dear Jonas Gorski,
> 
> In message <1367581025-26740-1-git-send-email-jogo@openwrt.org> you
> wrote:
> > Common image usage is uImage + appended rootfs, so the the uImage
> > data
> 
> No, this is not at all "common usage".  Actually this something you
> should never do.
> 
> > is only part of the total image. So read out and use the header's
> > ih_size field instead of the total file size.
> > 
> > To prevent reading over the end of the buffer, check that the image
> > file is big enough to contain the data before calculating its
> > checksum.
> > 
> > Before:
> > ~# mkimage -l dir665_fw_100NA.bin
> > mkimage: ERROR: "dir665_fw_100NA/dir665_fw_100NA.bin" has corrupted
> > data!
> 
> Sorry, I don't know how you create your image files, but you must be
> doing something fundamentally wrong.  If mkimage reports a bug here,
> it is probably right.  If the actual payload size is different from
> the content of the  ih_size  field, then your image _is_ broken.

That what else for is the ih_size field then except to say what the
actual datasize is? mkuimage also sets this fields to the
correct size.

And this isn't from me, but this is how most firmware images are
created for devices using U-Boot, i.e. uImage packed kernel + appended
rootfs. Also U-Boot itself only cares for the first ih_size bytes of the
image and not for any "garbage" that might be behind it:

int image_check_dcrc(const image_header_t *hdr)
{
	...
	ulong len = image_get_data_size(hdr);
	...
	ulong dcrc = crc32_wd(...,len,...);
	...
}

static inline uint32_t image_get_data_size(const image_header_t *hdr)
{
	return image_get_size(hdr); /* == hdr->ih_size */
}

It checks the crc of the first ih_size bytes after the image_header -
and my change changes mkimage to mirror that behaviour.
It still reports data errors if the checksum is wrong for the data
actually specified by the image header, but now it actually respects
the length of the data field.

One might even argue that it is now *more* correct, because I can
can easily construct you an image that will pass the "old" mkimage, but
will be rejected by U-Boot, by creating a different length payload
that has the same crc32 as expected by the header.


Best regards
Jonas Gorski

[U-Boot] [PATCH] tools: default image: use ih_size for checking data size

[Wolfgang Denk]

Dear Jonas Gorski,

In message <20130503174205.00000070@unknown> you wrote:
>
> > Sorry, I don't know how you create your image files, but you must be
> > doing something fundamentally wrong.  If mkimage reports a bug here,
> > it is probably right.  If the actual payload size is different from
> > the content of the  ih_size  field, then your image _is_ broken.
> 
> That what else for is the ih_size field then except to say what the
> actual datasize is? mkuimage also sets this fields to the
> correct size.

It's exactly for this very purpose, and allows for consistency
checking.  You run into errors, becuase your images are not correct.
For plain legacy image format (i. e. we are not talking about
multi-file image format here), header size (= 64 bytes) plus the
content of the ih_size  field will give the total file size of the
image.  If this condition is not met, then your image is broken.

> And this isn't from me, but this is how most firmware images are

Define "most".

> created for devices using U-Boot, i.e. uImage packed kernel + appended
> rootfs. Also U-Boot itself only cares for the first ih_size bytes of the
> image and not for any "garbage" that might be behind it:

Yes, because in U-Boot we have no notion of "files" and thus no
indication where an image ends.  Otherwise such a check would be
there.

> It checks the crc of the first ih_size bytes after the image_header -
> and my change changes mkimage to mirror that behaviour.

But this is wrong.

> It still reports data errors if the checksum is wrong for the data
> actually specified by the image header, but now it actually respects
> the length of the data field.

Let me repeat: a valid image will have sizeof(struct image_header)
plus ih_size == file size.  If this condition is not true, then your
image is broken.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"...this does not mean that some of us should not want, in  a  rather
dispassionate sort of way, to put a bullet through csh's head."
                   - Larry Wall in <1992Aug6.221512.5963@netlabs.com>

[U-Boot] [PATCH] tools: default image: use ih_size for checking data size

[Jonas Gorski]

On Fri, 03 May 2013 17:54:25 +0200
Wolfgang Denk <wd@denx.de> wrote:

> Dear Jonas Gorski,
> 
> In message <20130503174205.00000070@unknown> you wrote:
> >
> > > Sorry, I don't know how you create your image files, but you must
> > > be doing something fundamentally wrong.  If mkimage reports a bug
> > > here, it is probably right.  If the actual payload size is
> > > different from the content of the  ih_size  field, then your
> > > image _is_ broken.
> > 
> > That what else for is the ih_size field then except to say what the
> > actual datasize is? mkuimage also sets this fields to the
> > correct size.
> 
> It's exactly for this very purpose, and allows for consistency
> checking.  You run into errors, becuase your images are not correct.
> For plain legacy image format (i. e. we are not talking about
> multi-file image format here), header size (= 64 bytes) plus the
> content of the ih_size  field will give the total file size of the
> image.  If this condition is not met, then your image is broken.
> 
> > And this isn't from me, but this is how most firmware images are
> 
> Define "most".

Most images I have seen for kirkwood, ar71xx, ralink, and I'm sure
there are more.

> > created for devices using U-Boot, i.e. uImage packed kernel +
> > appended rootfs. Also U-Boot itself only cares for the first
> > ih_size bytes of the image and not for any "garbage" that might be
> > behind it:
> 
> Yes, because in U-Boot we have no notion of "files" and thus no
> indication where an image ends.  Otherwise such a check would be
> there.
> 
> > It checks the crc of the first ih_size bytes after the image_header
> > - and my change changes mkimage to mirror that behaviour.
> 
> But this is wrong.

But that's what U-Boot does. The quoted code is taken straight from
git. My impression was that if U-Boot accepts this image, then mkimage
should, too.

> > It still reports data errors if the checksum is wrong for the data
> > actually specified by the image header, but now it actually respects
> > the length of the data field.
> 
> Let me repeat: a valid image will have sizeof(struct image_header)
> plus ih_size == file size.  If this condition is not true, then your
> image is broken.

Okay, after a bit of IRC discussion the following compromise:

If the header is correct, and the crc is correct for the ih_size data,
then assume everything is fine, but print out a warning if the file
length does not match the header lengh + ih_size.

My typical use case would be looking at vendor images or flashdumps and
check if they have a valid uimage header. With this patch I can do
it easily with the imagefile/flashdump, without it I would need to parse
the header first manually, find the ih_size field, truncate the file to
the expected size, and then hope the header is valid.


Best regards,
Jonas Gorski

[U-Boot] [PATCH] tools: default image: use ih_size for checking data size

[Wolfgang Denk]

Dear Jonas Gorski,

In message <20130503183128.00000900@unknown> you wrote:
>
> > > And this isn't from me, but this is how most firmware images are
> > 
> > Define "most".
> 
> Most images I have seen for kirkwood, ar71xx, ralink, and I'm sure
> there are more.

Maybe these do - but these are a tiny, virtually invisible minority of
the platforms supported in mainline.

> > Yes, because in U-Boot we have no notion of "files" and thus no
> > indication where an image ends.  Otherwise such a check would be
> > there.
> > 
> > > It checks the crc of the first ih_size bytes after the image_header
> > > - and my change changes mkimage to mirror that behaviour.
> > 
> > But this is wrong.
> 
> But that's what U-Boot does. The quoted code is taken straight from
> git. My impression was that if U-Boot accepts this image, then mkimage
> should, too.

I explained this before: in U-Boot we have no way to figure out the
total size (or "file size") as there is no such concept as files.
For U-Boot, an image is only identified by it's start address in
memory, but we have no length information available, so how could we
check it?

> Okay, after a bit of IRC discussion the following compromise:
> 
> If the header is correct, and the crc is correct for the ih_size data,
> then assume everything is fine, but print out a warning if the file
> length does not match the header lengh + ih_size.

No, this was not exactly what we discussed.  I wrote:

| I suggest you change mkimage such that it will display the (valid)
| image data, but then bail out with a "NNN bytes excess data present"
| error message.  Note that mkimage must return an error code (EFAILURE)
| in this case.

So no warning, but an error, please.  And only for excess lenght.

If the length is too short, we are really in trouble, especially if
the CRC still matches.  This should never happen.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If there was anything that depressed him more than his own  cynicism,
it was that quite often it still wasn't as cynical as real life.
                                 - Terry Pratchett, _Guards! Guards!_

[U-Boot] [PATCH] tools: default image: use ih_size for checking data size

[Peter Korsgaard]

>>>>> "Jonas" == Jonas Gorski <jogo@openwrt.org> writes:

 Jonas> Common image usage is uImage + appended rootfs, so the the uImage data
 Jonas> is only part of the total image. So read out and use the header's
 Jonas> ih_size field instead of the total file size.

 Jonas> To prevent reading over the end of the buffer, check that the image file
 Jonas> is big enough to contain the data before calculating its checksum.

 Jonas> Before:
 Jonas> ~# mkimage -l dir665_fw_100NA.bin
 Jonas> mkimage: ERROR: "dir665_fw_100NA/dir665_fw_100NA.bin" has corrupted data!

A related usecase is checking the version info of whatever is written to
flash - E.G.

mkimage -l /dev/mtdN

But that already fails as stat returns st_size=0 for devices.

-- 
Bye, Peter Korsgaard