[PATCH u-boot 1/2] tools: default_image: Verify header size

[PATCH u-boot 1/2] tools: default_image: Verify header size

[Pali Rohár]

Before reading image header, verify that image size is at least size of
the image header.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 tools/default_image.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/default_image.c b/tools/default_image.c
index 4a067e65862e..4aa9a33241cb 100644
--- a/tools/default_image.c
+++ b/tools/default_image.c
@@ -49,6 +49,12 @@ static int image_verify_header(unsigned char *ptr, int image_size,
 	struct legacy_img_hdr header;
 	struct legacy_img_hdr *hdr = &header;
 
+	if (image_size < sizeof(struct legacy_img_hdr)) {
+		debug("%s: Bad image size: \"%s\" is no valid image\n",
+		      params->cmdname, params->imagefile);
+		return -FDT_ERR_BADSTRUCTURE;
+	}
+
 	/*
 	 * create copy of header so that we can blank out the
 	 * checksum field for checking - this can't be done
-- 
2.20.1

[PATCH u-boot 2/2] tools: default_image: Accept images with padding

[Pali Rohár]

If image file is stored on flash partition then it contains padding, which
is not part of the image itself. Image data size is stored in the image
header. So use image size from the header instead of expecting that total
image file size is size of the header plus size of the image data. This
allows dumpimage to parse image files with padding (e.g. dumped from flash
partition).

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 tools/default_image.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/default_image.c b/tools/default_image.c
index 4aa9a33241cb..0996e1dfe9c8 100644
--- a/tools/default_image.c
+++ b/tools/default_image.c
@@ -81,7 +81,13 @@ static int image_verify_header(unsigned char *ptr, int image_size,
 	}
 
 	data = (const unsigned char *)ptr + sizeof(struct legacy_img_hdr);
-	len  = image_size - sizeof(struct legacy_img_hdr);
+	len = image_get_data_size(hdr);
+
+	if (image_size - sizeof(struct legacy_img_hdr) < len) {
+		debug("%s: Bad image size: \"%s\" is no valid image\n",
+		      params->cmdname, params->imagefile);
+		return -FDT_ERR_BADSTRUCTURE;
+	}
 
 	checksum = be32_to_cpu(hdr->ih_dcrc);
 	if (crc32(0, data, len) != checksum) {
-- 
2.20.1

Re: [PATCH u-boot 1/2] tools: default_image: Verify header size

[Simon Glass]

On Sun, 29 Jan 2023 at 09:44, Pali Rohár <pali@kernel.org> wrote:
>
> Before reading image header, verify that image size is at least size of
> the image header.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  tools/default_image.c | 6 ++++++
>  1 file changed, 6 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH u-boot 2/2] tools: default_image: Accept images with padding

[Simon Glass]

On Sun, 29 Jan 2023 at 09:44, Pali Rohár <pali@kernel.org> wrote:
>
> If image file is stored on flash partition then it contains padding, which
> is not part of the image itself. Image data size is stored in the image
> header. So use image size from the header instead of expecting that total
> image file size is size of the header plus size of the image data. This
> allows dumpimage to parse image files with padding (e.g. dumped from flash
> partition).
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  tools/default_image.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH u-boot 2/2] tools: default_image: Accept images with padding

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 695 bytes --]

On Sun, Jan 29, 2023 at 05:44:11PM +0100, Pali Rohár wrote:

> If image file is stored on flash partition then it contains padding, which
> is not part of the image itself. Image data size is stored in the image
> header. So use image size from the header instead of expecting that total
> image file size is size of the header plus size of the image data. This
> allows dumpimage to parse image files with padding (e.g. dumped from flash
> partition).
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Simon Glass <sjg@chromium.org>

This breaks imx6q_bosch_acc imx6dl_mamoj as:
./tools/mkimage: Failed to verify header of u-boot-ivt.img

now happens.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH u-boot 2/2] tools: default_image: Accept images with padding

[Pali Rohár]

On Monday 06 February 2023 14:43:07 Tom Rini wrote:
> On Sun, Jan 29, 2023 at 05:44:11PM +0100, Pali Rohár wrote:
> 
> > If image file is stored on flash partition then it contains padding, which
> > is not part of the image itself. Image data size is stored in the image
> > header. So use image size from the header instead of expecting that total
> > image file size is size of the header plus size of the image data. This
> > allows dumpimage to parse image files with padding (e.g. dumped from flash
> > partition).
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> This breaks imx6q_bosch_acc imx6dl_mamoj as:
> ./tools/mkimage: Failed to verify header of u-boot-ivt.img
> 
> now happens.

Ah :-( That is because IH_TYPE_FIRMWARE_IVT support was hacked into
default_image.c and its format is not compatible with other formats
supported but default_image.c

I tested following patch with imx6q_bosch_acc_defconfig and it worked:

diff --git a/tools/default_image.c b/tools/default_image.c
index 0996e1dfe9c8..e0e234a1e8f4 100644
--- a/tools/default_image.c
+++ b/tools/default_image.c
@@ -83,6 +83,10 @@ static int image_verify_header(unsigned char *ptr, int image_size,
 	data = (const unsigned char *)ptr + sizeof(struct legacy_img_hdr);
 	len = image_get_data_size(hdr);
 
+	if (image_get_type(hdr) == IH_TYPE_FIRMWARE_IVT)
+		/* Add size of CSF minus IVT */
+		len -= 0x2060 - sizeof(flash_header_v2_t);
+
 	if (image_size - sizeof(struct legacy_img_hdr) < len) {
 		debug("%s: Bad image size: \"%s\" is no valid image\n",
 		      params->cmdname, params->imagefile);

That is why validation functions _must_ always be implemented.

Feel free to amend this chunk into patch 2/2 and check if it works also
for you.

And looking at the image_extract_subimage() code in default_image.c and
it is also broken for IH_TYPE_FIRMWARE_IVT. Well, I'm not going to
fix image_extract_subimage() as it was broken before and nobody spotted
it yet.

Re: [PATCH u-boot 2/2] tools: default_image: Accept images with padding

[Simon Glass]

Hi Tom,

On Mon, 6 Feb 2023 at 12:43, Tom Rini <trini@konsulko.com> wrote:
>
> On Sun, Jan 29, 2023 at 05:44:11PM +0100, Pali Rohár wrote:
>
> > If image file is stored on flash partition then it contains padding, which
> > is not part of the image itself. Image data size is stored in the image
> > header. So use image size from the header instead of expecting that total
> > image file size is size of the header plus size of the image data. This
> > allows dumpimage to parse image files with padding (e.g. dumped from flash
> > partition).
> >
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Reviewed-by: Simon Glass <sjg@chromium.org>
>
> This breaks imx6q_bosch_acc imx6dl_mamoj as:
> ./tools/mkimage: Failed to verify header of u-boot-ivt.img
>
> now happens.

I know these failures are a pain, but it is great to see the testing
having such a big impact!

- Simon

Re: [PATCH u-boot 1/2] tools: default_image: Verify header size

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 306 bytes --]

On Sun, Jan 29, 2023 at 05:44:10PM +0100, Pali Rohár wrote:

> Before reading image header, verify that image size is at least size of
> the image header.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH u-boot 2/2] tools: default_image: Accept images with padding

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2308 bytes --]

