RE: U-Boot "lib: Add support for ECDSA image signing" commit breaks socfpga_*_atf_defconfig compilation

24 Apr 2021

      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