spl: implement CRC check on U-Boot uImage
PATCH 1/1 spl: implement CRC check on U-Boot uImage
SPL currently does not check uImage CRCs when loading U-Boot.
This patch adds checking the uImage CRC when SPL loads U-Boot. It does this by reusing the existing config option SPL_CRC32_SUPPORT to allow leaving out the CRC check on boards where the additional code size or boot time is a problem (adding the CRC check currently adds ~1.4 kByte to flash).
The SPL_CRC32_SUPPORT config option now gets enabled by default if SPL support for legacy images is enabled to check the CRC on all boards that don't actively take countermeasures.
---
common/spl/Kconfig | 12 ++++++------ common/spl/spl.c | 30 +++++++++++++++++++++++++++++- include/spl.h | 5 +++++ 3 files changed, 40 insertions(+), 7 deletions(-)
common/spl/Kconfig +6 -6
@@ -207,13 +207,13 @@ config SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION_TYPE config SPL_CRC32_SUPPORT bool "Support CRC32" - depends on SPL_FIT + default y if SPL_LEGACY_IMAGE_SUPPORT help - Enable this to support CRC32 in FIT images within SPL. This is a - 32-bit checksum value that can be used to verify images. This is - the least secure type of checksum, suitable for detected - accidental image corruption. For secure applications you should - consider SHA1 or SHA256. + Enable this to support CRC32 in uImages or FIT images within SPL. + This is a 32-bit checksum value that can be used to verify images. + For FIT images, this is the least secure type of checksum, suitable + for detected accidental image corruption. For secure applications you + should consider SHA1 or SHA256. config SPL_MD5_SUPPORT bool "Support MD5"
common/spl/spl.c +29 -1
@@ -236,6 +236,14 @@ int spl_parse_image_header(struct spl_image_info *spl_image,#ifdef CONFIG_SPL_LEGACY_IMAGE_SUPPORT u32 header_size =sizeof (struct image_header ); +if (CONFIG_IS_ENABLED (CRC32_SUPPORT )) { +/* check uImage header CRC */ +if (!image_check_hcrc (header)) { +puts ("SPL: Image header CRC check failed!\n" ); +return -EINVAL ; + } + } +if (spl_image->flags &SPL_COPY_PAYLOAD_ONLY ) {/* * On some system (e.g. powerpc), the load-address and
@@ -253,6 +261,13 @@ int spl_parse_image_header(struct spl_image_info *spl_image, spl_image->size =image_get_data_size (header) + header_size; } +#if CONFIG_IS_ENABLED (CRC32_SUPPORT ) +/* store uImage data length and CRC to check later */ + spl_image->dcrc_data =image_get_load (header); + spl_image->dcrc_length =image_get_data_size (header); + spl_image->dcrc =image_get_dcrc (header); +#endif + spl_image->os =image_get_os (header); spl_image->name =image_get_name (header); debug("spl: payload image: %32s load addr: 0x%lx size: %d\n",
@@ -424,12 +439,25 @@ static struct spl_image_loader *spl_ll_find_loader(uint boot_device)static int spl_load_image (struct spl_image_info *spl_image,struct spl_image_loader *loader) { +int ret;struct spl_boot_device bootdev; bootdev.boot_device = loader->boot_device; bootdev.boot_device_name =NULL ; -return loader->load_image (spl_image, &bootdev); + ret = loader->load_image (spl_image, &bootdev); +#if CONFIG_IS_ENABLED (CRC32_SUPPORT ) +if (!ret && spl_image->dcrc_length) { +/* check data crc */ +ulong dcrc =crc32_wd (0 , (unsigned char *)spl_image->dcrc_data, + spl_image->dcrc_length,CHUNKSZ_CRC32 ); +if (dcrc != spl_image->dcrc) { +puts ("SPL: Image data CRC check failed!\n" ); + ret = -EINVAL ; + } + } +#endif +return ret; }/**
include/spl.h +5 -0
@@ -33,6 +33,11 @@ struct spl_image_info {u32 size;u32 flags;void *arg; +#if CONFIG_IS_ENABLED (CRC32_SUPPORT ) +ulong dcrc_data; +ulong dcrc_length; +ulong dcrc; +#endif };/*
Signature · 1 line
2.17.1
PATCH 1/1 spl: implement CRC check on U-Boot uImage
Quoted 13 lines · Simon Goldschmidt
SPL currently does not check uImage CRCs when loading U-Boot.
This patch adds checking the uImage CRC when SPL loads U-Boot. It does this by reusing the existing config option
SPL_CRC32_SUPPORTto allow leaving out the CRC check on boards where the additional code size or boot time is a problem (adding the CRC check currently adds ~1.4 kByte to flash).The
SPL_CRC32_SUPPORTconfig option now gets enabled by default if SPL support for legacy images is enabled to check the CRC on all boards that don't actively take countermeasures.
Quoted 7 lines
Signed-off-by Simon Goldschmidt <[email protected]>---
common/spl/Kconfig | 12 ++++++------ common/spl/spl.c | 30 +++++++++++++++++++++++++++++- include/spl.h | 5 +++++ 3 files changed, 40 insertions(+), 7 deletions(-)
PATCH 1/1 spl: implement CRC check on U-Boot uImage
In reply to Simon GoldschmidtDear Simon,
Quoted 11 lines · In message <[email protected]> you wrote:
SPL currently does not check uImage CRCs when loading U-Boot.
This patch adds checking the uImage CRC when SPL loads U-Boot. It does this by reusing the existing config option
SPL_CRC32_SUPPORTto allow leaving out the CRC check on boards where the additional code size or boot time is a problem (adding the CRC check currently adds ~1.4 kByte to flash).The
SPL_CRC32_SUPPORTconfig option now gets enabled by default if SPL support for legacy images is enabled to check the CRC on all boards that don't actively take countermeasures.
The new features is definitive useful, but I have a few comments.
First, I am not sure if mandatorily binding this feature to SPL_CRC32_SUPPORT is a good idea?
Might there not be reasons to have SPL_CRC32_SUPPORT enabled but still wanting to keep the boot time minimal, and to intentionally NOT check the CRC (similar to what setting "verify=no" would do)? [This is commonly used when trading boot speed for reliability/securi- ty...]
Second, may I please ask you to change the code such that the CRC is checked "in-flight", i. e. while reading from the boot device and writing it to the RAM, instead of re-reading the image from RAM? Again, the reason is boot time. We will not reach a zero-copy boot, but at least we should avoid an additional reading of what we just wrote. Yes, this would catch memory corruptions in RAM, but the whole operation of U-Boot is based on the assumption that RAM is working reliable and error-free. [If you drop this assumption, you cannot guaranteee anything, i. e. you would have to fear that the memory content might even get corrupted when reading it for the CRC calculation - if you have such concerns, you should switch to hardware with ECC RAM.]
Thanks!
Best regards,
Wolfgang Denk
Signature · 4 lines
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk 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 Anything free is worth what you pay for it.
PATCH 1/1 spl: implement CRC check on U-Boot uImage
Quoted 16 lines · Wolfgang Denk
Dear Simon,
Quoted 11 lines · In message <[email protected]> you wrote:
SPL currently does not check uImage CRCs when loading U-Boot.
This patch adds checking the uImage CRC when SPL loads U-Boot. It does this by reusing the existing config option
SPL_CRC32_SUPPORTto allow leaving out the CRC check on boards where the additional code size or boot time is a problem (adding the CRC check currently adds ~1.4 kByte to flash).The
SPL_CRC32_SUPPORTconfig option now gets enabled by default if SPL support for legacy images is enabled to check the CRC on all boards that don't actively take countermeasures.The new features is definitive useful, but I have a few comments.
Quoted 8 lines
First, I am not sure if mandatorily binding this feature to
SPL_CRC32_SUPPORTis a good idea?Might there not be reasons to have
SPL_CRC32_SUPPORTenabled but still wanting to keep the boot time minimal, and to intentionally NOT check the CRC (similar to what setting "verify=no" would do)? [This is commonly used when trading boot speed for reliability/securi- ty...]
I also thought about that. I voted against to have less config options. But sure, I can add a config option for this in v2.
Quoted 4 lines
Second, may I please ask you to change the code such that the CRC is checked "in-flight", i. e. while reading from the boot device and writing it to the RAM, instead of re-reading the image from RAM? Again, the reason is boot time. We will not reach a zero-copy boot,
Quoted 8 lines
but at least we should avoid an additional reading of what we just wrote. Yes, this would catch memory corruptions in RAM, but the whole operation of U-Boot is based on the assumption that RAM is working reliable and error-free. [If you drop this assumption, you cannot guaranteee anything, i. e. you would have to fear that the memory content might even get corrupted when reading it for the CRC calculation - if you have such concerns, you should switch to hardware with ECC RAM.]
Hmm, while this is a good idea regarding speed, I'm not sure this can be done easily with the current infrastructure. The data is copied down at the driver somewhere. That would mean we'd have to change every driver interface and every driver to allow a callback where we'd update the CRC.
Is this really a rework that is worth it? Because it's also not done like that for loading other uImages (Kernel, fdt, ramdisk) when U-Boot loads the OS, or is it?
Regards,
Simon
Quoted 7 lines
Thanks!
Best regards,
Wolfgang Denk
PATCH 1/1 spl: implement CRC check on U-Boot uImage
Dear Simon,
Quoted 2 lines · In message <[email protected]> you wrote:
I also thought about that. I voted against to have less config options.
Yes, I can understand this. Actually I have to admit that I cannot think of a god reason to have CRC support in SPL for other purposes, but who knows...
Quoted 1 line
But sure, I can add a config option for this in v2.
Thanks.
Quoted 4 lines
Hmm, while this is a good idea regarding speed, I'm not sure this can be done easily with the current infrastructure. The data is copied down at the driver somewhere. That would mean we'd have to change every driver interface and every driver to allow a callback where we'd update the CRC.
I would hope it became easier now since we have a unified interface, but I admit I didn't look at the implementation details at all. In any case, this requires additional though, as the "loader" would also need to understand the structure of the file - for legacy uImage format it would have to know where to find the header checksum andverify the header, then where to find the image checksum(s) and length(s) and verify the payload(s).
Quoted 3 lines
Is this really a rework that is worth it? Because it's also not done like that for loading other uImages (Kernel, fdt, ramdisk) when U-Boot loads the OS, or is it?
Yesno. Image handling has been reworked frequently and I fear much of the original design ideas have not been preserved - either by lack of knowledge or by intention.
The idea was that there are two stages for booting: 1) loading some image to RAM, and 2) booting it.
The loading part would not care about the content - it loads a "file", i. e. an unstructured byte stream.
The booting part (i. e. the bootm command) would understand the image format and handle the checking. It would compute (and verify) the CRC while uncompressing the kernel and writing the uncompressed data to the memory region specified by the "load address" given in the image header, so no _extra_ reading was needed.
Best regards,
Wolfgang Denk
Signature · 5 lines
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk 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 two witches would watch two watches, which witch would watch which watch?
PATCH 1/1 spl: implement CRC check on U-Boot uImage
Quoted 33 lines · Wolfgang Denk
Dear Simon,
Quoted 1 line · In message <[email protected]> you wrote:
I also thought about that. I voted against to have less config options.
Yes, I can understand this. Actually I have to admit that I cannot think of a god reason to have CRC support in SPL for other purposes, but who knows...
Quoted 1 line
But sure, I can add a config option for this in v2.
Thanks.
Quoted 4 lines
Hmm, while this is a good idea regarding speed, I'm not sure this can be done easily with the current infrastructure. The data is copied down at the driver somewhere. That would mean we'd have to change every driver interface and every driver to allow a callback where we'd update the CRC.
I would hope it became easier now since we have a unified interface, but I admit I didn't look at the implementation details at all. In any case, this requires additional though, as the "loader" would also need to understand the structure of the file - for legacy uImage format it would have to know where to find the header checksum andverify the header, then where to find the image checksum(s) and length(s) and verify the payload(s).
Quoted 3 lines
Is this really a rework that is worth it? Because it's also not done like that for loading other uImages (Kernel, fdt, ramdisk) when U-Boot loads the OS, or is it?
Yesno. Image handling has been reworked frequently and I fear much of the original design ideas have not been preserved - either by lack of knowledge or by intention.
The idea was that there are two stages for booting: 1) loading some image to RAM, and 2) booting it.
Quoted 8 lines
The loading part would not care about the content - it loads a "file", i. e. an unstructured byte stream.
The booting part (i. e. the bootm command) would understand the image format and handle the checking. It would compute (and verify) the CRC while uncompressing the kernel and writing the uncompressed data to the memory region specified by the "load address" given in the image header, so no _extra_ reading was needed.
That still sounds like an interesting feature! Although it might be difficult to implement with all the different unzip algorithms we have to add a CRC check...
But I do not see it in the current U-Boot sources. Can you point me to a git commit in the past where I can have a look at how this was done?
Regards,
Simon
PATCH 1/1 spl: implement CRC check on U-Boot uImage
Dear Simon,
Quoted 7 lines · In message <[email protected]> you wrote:
That still sounds like an interesting feature! Although it might be difficult to implement with all the different unzip algorithms we have to add a CRC check...
But I do not see it in the current U-Boot sources. Can you point me to a git commit in the past where I can have a look at how this was done?
Hm... I can't find it either...
I checked v2008.10, v1.3.0 and even LABEL_2002_11_05_0120, but thay all have two passes: first verifying the CRC, then uncompressing/loading to the target address.
Sorry for the false alarm - I was sure we had this, but apparently it was just one of the old ideas I had in mind for boot time optimizations...
Best regards,
Wolfgang Denk
Signature · 5 lines
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk 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 There's no sense in being precise when you don't even know what you're talking about. -- John von Neumann
PATCH 1/1 spl: implement CRC check on U-Boot uImage
Am Mi., 28. Nov. 2018, 10:44 hat Wolfgang Denk <[email protected]> geschrieben:
Quoted 13 lines
Dear Simon,
Quoted 7 lines · In message <[email protected]> you wrote:
That still sounds like an interesting feature! Although it might be difficult to implement with all the different unzip algorithms we have to add a CRC check...
But I do not see it in the current U-Boot sources. Can you point me to a git commit in the past where I can have a look at how this was done?
Hm... I can't find it either...
Quoted 8 lines
I checked v2008.10, v1.3.0 and even
LABEL_2002_11_05_0120, but thay all have two passes: first verifying the CRC, then uncompressing/loading to the target address.Sorry for the false alarm - I was sure we had this, but apparently it was just one of the old ideas I had in mind for boot time optimizations...
Ok, thanks for checking. Still seems like a good idea especially for SPL where cache is often not enabled.
Regards,
Simon
PATCH 1/1 spl: implement CRC check on U-Boot uImage
In reply to Wolfgang DenkQuoted 9 lines · Wolfgang Denk
Dear Simon,
Quoted 2 lines · In message <[email protected]> you wrote:
I also thought about that. I voted against to have less config options.
Yes, I can understand this. Actually I have to admit that I cannot think of a god reason to have CRC support in SPL for other purposes, but who knows...
Ah, so I think today we only have CRC32 support for when we also have FIT images in SPL enabled.
Signature · 8 lines
Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181128/5e16c8ce/attachment.sig>