All of lore.kernel.org
 help / color / mirror / Atom feed
* 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

* Re: U-Boot "lib: Add support for ECDSA image signing" commit breaks socfpga_*_atf_defconfig compilation
  2021-04-24  7:43         ` U-Boot "lib: Add support for ECDSA image signing" commit breaks socfpga_*_atf_defconfig compilation Lim, Elly Siew Chin
@ 2021-05-31 19:01           ` Alex G.
  0 siblings, 0 replies; 2+ messages in thread
From: Alex G. @ 2021-05-31 19:01 UTC (permalink / raw)
  To: Lim, Elly Siew Chin, u-boot, Simon Glass
  Cc: Tan, Ley Foon, Gan, Yau Wai, Chee, Tien Fong, See, Chin Liang,
	Westergreen, Dalon


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.nuke.me@gmail.com/

That would enable you to have FIT_SIGNATURE, but not need OpenSSL 
support in mkimage.

Alex

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2021-05-31 19:01 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CO1PR11MB509256357A478B6F15F173C291459@CO1PR11MB5092.namprd11.prod.outlook.com>
     [not found] ` <76b0d5c1-4684-132f-8c82-230443a6c2f6@gmail.com>
     [not found]   ` <CO1PR11MB50924BFF383E0F5AF24EBE3D91459@CO1PR11MB5092.namprd11.prod.outlook.com>
     [not found]     ` <CO1PR11MB5092A668E6F6D8EB3E27659B91459@CO1PR11MB5092.namprd11.prod.outlook.com>
     [not found]       ` <CAPnjgZ0wyhaeG6jOgB6A+XRUquZ5mGsbouKwh4pJD1T1fcqytA@mail.gmail.com>
2021-04-24  7:43         ` U-Boot "lib: Add support for ECDSA image signing" commit breaks socfpga_*_atf_defconfig compilation Lim, Elly Siew Chin
2021-05-31 19:01           ` Alex G.

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.