Add this discussion to denx mailing list. > -----Original Message----- > From: Simon Glass <s...@chromium.org> > Sent: Saturday, April 24, 2021 3:29 AM > To: Lim, Elly Siew Chin <elly.siew.chin....@intel.com> > Cc: Alex G. <mr.nuke...@gmail.com>; Tan, Ley Foon > <ley.foon....@intel.com>; Gan, Yau Wai <yau.wai....@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....@intel.com> > wrote: > > > > > > > > > -----Original Message----- > > > From: Lim, Elly Siew Chin > > > Sent: Saturday, April 24, 2021 1:17 AM > > > To: Alex G. <mr.nuke...@gmail.com>; Simon Glass <s...@chromium.org> > > > Cc: Tan, Ley Foon <ley.foon....@intel.com>; Gan, Yau Wai > > > <yau.wai....@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...@gmail.com> > > > > Sent: Saturday, April 24, 2021 1:07 AM > > > > To: Lim, Elly Siew Chin <elly.siew.chin....@intel.com>; Simon > > > > Glass <s...@chromium.org> > > > > Cc: Tan, Ley Foon <ley.foon....@intel.com>; Gan, Yau Wai > > > > <yau.wai....@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 > > > > > https://source.denx.de/u-boot/u-boot/- > > > > /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 > >