RE: U-Boot "lib: Add support for ECDSA image signing" commit breaks socfpga_*_atf_defconfig compilation
Add this discussion to denx mailing list.
-----Original Message----- From: Simon Glass sjg@chromium.org Sent: Saturday, April 24, 2021 3:29 AM To: Lim, Elly Siew Chin elly.siew.chin.lim@intel.com Cc: Alex G. mr.nuke.me@gmail.com; Tan, Ley Foon ley.foon.tan@intel.com; Gan, Yau Wai yau.wai.gan@intel.com Subject: Re: U-Boot "lib: Add support for ECDSA image signing" commit breaks socfpga_*_atf_defconfig compilation
Hi,
On Sat, 24 Apr 2021 at 05:30, Lim, Elly Siew Chin elly.siew.chin.lim@intel.com wrote:
-----Original Message----- From: Lim, Elly Siew Chin Sent: Saturday, April 24, 2021 1:17 AM To: Alex G. mr.nuke.me@gmail.com; Simon Glass sjg@chromium.org Cc: Tan, Ley Foon ley.foon.tan@intel.com; Gan, Yau Wai yau.wai.gan@intel.com Subject: RE: U-Boot "lib: Add support for ECDSA image signing" commit breaks socfpga_*_atf_defconfig compilation
Hi Alex,
-----Original Message----- From: Alex G. mr.nuke.me@gmail.com Sent: Saturday, April 24, 2021 1:07 AM To: Lim, Elly Siew Chin elly.siew.chin.lim@intel.com; Simon Glass sjg@chromium.org Cc: Tan, Ley Foon ley.foon.tan@intel.com; Gan, Yau Wai yau.wai.gan@intel.com Subject: Re: U-Boot "lib: Add support for ECDSA image signing" commit breaks socfpga_*_atf_defconfig compilation
On 4/23/21 11:59 AM, Lim, Elly Siew Chin wrote:
Hi Alexandru, Simon,
/commit/ed6c9e0b6668a05d62f5d1b7
5aecaf246ba51042 <https://source.denx.de/u-boot/u-boot/-
/commit/ed6c9e0b6668a05d62f5d1b
75aecaf246ba51042> breaks Intel socfpga_*_atf_defconfig U-Boot compilation due to openssl.
This commit compile "u-boot/lib/ecdsa/ecdsa_libcrypto.c" based on CONFIG_FIT_SIGNATURE, and ecdsa_libcrypto.c requires openssl.
/ecdsa_libcrypto.c:/
/#include <openssl/ssl.h>/
/#include <openssl/ec.h>/
/#include <openssl/bn.h>/
However, in our use case (Intel socfpga_*_atf_defconfig) we use CONFIG_FIT_SIGNATURE with CRC32 which does need openssl.
It is my understanding that FIT_SIGNATURE implies cryptographic
support.
In your case, I wouldtry disabling CONFIG_FIT_SIGNATURE and using a hash-1 node with algo value of "crc32".
For example:
/images { kernel-1 { ... hash-1 { algo = "crc32"; value = <autogenerated>; };Alex
CONFIG_FIT_SIGNATURE=y
CONFIG_FIT_SIGNATURE_MAX_SIZE=0x10000000
CONFIG_SPL_FIT_SIGNATURE=y
CONFIG_SPL_CRC32_SUPPORT=y
We hit the following compilation error:
/toolstools//liblib//ecdsaecdsa//ecdsaecdsa--libcrypto.olibcrypto.o:: InIn functionfunction ``prepare_ctxprepare_ctx''::/
/ecdsaecdsa--libcrypto.clibcrypto.c::((..texttext++0x790x79)):: undefinedundefined referencereference toto ``OPENSSL_init_sslOPENSSL_init_ssl''/
Can you please help to help to add a new CONFIG
(CONFIG_ECDSA_SUPPORT)
to gate the compilation of "ecdsa_libcrypto.c" which similar to other algorithm? So that we can only compile when needed instead of force all to support openssl.
Reference: lib/rsa
/obj-$(CONFIG_$(SPL_TPL_)RSA_VERIFY) += rsa-verify.o/
Thanks,
Siew Chin
Our purpose is to use CRC32 as integrity check of binary file in FIT image, so, we need to use FIT_SIGNATURE. And, we did add the hash
node in FIT image.
(this thread should go to the mailing list)
From my reading, fit_image_verify_with_data() still verifies the hash when FIT_SIGNATURE is not enabled. It looks like spl_fit needs to use this same behaviour.
- Simon
Hi Simon,
In our use case, we use FIT image in both first stage (FSBL) and second stage (FSBL) boot loader.
(1) FSBL: SPL -> (u-boot.fit) -> ATF -> U-BOOT PROPER
In this phase, only "FIT_SIGNATURE" does fit image data integrity check. CONFIG_SPL_FIT_SIGNATURE=y CONFIG_SPL_CRC32_SUPPORT=y
common/spl/spl_fit.c if (CONFIG_IS_ENABLED(FIT_SIGNATURE)) { printf("## Checking hash(es) for Image %s ... ", fit_get_name(fit, node, NULL)); if (!fit_image_verify_with_data(fit, node, src, length)) return -EPERM; puts("OK\n"); }
(2) SSBL: U-BOOT PROPER -> (kernel.itb, using bootm command) -> Linux In this phase, the code did support FIT data integrity check without FIT_SIGNATURE. bootm command will call "fit_all_image_verify" in image-fit.c. "fit_all_image_verify" function verify data integrity for all images if hash presented.
cmd/bootm.c #if defined(CONFIG_FIT) case IMAGE_FORMAT_FIT: ... if (!fit_all_image_verify(hdr)) { puts("Bad hash in FIT image!\n"); unmap_sysmem(hdr); return 1; } #endif
I can think of two enhancement to fix this: (1) Add separate CONFIG to gate ECDSA algorithm. This enhancement benefits all use cases. I assume not all user need ECDSA algorithm when FIT_SIGNATURE is used. (2) Enhance spl/spl_fit.c to support verification of data integrity based on hash(es) in FIT image instead of based on FIT_SIGNATURE.
What do you think? If you agree: For (1), can we ask Alex's help to change it? For (2), who will be the right person to change this kind of common code?
Thanks, Siew Chin
Kindly refer to common/spl/spl_fit.c if (CONFIG_IS_ENABLED(FIT_SIGNATURE)) { printf("## Checking hash(es) for Image %s ... ", fit_get_name(fit, node, NULL)); if (!fit_image_verify_with_data(fit, node, src, length)) return -EPERM; puts("OK\n"); }
Results: ## Checking hash(es) for config conf ... OK ## Checking hash(es) for Image atf ... crc32+ OK ## Checking hash(es) for Image uboot ... crc32+ OK ## Checking hash(es) for Image fdt ... crc32+ OK
Thanks, Siew Chin
Besides, kindly refer to function "calculate_hash" in common/image-fit.c,
FIT image did support multiple algorithm, CRC32 is one of it. And, the common practice is each algorithm will gate by its specific CONFIG which allow us to only compile the source files when needed.
int calculate_hash(const void *data, int data_len, const char *algo, uint8_t *value, int *value_len) { if (IMAGE_ENABLE_CRC32 && strcmp(algo, "crc32") == 0) { *((uint32_t *)value) = crc32_wd(0, data, data_len, CHUNKSZ_CRC32); *((uint32_t *)value) = cpu_to_uimage(*((uint32_t *)value)); *value_len = 4; } else if (IMAGE_ENABLE_SHA1 && strcmp(algo, "sha1") == 0) { sha1_csum_wd((unsigned char *)data, data_len, (unsigned char *)value, CHUNKSZ_SHA1); *value_len = 20; } else if (IMAGE_ENABLE_SHA256 && strcmp(algo, "sha256") == 0) { sha256_csum_wd((unsigned char *)data, data_len, (unsigned char *)value, CHUNKSZ_SHA256); *value_len = SHA256_SUM_LEN; } else if (IMAGE_ENABLE_SHA384 && strcmp(algo, "sha384") == 0) { sha384_csum_wd((unsigned char *)data, data_len, (unsigned char *)value, CHUNKSZ_SHA384); *value_len = SHA384_SUM_LEN; } else if (IMAGE_ENABLE_SHA512 && strcmp(algo, "sha512") == 0) { sha512_csum_wd((unsigned char *)data, data_len, (unsigned char *)value, CHUNKSZ_SHA512); *value_len = SHA512_SUM_LEN; } else if (IMAGE_ENABLE_MD5 && strcmp(algo, "md5") == 0) { md5_wd((unsigned char *)data, data_len, value, CHUNKSZ_MD5); *value_len = 16; } else { debug("Unsupported hash alogrithm\n"); return -1; } return 0; }
Thanks, Siew Chin
On 4/24/21 2:43 AM, Lim, Elly Siew Chin wrote:
Add this discussion to denx mailing list.
[snip]
I can think of two enhancement to fix this: (1) Add separate CONFIG to gate ECDSA algorithm. This enhancement benefits all use cases. I assume not all user need ECDSA algorithm when FIT_SIGNATURE is used. (2) Enhance spl/spl_fit.c to support verification of data integrity based on hash(es) in FIT image instead of based on FIT_SIGNATURE.
What do you think? If you agree: For (1), can we ask Alex's help to change it? For (2), who will be the right person to change this kind of common code?
FYI, I proposed a change to decouple OpenSSL from FIT_SIGNATURE [1]
[1] https://patchwork.ozlabs.org/project/uboot/patch/20210524202317.1492578-1-mr...
That would enable you to have FIT_SIGNATURE, but not need OpenSSL support in mkimage.
Alex
participants (2)
-
Alex G. -
Lim, Elly Siew Chin