On Mon, Feb 06, 2023 at 10:47:43PM +0100, Pali Rohár wrote:
> On Monday 06 February 2023 14:43:07 Tom Rini wrote:
> > On Sun, Jan 29, 2023 at 05:44:11PM +0100, Pali Rohár wrote:
> > 
> > > If image file is stored on flash partition then it contains padding, which
> > > is not part of the image itself. Image data size is stored in the image
> > > header. So use image size from the header instead of expecting that total
> > > image file size is size of the header plus size of the image data. This
> > > allows dumpimage to parse image files with padding (e.g. dumped from flash
> > > partition).
> > > 
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > 
> > This breaks imx6q_bosch_acc imx6dl_mamoj as:
> > ./tools/mkimage: Failed to verify header of u-boot-ivt.img
> > 
> > now happens.
> 
> Ah :-( That is because IH_TYPE_FIRMWARE_IVT support was hacked into
> default_image.c and its format is not compatible with other formats
> supported but default_image.c
> 
> I tested following patch with imx6q_bosch_acc_defconfig and it worked:
> 
> diff --git a/tools/default_image.c b/tools/default_image.c
> index 0996e1dfe9c8..e0e234a1e8f4 100644
> --- a/tools/default_image.c
> +++ b/tools/default_image.c
> @@ -83,6 +83,10 @@ static int image_verify_header(unsigned char *ptr, int image_size,
>  	data = (const unsigned char *)ptr + sizeof(struct legacy_img_hdr);
>  	len = image_get_data_size(hdr);
>  
> +	if (image_get_type(hdr) == IH_TYPE_FIRMWARE_IVT)
> +		/* Add size of CSF minus IVT */
> +		len -= 0x2060 - sizeof(flash_header_v2_t);
> +
>  	if (image_size - sizeof(struct legacy_img_hdr) < len) {
>  		debug("%s: Bad image size: \"%s\" is no valid image\n",
>  		      params->cmdname, params->imagefile);
> 
> That is why validation functions _must_ always be implemented.
> 
> Feel free to amend this chunk into patch 2/2 and check if it works also
> for you.
> 
> And looking at the image_extract_subimage() code in default_image.c and
> it is also broken for IH_TYPE_FIRMWARE_IVT. Well, I'm not going to
> fix image_extract_subimage() as it was broken before and nobody spotted
> it yet.

With this included now, the original patch is applied to u-boot/master
now, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]