* U-Boot "lib: Add support for ECDSA image signing" commit breaks socfpga_*_atf_defconfig compilation
[not found] ` <CAPnjgZ0wyhaeG6jOgB6A+XRUquZ5mGsbouKwh4pJD1T1fcqytA@mail.gmail.com>
@ 2021-04-24 7:43 ` Lim, Elly Siew Chin
2021-05-31 19:01 ` Alex G.
0 siblings, 1 reply; 2+ messages in thread
From: Lim, Elly Siew Chin @ 2021-04-24 7:43 UTC (permalink / raw)
To: u-boot
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
> > > > > 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
> >
^ permalink raw reply [flat|nested] 2+ messages in thread