[PATCH u-boot 1/2] tools: default_image: Verify header size
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
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) {
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
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.
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.
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!
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
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
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!
participants (3)
-
Pali Rohár -
Simon Glass -
Tom Rini