[PATCH 00/18] image: Reduce #ifdef abuse in image code
This is a combination of select patches from Simon's series: "image: Reduce #ifdefs and ad-hoc defines in image code" and alternative solutions I proposed in: "image: Reduce the abuse of #ifdefs in image-sig.c"
After syncing with Simon, we agree that this is a reasonable base for further work in #ifdef reduction. Rather than describing changes from Simon's series or mine, we present this series de novo. Most patches are already peer-reviewed.
The latest patch in this series is optional: 'image: Add support for relocating crypto_algos in linker lists" I don't have a way to test it, and I don't know if it's still needed. I have included it for completeness.
Alexandru Gagniuc (10): common: Move host-only logic in image-sig.c to separate file common: image-sig.c: Remove host-specific logic and #ifdefs image: Add support for placing crypto_algo in linker lists image: rsa: Move verification algorithm to a linker list image: image-sig.c: Remove crypto_algos array lib: ecdsa: Remove #ifdefs from ecdsa.h lib: rsa: Remove #ifdefs from rsa.h image: Eliminate IMAGE_ENABLE_VERIFY macro image: Eliminate IMAGE_ENABLE_VERIFY_ECDSA macro [UNTESTED] image: Add support for relocating crypto_algos in linker lists
Simon Glass (8): image: Shorten FIT_ENABLE_SHAxxx_SUPPORT image: Rename SPL_SHAxxx_SUPPORT to SPL_FIT_SHAxxx image: Rename CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT Kconfig: Rename SPL_CRC32_SUPPORT to SPL_CRC32 Kconfig: Rename SPL_MD5_SUPPORT to SPL_MD5 image: Drop IMAGE_ENABLE_SHA1 image: Drop IMAGE_ENABLE_SHAxxx image: Drop IMAGE_ENABLE_BEST_MATCH
common/Kconfig.boot | 8 +- common/image-fit.c | 10 +- common/image-sig.c | 75 +++--------- common/spl/Kconfig | 14 +-- configs/axm_defconfig | 2 +- configs/bcm963158_ram_defconfig | 2 +- configs/chromebit_mickey_defconfig | 2 +- configs/chromebook_jerry_defconfig | 2 +- configs/chromebook_minnie_defconfig | 2 +- configs/chromebook_speedy_defconfig | 2 +- configs/evb-px30_defconfig | 2 +- configs/firefly-px30_defconfig | 2 +- configs/imxrt1020-evk_defconfig | 2 +- configs/imxrt1050-evk_defconfig | 2 +- configs/mt8516_pumpkin_defconfig | 2 +- configs/odroid-go2_defconfig | 2 +- configs/px30-core-ctouch2-px30_defconfig | 2 +- configs/px30-core-edimm2.2-px30_defconfig | 2 +- configs/sandbox_defconfig | 2 +- configs/socfpga_agilex_atf_defconfig | 2 +- configs/socfpga_agilex_vab_defconfig | 2 +- configs/socfpga_stratix10_atf_defconfig | 2 +- configs/taurus_defconfig | 2 +- include/image.h | 59 ++-------- include/u-boot/ecdsa.h | 25 ---- include/u-boot/rsa.h | 51 +-------- lib/rsa/rsa-sign.c | 4 +- lib/rsa/rsa-verify.c | 18 ++- tools/Makefile | 5 +- tools/image-sig-host.c | 133 ++++++++++++++++++++++ 30 files changed, 220 insertions(+), 220 deletions(-) create mode 100644 tools/image-sig-host.c
From: Simon Glass sjg@chromium.org
The ENABLE part of this name is redundant, since all boolean Kconfig options serve to enable something. The SUPPORT part is also redundant since Kconfigs can be assumed to enable support for something. Together they just serve to make these options overly long and inconsistent with other options.
Rename FIT_ENABLE_SHAxxx_SUPPORT to FIT_SHAxxx
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Alexandru Gagniuc mr.nuke.me@gmail.com Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com --- common/Kconfig.boot | 6 +++--- configs/mt8516_pumpkin_defconfig | 2 +- include/image.h | 12 ++++++------ 3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/common/Kconfig.boot b/common/Kconfig.boot index 5a18d62d78..af3325a7ce 100644 --- a/common/Kconfig.boot +++ b/common/Kconfig.boot @@ -35,7 +35,7 @@ config FIT_EXTERNAL_OFFSET could be put in the hole between data payload and fit image header, such as CSF data on i.MX platform.
-config FIT_ENABLE_SHA256_SUPPORT +config FIT_SHA256 bool "Support SHA256 checksum of FIT image contents" default y select SHA256 @@ -44,7 +44,7 @@ config FIT_ENABLE_SHA256_SUPPORT SHA256 checksum is a 256-bit (32-byte) hash value used to check that the image contents have not been corrupted.
-config FIT_ENABLE_SHA384_SUPPORT +config FIT_SHA384 bool "Support SHA384 checksum of FIT image contents" default n select SHA384 @@ -54,7 +54,7 @@ config FIT_ENABLE_SHA384_SUPPORT the image contents have not been corrupted. Use this for the highest security.
-config FIT_ENABLE_SHA512_SUPPORT +config FIT_SHA512 bool "Support SHA512 checksum of FIT image contents" default n select SHA512 diff --git a/configs/mt8516_pumpkin_defconfig b/configs/mt8516_pumpkin_defconfig index 780660058d..356f18acd8 100644 --- a/configs/mt8516_pumpkin_defconfig +++ b/configs/mt8516_pumpkin_defconfig @@ -13,7 +13,7 @@ CONFIG_DEBUG_UART_CLOCK=26000000 CONFIG_DEFAULT_DEVICE_TREE="mt8516-pumpkin" CONFIG_DEBUG_UART=y CONFIG_FIT=y -# CONFIG_FIT_ENABLE_SHA256_SUPPORT is not set +# CONFIG_FIT_SHA256 is not set # CONFIG_ARCH_FIXUP_FDT_MEMORY is not set CONFIG_DEFAULT_FDT_FILE="mt8516-pumpkin" # CONFIG_DISPLAY_BOARDINFO is not set diff --git a/include/image.h b/include/image.h index 459685d4d4..9319a779b9 100644 --- a/include/image.h +++ b/include/image.h @@ -31,9 +31,9 @@ struct fdt_region; #define IMAGE_ENABLE_OF_LIBFDT 1 #define CONFIG_FIT_VERBOSE 1 /* enable fit_format_{error,warning}() */ #define CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT 1 -#define CONFIG_FIT_ENABLE_SHA256_SUPPORT -#define CONFIG_FIT_ENABLE_SHA384_SUPPORT -#define CONFIG_FIT_ENABLE_SHA512_SUPPORT +#define CONFIG_FIT_SHA256 +#define CONFIG_FIT_SHA384 +#define CONFIG_FIT_SHA512 #define CONFIG_SHA1 #define CONFIG_SHA256 #define CONFIG_SHA384 @@ -89,21 +89,21 @@ struct fdt_region; #define IMAGE_ENABLE_SHA1 0 #endif
-#if defined(CONFIG_FIT_ENABLE_SHA256_SUPPORT) || \ +#if defined(CONFIG_FIT_SHA256) || \ defined(CONFIG_SPL_SHA256_SUPPORT) #define IMAGE_ENABLE_SHA256 1 #else #define IMAGE_ENABLE_SHA256 0 #endif
-#if defined(CONFIG_FIT_ENABLE_SHA384_SUPPORT) || \ +#if defined(CONFIG_FIT_SHA384) || \ defined(CONFIG_SPL_SHA384_SUPPORT) #define IMAGE_ENABLE_SHA384 1 #else #define IMAGE_ENABLE_SHA384 0 #endif
-#if defined(CONFIG_FIT_ENABLE_SHA512_SUPPORT) || \ +#if defined(CONFIG_FIT_SHA512) || \ defined(CONFIG_SPL_SHA512_SUPPORT) #define IMAGE_ENABLE_SHA512 1 #else
From: Simon Glass sjg@chromium.org
These option are named inconsistently with other SPL options, thus making them incompatible with the CONFIG_IS_ENABLED() macro. Rename them.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Alexandru Gagniuc mr.nuke.me@gmail.com Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com --- common/spl/Kconfig | 8 ++++---- include/image.h | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/common/spl/Kconfig b/common/spl/Kconfig index df5468f1ac..d94b989217 100644 --- a/common/spl/Kconfig +++ b/common/spl/Kconfig @@ -429,7 +429,7 @@ config SPL_MD5_SUPPORT applications where images may be changed maliciously, you should consider SHA256 or SHA384.
-config SPL_SHA1_SUPPORT +config SPL_FIT_SHA1 bool "Support SHA1" depends on SPL_FIT select SHA1 @@ -441,7 +441,7 @@ config SPL_SHA1_SUPPORT due to the expanding computing power available to brute-force attacks. For more security, consider SHA256 or SHA384.
-config SPL_SHA256_SUPPORT +config SPL_FIT_SHA256 bool "Support SHA256" depends on SPL_FIT select SHA256 @@ -450,7 +450,7 @@ config SPL_SHA256_SUPPORT checksum is a 256-bit (32-byte) hash value used to check that the image contents have not been corrupted.
-config SPL_SHA384_SUPPORT +config SPL_FIT_SHA384 bool "Support SHA384" depends on SPL_FIT select SHA384 @@ -461,7 +461,7 @@ config SPL_SHA384_SUPPORT image contents have not been corrupted. Use this for the highest security.
-config SPL_SHA512_SUPPORT +config SPL_FIT_SHA512 bool "Support SHA512" depends on SPL_FIT select SHA512 diff --git a/include/image.h b/include/image.h index 9319a779b9..3284f36c97 100644 --- a/include/image.h +++ b/include/image.h @@ -68,7 +68,7 @@ struct fdt_region; # ifdef CONFIG_SPL_MD5_SUPPORT # define IMAGE_ENABLE_MD5 1 # endif -# ifdef CONFIG_SPL_SHA1_SUPPORT +# ifdef CONFIG_SPL_FIT_SHA1 # define IMAGE_ENABLE_SHA1 1 # endif # else @@ -90,21 +90,21 @@ struct fdt_region; #endif
#if defined(CONFIG_FIT_SHA256) || \ - defined(CONFIG_SPL_SHA256_SUPPORT) + defined(CONFIG_SPL_FIT_SHA256) #define IMAGE_ENABLE_SHA256 1 #else #define IMAGE_ENABLE_SHA256 0 #endif
#if defined(CONFIG_FIT_SHA384) || \ - defined(CONFIG_SPL_SHA384_SUPPORT) + defined(CONFIG_SPL_FIT_SHA384) #define IMAGE_ENABLE_SHA384 1 #else #define IMAGE_ENABLE_SHA384 0 #endif
#if defined(CONFIG_FIT_SHA512) || \ - defined(CONFIG_SPL_SHA512_SUPPORT) + defined(CONFIG_SPL_FIT_SHA512) #define IMAGE_ENABLE_SHA512 1 #else #define IMAGE_ENABLE_SHA512 0
From: Simon Glass sjg@chromium.org
Drop the ENABLE and SUPPORT parts of this, which are redundant.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Alexandru Gagniuc mr.nuke.me@gmail.com Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com --- common/Kconfig.boot | 2 +- common/image-sig.c | 4 ++-- configs/bcm963158_ram_defconfig | 2 +- configs/sandbox_defconfig | 2 +- include/image.h | 2 +- include/u-boot/rsa.h | 8 ++++---- lib/rsa/rsa-sign.c | 4 ++-- lib/rsa/rsa-verify.c | 2 +- 8 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/common/Kconfig.boot b/common/Kconfig.boot index af3325a7ce..03a6e6f214 100644 --- a/common/Kconfig.boot +++ b/common/Kconfig.boot @@ -103,7 +103,7 @@ config FIT_SIGNATURE_MAX_SIZE device memory. Assure this size does not extend past expected storage space.
-config FIT_ENABLE_RSASSA_PSS_SUPPORT +config FIT_RSASSA_PSS bool "Support rsassa-pss signature scheme of FIT image contents" depends on FIT_SIGNATURE default n diff --git a/common/image-sig.c b/common/image-sig.c index 0f8e592aba..8b5cecbfa4 100644 --- a/common/image-sig.c +++ b/common/image-sig.c @@ -99,12 +99,12 @@ struct padding_algo padding_algos[] = { .name = "pkcs-1.5", .verify = padding_pkcs_15_verify, }, -#ifdef CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT +#ifdef CONFIG_FIT_RSASSA_PSS { .name = "pss", .verify = padding_pss_verify, } -#endif /* CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT */ +#endif /* CONFIG_FIT_RSASSA_PSS */ };
struct checksum_algo *image_get_checksum_algo(const char *full_name) diff --git a/configs/bcm963158_ram_defconfig b/configs/bcm963158_ram_defconfig index 0be1e0981a..ec006514d1 100644 --- a/configs/bcm963158_ram_defconfig +++ b/configs/bcm963158_ram_defconfig @@ -11,7 +11,7 @@ CONFIG_DEFAULT_DEVICE_TREE="bcm963158" CONFIG_ENV_VARS_UBOOT_CONFIG=y CONFIG_FIT=y CONFIG_FIT_SIGNATURE=y -CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT=y +CONFIG_FIT_RSASSA_PSS=y CONFIG_FIT_VERBOSE=y CONFIG_LEGACY_IMAGE_FORMAT=y CONFIG_SUPPORT_RAW_INITRD=y diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig index bdbf714e2b..7b8603d1ef 100644 --- a/configs/sandbox_defconfig +++ b/configs/sandbox_defconfig @@ -10,7 +10,7 @@ CONFIG_DEBUG_UART=y CONFIG_DISTRO_DEFAULTS=y CONFIG_FIT=y CONFIG_FIT_SIGNATURE=y -CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT=y +CONFIG_FIT_RSASSA_PSS=y CONFIG_FIT_CIPHER=y CONFIG_FIT_VERBOSE=y CONFIG_BOOTSTAGE=y diff --git a/include/image.h b/include/image.h index 3284f36c97..9eb45ffd40 100644 --- a/include/image.h +++ b/include/image.h @@ -30,7 +30,7 @@ struct fdt_region; #define IMAGE_ENABLE_FIT 1 #define IMAGE_ENABLE_OF_LIBFDT 1 #define CONFIG_FIT_VERBOSE 1 /* enable fit_format_{error,warning}() */ -#define CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT 1 +#define CONFIG_FIT_RSASSA_PSS 1 #define CONFIG_FIT_SHA256 #define CONFIG_FIT_SHA384 #define CONFIG_FIT_SHA512 diff --git a/include/u-boot/rsa.h b/include/u-boot/rsa.h index bed1c097c2..bc564d56fa 100644 --- a/include/u-boot/rsa.h +++ b/include/u-boot/rsa.h @@ -119,11 +119,11 @@ int padding_pkcs_15_verify(struct image_sign_info *info, uint8_t *msg, int msg_len, const uint8_t *hash, int hash_len);
-#ifdef CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT +#ifdef CONFIG_FIT_RSASSA_PSS int padding_pss_verify(struct image_sign_info *info, uint8_t *msg, int msg_len, const uint8_t *hash, int hash_len); -#endif /* CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT */ +#endif /* CONFIG_FIT_RSASSA_PSS */ #else static inline int rsa_verify_hash(struct image_sign_info *info, const uint8_t *hash, @@ -146,14 +146,14 @@ static inline int padding_pkcs_15_verify(struct image_sign_info *info, return -ENXIO; }
-#ifdef CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT +#ifdef CONFIG_FIT_RSASSA_PSS static inline int padding_pss_verify(struct image_sign_info *info, uint8_t *msg, int msg_len, const uint8_t *hash, int hash_len) { return -ENXIO; } -#endif /* CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT */ +#endif /* CONFIG_FIT_RSASSA_PSS */ #endif
#define RSA_DEFAULT_PADDING_NAME "pkcs-1.5" diff --git a/lib/rsa/rsa-sign.c b/lib/rsa/rsa-sign.c index 5a1583b8f7..f4ed11e74a 100644 --- a/lib/rsa/rsa-sign.c +++ b/lib/rsa/rsa-sign.c @@ -442,7 +442,7 @@ static int rsa_sign_with_key(EVP_PKEY *pkey, struct padding_algo *padding_algo, goto err_sign; }
-#ifdef CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT +#ifdef CONFIG_FIT_RSASSA_PSS if (padding_algo && !strcmp(padding_algo->name, "pss")) { if (EVP_PKEY_CTX_set_rsa_padding(ckey, RSA_PKCS1_PSS_PADDING) <= 0) { @@ -450,7 +450,7 @@ static int rsa_sign_with_key(EVP_PKEY *pkey, struct padding_algo *padding_algo, goto err_sign; } } -#endif /* CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT */ +#endif /* CONFIG_FIT_RSASSA_PSS */
for (i = 0; i < region_count; i++) { if (!EVP_DigestSignUpdate(context, region[i].data, diff --git a/lib/rsa/rsa-verify.c b/lib/rsa/rsa-verify.c index aee76f42d5..1998c773fc 100644 --- a/lib/rsa/rsa-verify.c +++ b/lib/rsa/rsa-verify.c @@ -95,7 +95,7 @@ int padding_pkcs_15_verify(struct image_sign_info *info, return 0; }
-#ifdef CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT +#ifdef CONFIG_FIT_RSASSA_PSS static void u32_i2osp(uint32_t val, uint8_t *buf) { buf[0] = (uint8_t)((val >> 24) & 0xff);
From: Simon Glass sjg@chromium.org
Drop the _SUPPORT suffix so we can use CONFIG_IS_ENABLED() with this option.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Alexandru Gagniuc mr.nuke.me@gmail.com Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com --- common/spl/Kconfig | 4 ++-- configs/axm_defconfig | 2 +- configs/chromebit_mickey_defconfig | 2 +- configs/chromebook_jerry_defconfig | 2 +- configs/chromebook_minnie_defconfig | 2 +- configs/chromebook_speedy_defconfig | 2 +- configs/evb-px30_defconfig | 2 +- configs/firefly-px30_defconfig | 2 +- configs/imxrt1020-evk_defconfig | 2 +- configs/imxrt1050-evk_defconfig | 2 +- configs/odroid-go2_defconfig | 2 +- configs/px30-core-ctouch2-px30_defconfig | 2 +- configs/px30-core-edimm2.2-px30_defconfig | 2 +- configs/socfpga_agilex_atf_defconfig | 2 +- configs/socfpga_agilex_vab_defconfig | 2 +- configs/socfpga_stratix10_atf_defconfig | 2 +- configs/taurus_defconfig | 2 +- include/image.h | 2 +- 18 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/common/spl/Kconfig b/common/spl/Kconfig index d94b989217..0329d93b29 100644 --- a/common/spl/Kconfig +++ b/common/spl/Kconfig @@ -204,7 +204,7 @@ config SPL_LEGACY_IMAGE_SUPPORT config SPL_LEGACY_IMAGE_CRC_CHECK bool "Check CRC of Legacy images" depends on SPL_LEGACY_IMAGE_SUPPORT - select SPL_CRC32_SUPPORT + select SPL_CRC32 help Enable this to check the CRC of Legacy images. While this increases reliability, it affects both code size and boot duration. @@ -407,7 +407,7 @@ config SYS_MMCSD_RAW_MODE_EMMC_BOOT_PARTITION the eMMC EXT_CSC_PART_CONFIG selection should be overridden in SPL by user defined partition number.
-config SPL_CRC32_SUPPORT +config SPL_CRC32 bool "Support CRC32" default y if SPL_LEGACY_IMAGE_SUPPORT help diff --git a/configs/axm_defconfig b/configs/axm_defconfig index 0bfd7548b0..4e776fd695 100644 --- a/configs/axm_defconfig +++ b/configs/axm_defconfig @@ -32,7 +32,7 @@ CONFIG_BOOTCOMMAND="run flash_self" CONFIG_BOARD_EARLY_INIT_F=y # CONFIG_SPL_LEGACY_IMAGE_SUPPORT is not set CONFIG_SPL_SYS_MALLOC_SIMPLE=y -CONFIG_SPL_CRC32_SUPPORT=y +CONFIG_SPL_CRC32=y CONFIG_SPL_NAND_SUPPORT=y CONFIG_SPL_NAND_DRIVERS=y CONFIG_SPL_NAND_ECC=y diff --git a/configs/chromebit_mickey_defconfig b/configs/chromebit_mickey_defconfig index c09b63b946..2b664e118c 100644 --- a/configs/chromebit_mickey_defconfig +++ b/configs/chromebit_mickey_defconfig @@ -25,7 +25,7 @@ CONFIG_BOARD_EARLY_INIT_R=y CONFIG_SPL_STACK_R=y CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x2000 # CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR is not set -# CONFIG_SPL_CRC32_SUPPORT is not set +# CONFIG_SPL_CRC32 is not set CONFIG_SPL_SPI_LOAD=y CONFIG_CMD_GPIO=y CONFIG_CMD_GPT=y diff --git a/configs/chromebook_jerry_defconfig b/configs/chromebook_jerry_defconfig index 692b630174..a757d259f5 100644 --- a/configs/chromebook_jerry_defconfig +++ b/configs/chromebook_jerry_defconfig @@ -26,7 +26,7 @@ CONFIG_BOARD_EARLY_INIT_R=y CONFIG_SPL_STACK_R=y CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x2000 # CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR is not set -# CONFIG_SPL_CRC32_SUPPORT is not set +# CONFIG_SPL_CRC32 is not set CONFIG_SPL_SPI_LOAD=y CONFIG_CMD_GPIO=y CONFIG_CMD_GPT=y diff --git a/configs/chromebook_minnie_defconfig b/configs/chromebook_minnie_defconfig index ae55842e3b..353aa01ea9 100644 --- a/configs/chromebook_minnie_defconfig +++ b/configs/chromebook_minnie_defconfig @@ -26,7 +26,7 @@ CONFIG_BOARD_EARLY_INIT_R=y CONFIG_SPL_STACK_R=y CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x2000 # CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR is not set -# CONFIG_SPL_CRC32_SUPPORT is not set +# CONFIG_SPL_CRC32 is not set CONFIG_SPL_SPI_LOAD=y CONFIG_CMD_GPIO=y CONFIG_CMD_GPT=y diff --git a/configs/chromebook_speedy_defconfig b/configs/chromebook_speedy_defconfig index 4b460ee6a9..c5be5597b1 100644 --- a/configs/chromebook_speedy_defconfig +++ b/configs/chromebook_speedy_defconfig @@ -26,7 +26,7 @@ CONFIG_BOARD_EARLY_INIT_R=y CONFIG_SPL_STACK_R=y CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x2000 # CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR is not set -# CONFIG_SPL_CRC32_SUPPORT is not set +# CONFIG_SPL_CRC32 is not set CONFIG_SPL_SPI_LOAD=y CONFIG_CMD_GPIO=y CONFIG_CMD_GPT=y diff --git a/configs/evb-px30_defconfig b/configs/evb-px30_defconfig index d2fdfef293..55e2702a17 100644 --- a/configs/evb-px30_defconfig +++ b/configs/evb-px30_defconfig @@ -29,7 +29,7 @@ CONFIG_SPL_BOOTROM_SUPPORT=y # CONFIG_SPL_RAW_IMAGE_SUPPORT is not set CONFIG_SPL_STACK_R=y # CONFIG_TPL_BANNER_PRINT is not set -CONFIG_SPL_CRC32_SUPPORT=y +CONFIG_SPL_CRC32=y CONFIG_SPL_ATF=y # CONFIG_TPL_FRAMEWORK is not set # CONFIG_CMD_BOOTD is not set diff --git a/configs/firefly-px30_defconfig b/configs/firefly-px30_defconfig index 6487615fe0..978a360405 100644 --- a/configs/firefly-px30_defconfig +++ b/configs/firefly-px30_defconfig @@ -30,7 +30,7 @@ CONFIG_SPL_BOOTROM_SUPPORT=y # CONFIG_SPL_RAW_IMAGE_SUPPORT is not set CONFIG_SPL_STACK_R=y # CONFIG_TPL_BANNER_PRINT is not set -CONFIG_SPL_CRC32_SUPPORT=y +CONFIG_SPL_CRC32=y CONFIG_SPL_ATF=y # CONFIG_TPL_FRAMEWORK is not set # CONFIG_CMD_BOOTD is not set diff --git a/configs/imxrt1020-evk_defconfig b/configs/imxrt1020-evk_defconfig index ad408ebef8..dec55de695 100644 --- a/configs/imxrt1020-evk_defconfig +++ b/configs/imxrt1020-evk_defconfig @@ -24,7 +24,7 @@ CONFIG_SPL_BOARD_INIT=y CONFIG_SPL_SYS_MALLOC_SIMPLE=y CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR=y CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR=0x100 -# CONFIG_SPL_CRC32_SUPPORT is not set +# CONFIG_SPL_CRC32 is not set # CONFIG_BOOTM_NETBSD is not set # CONFIG_BOOTM_PLAN9 is not set # CONFIG_BOOTM_RTEMS is not set diff --git a/configs/imxrt1050-evk_defconfig b/configs/imxrt1050-evk_defconfig index d03572e7db..08379ae022 100644 --- a/configs/imxrt1050-evk_defconfig +++ b/configs/imxrt1050-evk_defconfig @@ -27,7 +27,7 @@ CONFIG_SPL_BOARD_INIT=y CONFIG_SPL_SYS_MALLOC_SIMPLE=y CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR=y CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR=0x100 -# CONFIG_SPL_CRC32_SUPPORT is not set +# CONFIG_SPL_CRC32 is not set # CONFIG_BOOTM_NETBSD is not set # CONFIG_BOOTM_PLAN9 is not set # CONFIG_BOOTM_RTEMS is not set diff --git a/configs/odroid-go2_defconfig b/configs/odroid-go2_defconfig index 6aa41e3755..82e340a16e 100644 --- a/configs/odroid-go2_defconfig +++ b/configs/odroid-go2_defconfig @@ -33,7 +33,7 @@ CONFIG_SPL_BOOTROM_SUPPORT=y # CONFIG_SPL_RAW_IMAGE_SUPPORT is not set CONFIG_SPL_STACK_R=y # CONFIG_TPL_BANNER_PRINT is not set -CONFIG_SPL_CRC32_SUPPORT=y +CONFIG_SPL_CRC32=y CONFIG_SPL_I2C_SUPPORT=y CONFIG_SPL_POWER_SUPPORT=y CONFIG_SPL_ATF=y diff --git a/configs/px30-core-ctouch2-px30_defconfig b/configs/px30-core-ctouch2-px30_defconfig index 1afc146bbf..3ac0ea4ccc 100644 --- a/configs/px30-core-ctouch2-px30_defconfig +++ b/configs/px30-core-ctouch2-px30_defconfig @@ -30,7 +30,7 @@ CONFIG_SPL_BOOTROM_SUPPORT=y # CONFIG_SPL_RAW_IMAGE_SUPPORT is not set CONFIG_SPL_STACK_R=y # CONFIG_TPL_BANNER_PRINT is not set -CONFIG_SPL_CRC32_SUPPORT=y +CONFIG_SPL_CRC32=y CONFIG_SPL_ATF=y # CONFIG_TPL_FRAMEWORK is not set # CONFIG_CMD_BOOTD is not set diff --git a/configs/px30-core-edimm2.2-px30_defconfig b/configs/px30-core-edimm2.2-px30_defconfig index 9d78eee84d..f208297de5 100644 --- a/configs/px30-core-edimm2.2-px30_defconfig +++ b/configs/px30-core-edimm2.2-px30_defconfig @@ -30,7 +30,7 @@ CONFIG_SPL_BOOTROM_SUPPORT=y # CONFIG_SPL_RAW_IMAGE_SUPPORT is not set CONFIG_SPL_STACK_R=y # CONFIG_TPL_BANNER_PRINT is not set -CONFIG_SPL_CRC32_SUPPORT=y +CONFIG_SPL_CRC32=y CONFIG_SPL_ATF=y # CONFIG_TPL_FRAMEWORK is not set # CONFIG_CMD_BOOTD is not set diff --git a/configs/socfpga_agilex_atf_defconfig b/configs/socfpga_agilex_atf_defconfig index 29e3fb865e..8142cbdd4e 100644 --- a/configs/socfpga_agilex_atf_defconfig +++ b/configs/socfpga_agilex_atf_defconfig @@ -23,7 +23,7 @@ CONFIG_USE_BOOTARGS=y CONFIG_BOOTARGS="earlycon" CONFIG_USE_BOOTCOMMAND=y CONFIG_BOOTCOMMAND="run fatscript; run mmcfitload; run linux_qspi_enable; run mmcfitboot" -CONFIG_SPL_CRC32_SUPPORT=y +CONFIG_SPL_CRC32=y CONFIG_SPL_CACHE=y CONFIG_SPL_SPI_LOAD=y CONFIG_SPL_ATF=y diff --git a/configs/socfpga_agilex_vab_defconfig b/configs/socfpga_agilex_vab_defconfig index af726bab69..78b2b7558f 100644 --- a/configs/socfpga_agilex_vab_defconfig +++ b/configs/socfpga_agilex_vab_defconfig @@ -24,7 +24,7 @@ CONFIG_USE_BOOTARGS=y CONFIG_BOOTARGS="earlycon" CONFIG_USE_BOOTCOMMAND=y CONFIG_BOOTCOMMAND="run fatscript; run mmcfitload; run mmcfitboot" -CONFIG_SPL_CRC32_SUPPORT=y +CONFIG_SPL_CRC32=y CONFIG_SPL_CACHE=y CONFIG_SPL_SPI_LOAD=y CONFIG_SPL_ATF=y diff --git a/configs/socfpga_stratix10_atf_defconfig b/configs/socfpga_stratix10_atf_defconfig index 9f2f220c3a..bf89fe5a66 100644 --- a/configs/socfpga_stratix10_atf_defconfig +++ b/configs/socfpga_stratix10_atf_defconfig @@ -23,7 +23,7 @@ CONFIG_USE_BOOTARGS=y CONFIG_BOOTARGS="earlycon" CONFIG_USE_BOOTCOMMAND=y CONFIG_BOOTCOMMAND="run fatscript; run mmcfitload; run linux_qspi_enable; run mmcfitboot" -CONFIG_SPL_CRC32_SUPPORT=y +CONFIG_SPL_CRC32=y CONFIG_SPL_SPI_LOAD=y CONFIG_SPL_ATF=y CONFIG_SPL_ATF_NO_PLATFORM_PARAM=y diff --git a/configs/taurus_defconfig b/configs/taurus_defconfig index a79cdf3fa7..5caf9ab30d 100644 --- a/configs/taurus_defconfig +++ b/configs/taurus_defconfig @@ -36,7 +36,7 @@ CONFIG_BOOTCOMMAND="nand read 0x22000000 0x200000 0x300000; bootm" CONFIG_BOARD_EARLY_INIT_F=y # CONFIG_SPL_LEGACY_IMAGE_SUPPORT is not set CONFIG_SPL_SYS_MALLOC_SIMPLE=y -CONFIG_SPL_CRC32_SUPPORT=y +CONFIG_SPL_CRC32=y CONFIG_SPL_NAND_SUPPORT=y CONFIG_SPL_NAND_DRIVERS=y CONFIG_SPL_NAND_ECC=y diff --git a/include/image.h b/include/image.h index 9eb45ffd40..2c812d22a9 100644 --- a/include/image.h +++ b/include/image.h @@ -62,7 +62,7 @@ struct fdt_region; #include <linux/libfdt.h> #include <fdt_support.h> # ifdef CONFIG_SPL_BUILD -# ifdef CONFIG_SPL_CRC32_SUPPORT +# ifdef CONFIG_SPL_CRC32 # define IMAGE_ENABLE_CRC32 1 # endif # ifdef CONFIG_SPL_MD5_SUPPORT
From: Simon Glass sjg@chromium.org
Drop the _SUPPORT suffix so we can use CONFIG_IS_ENABLED() with this option.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Alexandru Gagniuc mr.nuke.me@gmail.com Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com --- common/spl/Kconfig | 2 +- include/image.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/common/spl/Kconfig b/common/spl/Kconfig index 0329d93b29..c83ce4d367 100644 --- a/common/spl/Kconfig +++ b/common/spl/Kconfig @@ -417,7 +417,7 @@ config SPL_CRC32 for detected accidental image corruption. For secure applications you should consider SHA1 or SHA256.
-config SPL_MD5_SUPPORT +config SPL_MD5 bool "Support MD5" depends on SPL_FIT help diff --git a/include/image.h b/include/image.h index 2c812d22a9..887a3270bd 100644 --- a/include/image.h +++ b/include/image.h @@ -65,7 +65,7 @@ struct fdt_region; # ifdef CONFIG_SPL_CRC32 # define IMAGE_ENABLE_CRC32 1 # endif -# ifdef CONFIG_SPL_MD5_SUPPORT +# ifdef CONFIG_SPL_MD5 # define IMAGE_ENABLE_MD5 1 # endif # ifdef CONFIG_SPL_FIT_SHA1
From: Simon Glass sjg@chromium.org
We already have a host Kconfig for SHA1. Use CONFIG_IS_ENABLED(SHA1) directly in the code shared with the host build, so we can drop the unnecessary indirection.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Alexandru Gagniuc mr.nuke.me@gmail.com Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com --- common/image-fit.c | 2 +- include/image.h | 8 -------- 2 files changed, 1 insertion(+), 9 deletions(-)
diff --git a/common/image-fit.c b/common/image-fit.c index e614643fe3..24e92a8e92 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -1218,7 +1218,7 @@ int calculate_hash(const void *data, int data_len, const char *algo, CHUNKSZ_CRC32); *((uint32_t *)value) = cpu_to_uimage(*((uint32_t *)value)); *value_len = 4; - } else if (IMAGE_ENABLE_SHA1 && strcmp(algo, "sha1") == 0) { + } else if (CONFIG_IS_ENABLED(SHA1) && strcmp(algo, "sha1") == 0) { sha1_csum_wd((unsigned char *)data, data_len, (unsigned char *)value, CHUNKSZ_SHA1); *value_len = 20; diff --git a/include/image.h b/include/image.h index 887a3270bd..8c718adba0 100644 --- a/include/image.h +++ b/include/image.h @@ -68,13 +68,9 @@ struct fdt_region; # ifdef CONFIG_SPL_MD5 # define IMAGE_ENABLE_MD5 1 # endif -# ifdef CONFIG_SPL_FIT_SHA1 -# define IMAGE_ENABLE_SHA1 1 -# endif # else # define IMAGE_ENABLE_CRC32 1 # define IMAGE_ENABLE_MD5 1 -# define IMAGE_ENABLE_SHA1 1 # endif
#ifndef IMAGE_ENABLE_CRC32 @@ -85,10 +81,6 @@ struct fdt_region; #define IMAGE_ENABLE_MD5 0 #endif
-#ifndef IMAGE_ENABLE_SHA1 -#define IMAGE_ENABLE_SHA1 0 -#endif - #if defined(CONFIG_FIT_SHA256) || \ defined(CONFIG_SPL_FIT_SHA256) #define IMAGE_ENABLE_SHA256 1
Hi Alexandru,
On Mon, 17 May 2021 at 10:38, Alexandru Gagniuc mr.nuke.me@gmail.com wrote:
From: Simon Glass sjg@chromium.org
We already have a host Kconfig for SHA1. Use CONFIG_IS_ENABLED(SHA1) directly in the code shared with the host build, so we can drop the unnecessary indirection.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Alexandru Gagniuc mr.nuke.me@gmail.com Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com
common/image-fit.c | 2 +- include/image.h | 8 -------- 2 files changed, 1 insertion(+), 9 deletions(-)
diff --git a/common/image-fit.c b/common/image-fit.c index e614643fe3..24e92a8e92 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -1218,7 +1218,7 @@ int calculate_hash(const void *data, int data_len, const char *algo, CHUNKSZ_CRC32); *((uint32_t *)value) = cpu_to_uimage(*((uint32_t *)value)); *value_len = 4;
} else if (IMAGE_ENABLE_SHA1 && strcmp(algo, "sha1") == 0) {
} else if (CONFIG_IS_ENABLED(SHA1) && strcmp(algo, "sha1") == 0) {
This can only work if the my host Kconfig patch comes first. Otherwise this code will just be skipped on the host.
sha1_csum_wd((unsigned char *)data, data_len, (unsigned char *)value, CHUNKSZ_SHA1); *value_len = 20;diff --git a/include/image.h b/include/image.h index 887a3270bd..8c718adba0 100644 --- a/include/image.h +++ b/include/image.h @@ -68,13 +68,9 @@ struct fdt_region; # ifdef CONFIG_SPL_MD5 # define IMAGE_ENABLE_MD5 1 # endif -# ifdef CONFIG_SPL_FIT_SHA1 -# define IMAGE_ENABLE_SHA1 1 -# endif # else # define IMAGE_ENABLE_CRC32 1 # define IMAGE_ENABLE_MD5 1 -# define IMAGE_ENABLE_SHA1 1 # endif
#ifndef IMAGE_ENABLE_CRC32 @@ -85,10 +81,6 @@ struct fdt_region; #define IMAGE_ENABLE_MD5 0 #endif
-#ifndef IMAGE_ENABLE_SHA1 -#define IMAGE_ENABLE_SHA1 0 -#endif
#if defined(CONFIG_FIT_SHA256) || \ defined(CONFIG_SPL_FIT_SHA256)
#define IMAGE_ENABLE_SHA256 1
2.31.1
Regards, Simon
On 5/19/21 11:36 AM, Simon Glass wrote:
Hi Alexandru,
On Mon, 17 May 2021 at 10:38, Alexandru Gagniuc mr.nuke.me@gmail.com wrote:
From: Simon Glass sjg@chromium.org
We already have a host Kconfig for SHA1. Use CONFIG_IS_ENABLED(SHA1) directly in the code shared with the host build, so we can drop the unnecessary indirection.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Alexandru Gagniuc mr.nuke.me@gmail.com Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com
common/image-fit.c | 2 +- include/image.h | 8 -------- 2 files changed, 1 insertion(+), 9 deletions(-)
diff --git a/common/image-fit.c b/common/image-fit.c index e614643fe3..24e92a8e92 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -1218,7 +1218,7 @@ int calculate_hash(const void *data, int data_len, const char *algo, CHUNKSZ_CRC32); *((uint32_t *)value) = cpu_to_uimage(*((uint32_t *)value)); *value_len = 4;
} else if (IMAGE_ENABLE_SHA1 && strcmp(algo, "sha1") == 0) {
} else if (CONFIG_IS_ENABLED(SHA1) && strcmp(algo, "sha1") == 0) {This can only work if the my host Kconfig patch comes first. Otherwise this code will just be skipped on the host.
I was scratching my head too as to why this works in practice, but not in theory. There is a #define CONFIG_SHA1 in image.h.
Although not a perfect fix, we go from two ways to enable SHA1 ("#define IMAGE_ENABLE_SHA1", and "#define CONFIG_SHA1"), to just one. That's why I think this change is an improvement, and part of this series.
Alex
sha1_csum_wd((unsigned char *)data, data_len, (unsigned char *)value, CHUNKSZ_SHA1); *value_len = 20;diff --git a/include/image.h b/include/image.h index 887a3270bd..8c718adba0 100644 --- a/include/image.h +++ b/include/image.h @@ -68,13 +68,9 @@ struct fdt_region; # ifdef CONFIG_SPL_MD5 # define IMAGE_ENABLE_MD5 1 # endif -# ifdef CONFIG_SPL_FIT_SHA1 -# define IMAGE_ENABLE_SHA1 1 -# endif # else # define IMAGE_ENABLE_CRC32 1 # define IMAGE_ENABLE_MD5 1 -# define IMAGE_ENABLE_SHA1 1 # endif
#ifndef IMAGE_ENABLE_CRC32 @@ -85,10 +81,6 @@ struct fdt_region; #define IMAGE_ENABLE_MD5 0 #endif
-#ifndef IMAGE_ENABLE_SHA1 -#define IMAGE_ENABLE_SHA1 0 -#endif
- #if defined(CONFIG_FIT_SHA256) || \ defined(CONFIG_SPL_FIT_SHA256) #define IMAGE_ENABLE_SHA256 1
-- 2.31.1
Regards, Simon
Hi Alex,
On Wed, 19 May 2021 at 11:44, Alex G mr.nuke.me@gmail.com wrote:
On 5/19/21 11:36 AM, Simon Glass wrote:
Hi Alexandru,
On Mon, 17 May 2021 at 10:38, Alexandru Gagniuc mr.nuke.me@gmail.com wrote:
From: Simon Glass sjg@chromium.org
We already have a host Kconfig for SHA1. Use CONFIG_IS_ENABLED(SHA1) directly in the code shared with the host build, so we can drop the unnecessary indirection.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Alexandru Gagniuc mr.nuke.me@gmail.com Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com
common/image-fit.c | 2 +- include/image.h | 8 -------- 2 files changed, 1 insertion(+), 9 deletions(-)
diff --git a/common/image-fit.c b/common/image-fit.c index e614643fe3..24e92a8e92 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -1218,7 +1218,7 @@ int calculate_hash(const void *data, int data_len, const char *algo, CHUNKSZ_CRC32); *((uint32_t *)value) = cpu_to_uimage(*((uint32_t *)value)); *value_len = 4;
} else if (IMAGE_ENABLE_SHA1 && strcmp(algo, "sha1") == 0) {
} else if (CONFIG_IS_ENABLED(SHA1) && strcmp(algo, "sha1") == 0) {This can only work if the my host Kconfig patch comes first. Otherwise this code will just be skipped on the host.
I was scratching my head too as to why this works in practice, but not in theory. There is a #define CONFIG_SHA1 in image.h.
Although not a perfect fix, we go from two ways to enable SHA1 ("#define IMAGE_ENABLE_SHA1", and "#define CONFIG_SHA1"), to just one. That's why I think this change is an improvement, and part of this series.
No, we really should not do that...everything needs to be in Kconfig.
[..]
Regards, Simon
On 5/19/21 4:55 PM, Simon Glass wrote:
Hi Alex,
On Wed, 19 May 2021 at 11:44, Alex G mr.nuke.me@gmail.com wrote:
On 5/19/21 11:36 AM, Simon Glass wrote:
Hi Alexandru,
On Mon, 17 May 2021 at 10:38, Alexandru Gagniuc mr.nuke.me@gmail.com wrote:
From: Simon Glass sjg@chromium.org
We already have a host Kconfig for SHA1. Use CONFIG_IS_ENABLED(SHA1) directly in the code shared with the host build, so we can drop the unnecessary indirection.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Alexandru Gagniuc mr.nuke.me@gmail.com Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com
common/image-fit.c | 2 +- include/image.h | 8 -------- 2 files changed, 1 insertion(+), 9 deletions(-)
diff --git a/common/image-fit.c b/common/image-fit.c index e614643fe3..24e92a8e92 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -1218,7 +1218,7 @@ int calculate_hash(const void *data, int data_len, const char *algo, CHUNKSZ_CRC32); *((uint32_t *)value) = cpu_to_uimage(*((uint32_t *)value)); *value_len = 4;
} else if (IMAGE_ENABLE_SHA1 && strcmp(algo, "sha1") == 0) {
} else if (CONFIG_IS_ENABLED(SHA1) && strcmp(algo, "sha1") == 0) {This can only work if the my host Kconfig patch comes first. Otherwise this code will just be skipped on the host.
I was scratching my head too as to why this works in practice, but not in theory. There is a #define CONFIG_SHA1 in image.h.
Although not a perfect fix, we go from two ways to enable SHA1 ("#define IMAGE_ENABLE_SHA1", and "#define CONFIG_SHA1"), to just one. That's why I think this change is an improvement, and part of this series.
No, we really should not do that...everything needs to be in Kconfig.
I agree for target code. But, as a long term solution, let's look at how we can get hash algos in linker lists, like we're proposing to do for crytpo algos. Or I could just drop this change in v2.
Alex
Hi Alex,
On Wed, 19 May 2021 at 20:41, Alex G. mr.nuke.me@gmail.com wrote:
On 5/19/21 4:55 PM, Simon Glass wrote:
Hi Alex,
On Wed, 19 May 2021 at 11:44, Alex G mr.nuke.me@gmail.com wrote:
On 5/19/21 11:36 AM, Simon Glass wrote:
Hi Alexandru,
On Mon, 17 May 2021 at 10:38, Alexandru Gagniuc mr.nuke.me@gmail.com wrote:
From: Simon Glass sjg@chromium.org
We already have a host Kconfig for SHA1. Use CONFIG_IS_ENABLED(SHA1) directly in the code shared with the host build, so we can drop the unnecessary indirection.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Alexandru Gagniuc mr.nuke.me@gmail.com Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com
common/image-fit.c | 2 +- include/image.h | 8 -------- 2 files changed, 1 insertion(+), 9 deletions(-)
diff --git a/common/image-fit.c b/common/image-fit.c index e614643fe3..24e92a8e92 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -1218,7 +1218,7 @@ int calculate_hash(const void *data, int data_len, const char *algo, CHUNKSZ_CRC32); *((uint32_t *)value) = cpu_to_uimage(*((uint32_t *)value)); *value_len = 4;
} else if (IMAGE_ENABLE_SHA1 && strcmp(algo, "sha1") == 0) {
} else if (CONFIG_IS_ENABLED(SHA1) && strcmp(algo, "sha1") == 0) {This can only work if the my host Kconfig patch comes first. Otherwise this code will just be skipped on the host.
I was scratching my head too as to why this works in practice, but not in theory. There is a #define CONFIG_SHA1 in image.h.
Although not a perfect fix, we go from two ways to enable SHA1 ("#define IMAGE_ENABLE_SHA1", and "#define CONFIG_SHA1"), to just one. That's why I think this change is an improvement, and part of this series.
No, we really should not do that...everything needs to be in Kconfig.
I agree for target code. But, as a long term solution, let's look at how we can get hash algos in linker lists, like we're proposing to do for crytpo algos. Or I could just drop this change in v2.
Would it not be easier to have a host Kconfig for these? You seem to be going to extreme lengths to avoid it, but it seems like the simplest solution, easy to understand, no effect on code size and scalable to the future.
Regards, Simon
On 5/20/21 12:52 PM, Simon Glass wrote:
Hi Alex,
On Wed, 19 May 2021 at 20:41, Alex G. mr.nuke.me@gmail.com wrote:
On 5/19/21 4:55 PM, Simon Glass wrote:
Hi Alex,
On Wed, 19 May 2021 at 11:44, Alex G mr.nuke.me@gmail.com wrote:
On 5/19/21 11:36 AM, Simon Glass wrote:
Hi Alexandru,
On Mon, 17 May 2021 at 10:38, Alexandru Gagniuc mr.nuke.me@gmail.com wrote:
From: Simon Glass sjg@chromium.org
We already have a host Kconfig for SHA1. Use CONFIG_IS_ENABLED(SHA1) directly in the code shared with the host build, so we can drop the unnecessary indirection.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Alexandru Gagniuc mr.nuke.me@gmail.com Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com
common/image-fit.c | 2 +- include/image.h | 8 -------- 2 files changed, 1 insertion(+), 9 deletions(-)diff --git a/common/image-fit.c b/common/image-fit.c index e614643fe3..24e92a8e92 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -1218,7 +1218,7 @@ int calculate_hash(const void *data, int data_len, const char *algo, CHUNKSZ_CRC32); *((uint32_t *)value) = cpu_to_uimage(*((uint32_t *)value)); *value_len = 4;
} else if (IMAGE_ENABLE_SHA1 && strcmp(algo, "sha1") == 0) {
} else if (CONFIG_IS_ENABLED(SHA1) && strcmp(algo, "sha1") == 0) {This can only work if the my host Kconfig patch comes first. Otherwise this code will just be skipped on the host.
I was scratching my head too as to why this works in practice, but not in theory. There is a #define CONFIG_SHA1 in image.h.
Although not a perfect fix, we go from two ways to enable SHA1 ("#define IMAGE_ENABLE_SHA1", and "#define CONFIG_SHA1"), to just one. That's why I think this change is an improvement, and part of this series.
No, we really should not do that...everything needs to be in Kconfig.
I agree for target code. But, as a long term solution, let's look at how we can get hash algos in linker lists, like we're proposing to do for crytpo algos. Or I could just drop this change in v2.
Would it not be easier to have a host Kconfig for these? You seem to be going to extreme lengths to avoid it, but it seems like the simplest solution, easy to understand, no effect on code size and scalable to the future.
It's easy for the short term in terms if the goal is to get something merged. It just hides more fundamental issues with the code. For ecample, why is there hash_calculate() and clacultae_hash()
I was under the impression that we were agreed on the combination of patches. I won't try to defend your patch from yourself. I'll drop the hash changes from v2 if it helps get things moving along.
Alex
Hi Alex,
On Thu, 20 May 2021 at 17:13, Alex G. mr.nuke.me@gmail.com wrote:
On 5/20/21 12:52 PM, Simon Glass wrote:
Hi Alex,
On Wed, 19 May 2021 at 20:41, Alex G. mr.nuke.me@gmail.com wrote:
On 5/19/21 4:55 PM, Simon Glass wrote:
Hi Alex,
On Wed, 19 May 2021 at 11:44, Alex G mr.nuke.me@gmail.com wrote:
On 5/19/21 11:36 AM, Simon Glass wrote:
Hi Alexandru,
On Mon, 17 May 2021 at 10:38, Alexandru Gagniuc mr.nuke.me@gmail.com wrote: > > From: Simon Glass sjg@chromium.org > > We already have a host Kconfig for SHA1. Use CONFIG_IS_ENABLED(SHA1) > directly in the code shared with the host build, so we can drop the > unnecessary indirection. > > Signed-off-by: Simon Glass sjg@chromium.org > Reviewed-by: Alexandru Gagniuc mr.nuke.me@gmail.com > Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com > --- > common/image-fit.c | 2 +- > include/image.h | 8 -------- > 2 files changed, 1 insertion(+), 9 deletions(-) > > diff --git a/common/image-fit.c b/common/image-fit.c > index e614643fe3..24e92a8e92 100644 > --- a/common/image-fit.c > +++ b/common/image-fit.c > @@ -1218,7 +1218,7 @@ int calculate_hash(const void *data, int data_len, const char *algo, > CHUNKSZ_CRC32); > *((uint32_t *)value) = cpu_to_uimage(*((uint32_t *)value)); > *value_len = 4; > - } else if (IMAGE_ENABLE_SHA1 && strcmp(algo, "sha1") == 0) { > + } else if (CONFIG_IS_ENABLED(SHA1) && strcmp(algo, "sha1") == 0) {
This can only work if the my host Kconfig patch comes first. Otherwise this code will just be skipped on the host.
I was scratching my head too as to why this works in practice, but not in theory. There is a #define CONFIG_SHA1 in image.h.
Although not a perfect fix, we go from two ways to enable SHA1 ("#define IMAGE_ENABLE_SHA1", and "#define CONFIG_SHA1"), to just one. That's why I think this change is an improvement, and part of this series.
No, we really should not do that...everything needs to be in Kconfig.
I agree for target code. But, as a long term solution, let's look at how we can get hash algos in linker lists, like we're proposing to do for crytpo algos. Or I could just drop this change in v2.
Would it not be easier to have a host Kconfig for these? You seem to be going to extreme lengths to avoid it, but it seems like the simplest solution, easy to understand, no effect on code size and scalable to the future.
It's easy for the short term in terms if the goal is to get something merged. It just hides more fundamental issues with the code. For ecample, why is there hash_calculate() and clacultae_hash()
It is just a naming issue, isn't it? They are quite different functions.
I was under the impression that we were agreed on the combination of patches. I won't try to defend your patch from yourself. I'll drop the hash changes from v2 if it helps get things moving along.
I'm OK with this as a short-term fix to get this series through. But I think we are going to end up with a Kconfig solution at some point. What do you think?
Regards, Simon
On 5/20/21 6:17 PM, Simon Glass wrote:
Hi Alex,
On Thu, 20 May 2021 at 17:13, Alex G. mr.nuke.me@gmail.com wrote:
On 5/20/21 12:52 PM, Simon Glass wrote:
Hi Alex,
On Wed, 19 May 2021 at 20:41, Alex G. mr.nuke.me@gmail.com wrote:
On 5/19/21 4:55 PM, Simon Glass wrote:
Hi Alex,
On Wed, 19 May 2021 at 11:44, Alex G mr.nuke.me@gmail.com wrote:
On 5/19/21 11:36 AM, Simon Glass wrote: > Hi Alexandru, > > On Mon, 17 May 2021 at 10:38, Alexandru Gagniuc mr.nuke.me@gmail.com wrote: >> >> From: Simon Glass sjg@chromium.org >> >> We already have a host Kconfig for SHA1. Use CONFIG_IS_ENABLED(SHA1) >> directly in the code shared with the host build, so we can drop the >> unnecessary indirection. >> >> Signed-off-by: Simon Glass sjg@chromium.org >> Reviewed-by: Alexandru Gagniuc mr.nuke.me@gmail.com >> Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com >> --- >> common/image-fit.c | 2 +- >> include/image.h | 8 -------- >> 2 files changed, 1 insertion(+), 9 deletions(-) >> >> diff --git a/common/image-fit.c b/common/image-fit.c >> index e614643fe3..24e92a8e92 100644 >> --- a/common/image-fit.c >> +++ b/common/image-fit.c >> @@ -1218,7 +1218,7 @@ int calculate_hash(const void *data, int data_len, const char *algo, >> CHUNKSZ_CRC32); >> *((uint32_t *)value) = cpu_to_uimage(*((uint32_t *)value)); >> *value_len = 4; >> - } else if (IMAGE_ENABLE_SHA1 && strcmp(algo, "sha1") == 0) { >> + } else if (CONFIG_IS_ENABLED(SHA1) && strcmp(algo, "sha1") == 0) { > > This can only work if the my host Kconfig patch comes first. Otherwise > this code will just be skipped on the host.
I was scratching my head too as to why this works in practice, but not in theory. There is a #define CONFIG_SHA1 in image.h.
Although not a perfect fix, we go from two ways to enable SHA1 ("#define IMAGE_ENABLE_SHA1", and "#define CONFIG_SHA1"), to just one. That's why I think this change is an improvement, and part of this series.
No, we really should not do that...everything needs to be in Kconfig.
I agree for target code. But, as a long term solution, let's look at how we can get hash algos in linker lists, like we're proposing to do for crytpo algos. Or I could just drop this change in v2.
Would it not be easier to have a host Kconfig for these? You seem to be going to extreme lengths to avoid it, but it seems like the simplest solution, easy to understand, no effect on code size and scalable to the future.
It's easy for the short term in terms if the goal is to get something merged. It just hides more fundamental issues with the code. For ecample, why is there hash_calculate() and clacultae_hash()
It is just a naming issue, isn't it? They are quite different functions.
Because one resets the watchdog after every CHUNK bytes and the other doesn't?
I was under the impression that we were agreed on the combination of patches. I won't try to defend your patch from yourself. I'll drop the hash changes from v2 if it helps get things moving along.
I'm OK with this as a short-term fix to get this series through. But I think we are going to end up with a Kconfig solution at some point. What do you think?
I think it's possible and reasonable to have common code without host Kconfig. coreboot did it.
Alex
Hi Alex,
On Thu, 20 May 2021 at 18:07, Alex G. mr.nuke.me@gmail.com wrote:
On 5/20/21 6:17 PM, Simon Glass wrote:
Hi Alex,
On Thu, 20 May 2021 at 17:13, Alex G. mr.nuke.me@gmail.com wrote:
On 5/20/21 12:52 PM, Simon Glass wrote:
Hi Alex,
On Wed, 19 May 2021 at 20:41, Alex G. mr.nuke.me@gmail.com wrote:
On 5/19/21 4:55 PM, Simon Glass wrote:
Hi Alex,
On Wed, 19 May 2021 at 11:44, Alex G mr.nuke.me@gmail.com wrote: > > > > On 5/19/21 11:36 AM, Simon Glass wrote: >> Hi Alexandru, >> >> On Mon, 17 May 2021 at 10:38, Alexandru Gagniuc mr.nuke.me@gmail.com wrote: >>> >>> From: Simon Glass sjg@chromium.org >>> >>> We already have a host Kconfig for SHA1. Use CONFIG_IS_ENABLED(SHA1) >>> directly in the code shared with the host build, so we can drop the >>> unnecessary indirection. >>> >>> Signed-off-by: Simon Glass sjg@chromium.org >>> Reviewed-by: Alexandru Gagniuc mr.nuke.me@gmail.com >>> Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com >>> --- >>> common/image-fit.c | 2 +- >>> include/image.h | 8 -------- >>> 2 files changed, 1 insertion(+), 9 deletions(-) >>> >>> diff --git a/common/image-fit.c b/common/image-fit.c >>> index e614643fe3..24e92a8e92 100644 >>> --- a/common/image-fit.c >>> +++ b/common/image-fit.c >>> @@ -1218,7 +1218,7 @@ int calculate_hash(const void *data, int data_len, const char *algo, >>> CHUNKSZ_CRC32); >>> *((uint32_t *)value) = cpu_to_uimage(*((uint32_t *)value)); >>> *value_len = 4; >>> - } else if (IMAGE_ENABLE_SHA1 && strcmp(algo, "sha1") == 0) { >>> + } else if (CONFIG_IS_ENABLED(SHA1) && strcmp(algo, "sha1") == 0) { >> >> This can only work if the my host Kconfig patch comes first. Otherwise >> this code will just be skipped on the host. > > I was scratching my head too as to why this works in practice, but not > in theory. There is a #define CONFIG_SHA1 in image.h. > > Although not a perfect fix, we go from two ways to enable SHA1 ("#define > IMAGE_ENABLE_SHA1", and "#define CONFIG_SHA1"), to just one. That's why > I think this change is an improvement, and part of this series.
No, we really should not do that...everything needs to be in Kconfig.
I agree for target code. But, as a long term solution, let's look at how we can get hash algos in linker lists, like we're proposing to do for crytpo algos. Or I could just drop this change in v2.
Would it not be easier to have a host Kconfig for these? You seem to be going to extreme lengths to avoid it, but it seems like the simplest solution, easy to understand, no effect on code size and scalable to the future.
It's easy for the short term in terms if the goal is to get something merged. It just hides more fundamental issues with the code. For ecample, why is there hash_calculate() and clacultae_hash()
It is just a naming issue, isn't it? They are quite different functions.
Because one resets the watchdog after every CHUNK bytes and the other doesn't?
Well hash_calculate() is used for hashing parts of a devicetree, so is quite a different function.
I was under the impression that we were agreed on the combination of patches. I won't try to defend your patch from yourself. I'll drop the hash changes from v2 if it helps get things moving along.
I'm OK with this as a short-term fix to get this series through. But I think we are going to end up with a Kconfig solution at some point. What do you think?
I think it's possible and reasonable to have common code without host Kconfig. coreboot did it.
There is very little code shared between target and tools there. I am sure there is some, but can't think of anything except some library functions. There is also no equivalent of CONFIG_IS_ENABLED(), but instead a log of ENV_... macros and entirely separate rules in the Makefile.
Can you point to another way to do this? I think your approach is valuable in untangling code that does not need to be shared, but I still think that the host Kconfig thing is a great idea for everything else. It isn't my idea, but Rasmus' (now on cc).
Tom, do you have any thoughts?
Regards, SImon
On 5/21/21 2:39 PM, Simon Glass wrote:
Hi Alex,
On Thu, 20 May 2021 at 18:07, Alex G. mr.nuke.me@gmail.com wrote:
On 5/20/21 6:17 PM, Simon Glass wrote:
Hi Alex,
On Thu, 20 May 2021 at 17:13, Alex G. mr.nuke.me@gmail.com wrote:
On 5/20/21 12:52 PM, Simon Glass wrote:
Hi Alex,
On Wed, 19 May 2021 at 20:41, Alex G. mr.nuke.me@gmail.com wrote:
On 5/19/21 4:55 PM, Simon Glass wrote: > Hi Alex, > > On Wed, 19 May 2021 at 11:44, Alex G mr.nuke.me@gmail.com wrote: >> >> >> >> On 5/19/21 11:36 AM, Simon Glass wrote: >>> Hi Alexandru, >>> >>> On Mon, 17 May 2021 at 10:38, Alexandru Gagniuc mr.nuke.me@gmail.com wrote: >>>> >>>> From: Simon Glass sjg@chromium.org >>>> >>>> We already have a host Kconfig for SHA1. Use CONFIG_IS_ENABLED(SHA1) >>>> directly in the code shared with the host build, so we can drop the >>>> unnecessary indirection. >>>> >>>> Signed-off-by: Simon Glass sjg@chromium.org >>>> Reviewed-by: Alexandru Gagniuc mr.nuke.me@gmail.com >>>> Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com >>>> --- >>>> common/image-fit.c | 2 +- >>>> include/image.h | 8 -------- >>>> 2 files changed, 1 insertion(+), 9 deletions(-) >>>> >>>> diff --git a/common/image-fit.c b/common/image-fit.c >>>> index e614643fe3..24e92a8e92 100644 >>>> --- a/common/image-fit.c >>>> +++ b/common/image-fit.c >>>> @@ -1218,7 +1218,7 @@ int calculate_hash(const void *data, int data_len, const char *algo, >>>> CHUNKSZ_CRC32); >>>> *((uint32_t *)value) = cpu_to_uimage(*((uint32_t *)value)); >>>> *value_len = 4; >>>> - } else if (IMAGE_ENABLE_SHA1 && strcmp(algo, "sha1") == 0) { >>>> + } else if (CONFIG_IS_ENABLED(SHA1) && strcmp(algo, "sha1") == 0) { >>> >>> This can only work if the my host Kconfig patch comes first. Otherwise >>> this code will just be skipped on the host. >> >> I was scratching my head too as to why this works in practice, but not >> in theory. There is a #define CONFIG_SHA1 in image.h. >> >> Although not a perfect fix, we go from two ways to enable SHA1 ("#define >> IMAGE_ENABLE_SHA1", and "#define CONFIG_SHA1"), to just one. That's why >> I think this change is an improvement, and part of this series. > > No, we really should not do that...everything needs to be in Kconfig.
I agree for target code. But, as a long term solution, let's look at how we can get hash algos in linker lists, like we're proposing to do for crytpo algos. Or I could just drop this change in v2.
Would it not be easier to have a host Kconfig for these? You seem to be going to extreme lengths to avoid it, but it seems like the simplest solution, easy to understand, no effect on code size and scalable to the future.
It's easy for the short term in terms if the goal is to get something merged. It just hides more fundamental issues with the code. For ecample, why is there hash_calculate() and clacultae_hash()
It is just a naming issue, isn't it? They are quite different functions.
Because one resets the watchdog after every CHUNK bytes and the other doesn't?
Well hash_calculate() is used for hashing parts of a devicetree, so is quite a different function.
I was under the impression that we were agreed on the combination of patches. I won't try to defend your patch from yourself. I'll drop the hash changes from v2 if it helps get things moving along.
I'm OK with this as a short-term fix to get this series through. But I think we are going to end up with a Kconfig solution at some point. What do you think?
I think it's possible and reasonable to have common code without host Kconfig. coreboot did it.
There is very little code shared between target and tools there. I am sure there is some, but can't think of anything except some library functions. There is also no equivalent of CONFIG_IS_ENABLED(), but instead a log of ENV_... macros and entirely separate rules in the Makefile.
Can you point to another way to do this? I think your approach is valuable in untangling code that does not need to be shared, but I still think that the host Kconfig thing is a great idea for everything else. It isn't my idea, but Rasmus' (now on cc).
I have a couple of ideas to start, but nothing submittable.
Let's start with hash_calculate(). It's just a big switch() with string processing. But we've already done part of the work in fit_image_check_hash(). So instead of stopping at a "char *algo", why not get an actual "struct hash_algo *" with the correct ops to begin with, and not need a huge switch() function() ?
We have "struct hash_algo" and "struct checksum_algo" that seem to serve very similar purposes. Would it actually make sense to merge them?
And if that is done, you open the gates to significantly reducing the CONFIG_IS_ENABLED() use for hashes, as well as really simplify the hash selection in Kconfig.
In order to do this, we are reducing the occurrence of IS_ENABLED(), which is just an eye-candy version of #ifdef. This leads to the natural conclusion of eliminating IS_ENABLED() from common code.
Hi Alex,
On Mon, 24 May 2021 at 13:59, Alex G. mr.nuke.me@gmail.com wrote:
On 5/21/21 2:39 PM, Simon Glass wrote:
Hi Alex,
On Thu, 20 May 2021 at 18:07, Alex G. mr.nuke.me@gmail.com wrote:
On 5/20/21 6:17 PM, Simon Glass wrote:
Hi Alex,
On Thu, 20 May 2021 at 17:13, Alex G. mr.nuke.me@gmail.com wrote:
On 5/20/21 12:52 PM, Simon Glass wrote:
Hi Alex,
On Wed, 19 May 2021 at 20:41, Alex G. mr.nuke.me@gmail.com wrote: > > > > On 5/19/21 4:55 PM, Simon Glass wrote: >> Hi Alex, >> >> On Wed, 19 May 2021 at 11:44, Alex G mr.nuke.me@gmail.com wrote: >>> >>> >>> >>> On 5/19/21 11:36 AM, Simon Glass wrote: >>>> Hi Alexandru, >>>> >>>> On Mon, 17 May 2021 at 10:38, Alexandru Gagniuc mr.nuke.me@gmail.com wrote: >>>>> >>>>> From: Simon Glass sjg@chromium.org >>>>> >>>>> We already have a host Kconfig for SHA1. Use CONFIG_IS_ENABLED(SHA1) >>>>> directly in the code shared with the host build, so we can drop the >>>>> unnecessary indirection. >>>>> >>>>> Signed-off-by: Simon Glass sjg@chromium.org >>>>> Reviewed-by: Alexandru Gagniuc mr.nuke.me@gmail.com >>>>> Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com >>>>> --- >>>>> common/image-fit.c | 2 +- >>>>> include/image.h | 8 -------- >>>>> 2 files changed, 1 insertion(+), 9 deletions(-) >>>>> >>>>> diff --git a/common/image-fit.c b/common/image-fit.c >>>>> index e614643fe3..24e92a8e92 100644 >>>>> --- a/common/image-fit.c >>>>> +++ b/common/image-fit.c >>>>> @@ -1218,7 +1218,7 @@ int calculate_hash(const void *data, int data_len, const char *algo, >>>>> CHUNKSZ_CRC32); >>>>> *((uint32_t *)value) = cpu_to_uimage(*((uint32_t *)value)); >>>>> *value_len = 4; >>>>> - } else if (IMAGE_ENABLE_SHA1 && strcmp(algo, "sha1") == 0) { >>>>> + } else if (CONFIG_IS_ENABLED(SHA1) && strcmp(algo, "sha1") == 0) { >>>> >>>> This can only work if the my host Kconfig patch comes first. Otherwise >>>> this code will just be skipped on the host. >>> >>> I was scratching my head too as to why this works in practice, but not >>> in theory. There is a #define CONFIG_SHA1 in image.h. >>> >>> Although not a perfect fix, we go from two ways to enable SHA1 ("#define >>> IMAGE_ENABLE_SHA1", and "#define CONFIG_SHA1"), to just one. That's why >>> I think this change is an improvement, and part of this series. >> >> No, we really should not do that...everything needs to be in Kconfig. > > I agree for target code. But, as a long term solution, let's look at how > we can get hash algos in linker lists, like we're proposing to do for > crytpo algos. Or I could just drop this change in v2.
Would it not be easier to have a host Kconfig for these? You seem to be going to extreme lengths to avoid it, but it seems like the simplest solution, easy to understand, no effect on code size and scalable to the future.
It's easy for the short term in terms if the goal is to get something merged. It just hides more fundamental issues with the code. For ecample, why is there hash_calculate() and clacultae_hash()
It is just a naming issue, isn't it? They are quite different functions.
Because one resets the watchdog after every CHUNK bytes and the other doesn't?
Well hash_calculate() is used for hashing parts of a devicetree, so is quite a different function.
I was under the impression that we were agreed on the combination of patches. I won't try to defend your patch from yourself. I'll drop the hash changes from v2 if it helps get things moving along.
I'm OK with this as a short-term fix to get this series through. But I think we are going to end up with a Kconfig solution at some point. What do you think?
I think it's possible and reasonable to have common code without host Kconfig. coreboot did it.
There is very little code shared between target and tools there. I am sure there is some, but can't think of anything except some library functions. There is also no equivalent of CONFIG_IS_ENABLED(), but instead a log of ENV_... macros and entirely separate rules in the Makefile.
Can you point to another way to do this? I think your approach is valuable in untangling code that does not need to be shared, but I still think that the host Kconfig thing is a great idea for everything else. It isn't my idea, but Rasmus' (now on cc).
I have a couple of ideas to start, but nothing submittable.
Let's start with hash_calculate(). It's just a big switch() with string processing. But we've already done part of the work in fit_image_check_hash(). So instead of stopping at a "char *algo", why not get an actual "struct hash_algo *" with the correct ops to begin with, and not need a huge switch() function() ?
We have "struct hash_algo" and "struct checksum_algo" that seem to serve very similar purposes. Would it actually make sense to merge them?
I'm not convinced it is. The checksum_algo thing allows checksumming lots of regions. We could perhaps have a helper function that reads each region and runs it through the hash_algo API. Then we could drop checksum_algo perhaps? Of course we still have the calculate_sign() call but I presume you could make that host-only.
And if that is done, you open the gates to significantly reducing the CONFIG_IS_ENABLED() use for hashes, as well as really simplify the hash selection in Kconfig.
In order to do this, we are reducing the occurrence of IS_ENABLED(), which is just an eye-candy version of #ifdef. This leads to the natural conclusion of eliminating IS_ENABLED() from common code.
Well if you are going to do this, let's see the patches. What's the plan for getting rid of the '#define CONFIG_xxxx' on the host side?
Regards, Simon
From: Simon Glass sjg@chromium.org
We already have a host Kconfig for these SHA options. Use CONFIG_IS_ENABLED(SHAxxx) directly in the code shared with the host build, so we can drop the unnecessary indirections.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Alexandru Gagniuc mr.nuke.me@gmail.com Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com --- common/image-fit.c | 6 +++--- include/image.h | 21 --------------------- 2 files changed, 3 insertions(+), 24 deletions(-)
diff --git a/common/image-fit.c b/common/image-fit.c index 24e92a8e92..c0c75e92ba 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -1222,15 +1222,15 @@ int calculate_hash(const void *data, int data_len, const char *algo, 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) { + } else if (CONFIG_IS_ENABLED(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) { + } else if (CONFIG_IS_ENABLED(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) { + } else if (CONFIG_IS_ENABLED(SHA512) && strcmp(algo, "sha512") == 0) { sha512_csum_wd((unsigned char *)data, data_len, (unsigned char *)value, CHUNKSZ_SHA512); *value_len = SHA512_SUM_LEN; diff --git a/include/image.h b/include/image.h index 8c718adba0..aa03f0a722 100644 --- a/include/image.h +++ b/include/image.h @@ -81,27 +81,6 @@ struct fdt_region; #define IMAGE_ENABLE_MD5 0 #endif
-#if defined(CONFIG_FIT_SHA256) || \ - defined(CONFIG_SPL_FIT_SHA256) -#define IMAGE_ENABLE_SHA256 1 -#else -#define IMAGE_ENABLE_SHA256 0 -#endif - -#if defined(CONFIG_FIT_SHA384) || \ - defined(CONFIG_SPL_FIT_SHA384) -#define IMAGE_ENABLE_SHA384 1 -#else -#define IMAGE_ENABLE_SHA384 0 -#endif - -#if defined(CONFIG_FIT_SHA512) || \ - defined(CONFIG_SPL_FIT_SHA512) -#define IMAGE_ENABLE_SHA512 1 -#else -#define IMAGE_ENABLE_SHA512 0 -#endif - #endif /* IMAGE_ENABLE_FIT */
#ifdef CONFIG_SYS_BOOT_GET_CMDLINE
From: Simon Glass sjg@chromium.org
This is not needed with Kconfig, since we can use IS_ENABLED() easily enough. Drop it.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Alexandru Gagniuc mr.nuke.me@gmail.com Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com --- common/image-fit.c | 2 +- include/image.h | 5 ----- 2 files changed, 1 insertion(+), 6 deletions(-)
diff --git a/common/image-fit.c b/common/image-fit.c index c0c75e92ba..5df2cf8571 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -2026,7 +2026,7 @@ int fit_image_load(bootm_headers_t *images, ulong addr, * fit_conf_get_node() will try to find default config node */ bootstage_mark(bootstage_id + BOOTSTAGE_SUB_NO_UNIT_NAME); - if (IMAGE_ENABLE_BEST_MATCH && !fit_uname_config) { + if (IS_ENABLED(CONFIG_FIT_BEST_MATCH) && !fit_uname_config) { cfg_noffset = fit_conf_find_compat(fit, gd_fdt_blob()); } else { cfg_noffset = fit_conf_get_node(fit, diff --git a/include/image.h b/include/image.h index aa03f0a722..dbb24993f1 100644 --- a/include/image.h +++ b/include/image.h @@ -1221,11 +1221,6 @@ void image_set_host_blob(void *host_blob); # define gd_fdt_blob() (gd->fdt_blob) #endif
-#ifdef CONFIG_FIT_BEST_MATCH -#define IMAGE_ENABLE_BEST_MATCH 1 -#else -#define IMAGE_ENABLE_BEST_MATCH 0 -#endif #endif /* IMAGE_ENABLE_FIT */
/*
image-sig.c is used to map a hash or crypto algorithm name to a handler of that algorithm. There is some similarity between the host and target variants, with the differences worked out by #ifdefs. The purpose of this change is to remove those ifdefs.
First, copy the file to a host-only version, and remove target specific code. Although it looks like we are duplicating code, subsequent patches will change the way target algorithms are searched. Besides we are only duplicating three string to struct mapping functions. This isn't something to fuss about.
Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com --- tools/Makefile | 5 +- tools/image-sig-host.c | 133 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 136 insertions(+), 2 deletions(-) create mode 100644 tools/image-sig-host.c
diff --git a/tools/Makefile b/tools/Makefile index d020c55d66..e39006b6f6 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -57,8 +57,9 @@ hostprogs-$(CONFIG_FIT_SIGNATURE) += fit_info fit_check_sign
hostprogs-$(CONFIG_CMD_BOOTEFI_SELFTEST) += file2include
-FIT_OBJS-$(CONFIG_FIT) := fit_common.o fit_image.o image-host.o common/image-fit.o -FIT_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := common/image-sig.o common/image-fit-sig.o +FIT_OBJS-$(CONFIG_FIT) := image-sig-host.o fit_common.o fit_image.o \ + image-host.o common/image-fit.o +FIT_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := common/image-fit-sig.o FIT_CIPHER_OBJS-$(CONFIG_FIT_CIPHER) := common/image-cipher.o
# The following files are synced with upstream DTC. diff --git a/tools/image-sig-host.c b/tools/image-sig-host.c new file mode 100644 index 0000000000..8ed6998dab --- /dev/null +++ b/tools/image-sig-host.c @@ -0,0 +1,133 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (c) 2013, Google Inc. + */ + +#include "mkimage.h" +#include <fdt_support.h> +#include <time.h> +#include <linux/libfdt.h> +#include <image.h> +#include <u-boot/ecdsa.h> +#include <u-boot/rsa.h> +#include <u-boot/hash-checksum.h> + +struct checksum_algo checksum_algos[] = { + { + .name = "sha1", + .checksum_len = SHA1_SUM_LEN, + .der_len = SHA1_DER_LEN, + .der_prefix = sha1_der_prefix, + .calculate_sign = EVP_sha1, + .calculate = hash_calculate, + }, + { + .name = "sha256", + .checksum_len = SHA256_SUM_LEN, + .der_len = SHA256_DER_LEN, + .der_prefix = sha256_der_prefix, + .calculate_sign = EVP_sha256, + .calculate = hash_calculate, + }, + { + .name = "sha384", + .checksum_len = SHA384_SUM_LEN, + .der_len = SHA384_DER_LEN, + .der_prefix = sha384_der_prefix, + .calculate_sign = EVP_sha384, + .calculate = hash_calculate, + }, + { + .name = "sha512", + .checksum_len = SHA512_SUM_LEN, + .der_len = SHA512_DER_LEN, + .der_prefix = sha512_der_prefix, + .calculate_sign = EVP_sha512, + .calculate = hash_calculate, + }, +}; + +struct crypto_algo crypto_algos[] = { + { + .name = "rsa2048", + .key_len = RSA2048_BYTES, + .sign = rsa_sign, + .add_verify_data = rsa_add_verify_data, + .verify = rsa_verify, + }, + { + .name = "rsa4096", + .key_len = RSA4096_BYTES, + .sign = rsa_sign, + .add_verify_data = rsa_add_verify_data, + .verify = rsa_verify, + }, + { + .name = "ecdsa256", + .key_len = ECDSA256_BYTES, + .sign = ecdsa_sign, + .add_verify_data = ecdsa_add_verify_data, + .verify = ecdsa_verify, + }, +}; + +struct padding_algo padding_algos[] = { + { + .name = "pkcs-1.5", + .verify = padding_pkcs_15_verify, + }, + { + .name = "pss", + .verify = padding_pss_verify, + } +}; + +struct checksum_algo *image_get_checksum_algo(const char *full_name) +{ + int i; + const char *name; + + for (i = 0; i < ARRAY_SIZE(checksum_algos); i++) { + name = checksum_algos[i].name; + /* Make sure names match and next char is a comma */ + if (!strncmp(name, full_name, strlen(name)) && + full_name[strlen(name)] == ',') + return &checksum_algos[i]; + } + + return NULL; +} + +struct crypto_algo *image_get_crypto_algo(const char *full_name) +{ + int i; + const char *name; + + /* Move name to after the comma */ + name = strchr(full_name, ','); + if (!name) + return NULL; + name += 1; + + for (i = 0; i < ARRAY_SIZE(crypto_algos); i++) { + if (!strcmp(crypto_algos[i].name, name)) + return &crypto_algos[i]; + } + + return NULL; +} + +struct padding_algo *image_get_padding_algo(const char *name) +{ + int i; + + if (!name) + return NULL; + + for (i = 0; i < ARRAY_SIZE(padding_algos); i++) { + if (!strcmp(padding_algos[i].name, name)) + return &padding_algos[i]; + } + + return NULL; +}
On 5/17/21 11:38 AM, Alexandru Gagniuc wrote:
image-sig.c is used to map a hash or crypto algorithm name to a handler of that algorithm. There is some similarity between the host and target variants, with the differences worked out by #ifdefs. The purpose of this change is to remove those ifdefs.
First, copy the file to a host-only version, and remove target specific code. Although it looks like we are duplicating code, subsequent patches will change the way target algorithms are searched. Besides we are only duplicating three string to struct mapping functions. This isn't something to fuss about.
Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com
tools/Makefile | 5 +- tools/image-sig-host.c | 133 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 136 insertions(+), 2 deletions(-) create mode 100644 tools/image-sig-host.c
diff --git a/tools/Makefile b/tools/Makefile index d020c55d66..e39006b6f6 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -57,8 +57,9 @@ hostprogs-$(CONFIG_FIT_SIGNATURE) += fit_info fit_check_sign
hostprogs-$(CONFIG_CMD_BOOTEFI_SELFTEST) += file2include
-FIT_OBJS-$(CONFIG_FIT) := fit_common.o fit_image.o image-host.o common/image-fit.o -FIT_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := common/image-sig.o common/image-fit-sig.o +FIT_OBJS-$(CONFIG_FIT) := image-sig-host.o fit_common.o fit_image.o \
image-host.o common/image-fit.o+FIT_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := common/image-fit-sig.o FIT_CIPHER_OBJS-$(CONFIG_FIT_CIPHER) := common/image-cipher.o
This may cause a build failure with FIT_SIGNATURE disabled. I will have this fixed in v2. The correction is trivial.
Correct diff below for reference:
FIT_OBJS-$(CONFIG_FIT) := fit_common.o fit_image.o image-host.o common/image-fit.o -FIT_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := common/image-sig.o common/image-fit-sig.o +FIT_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := image-sig-host.o common/image-fit-sig.o
On Mon, 17 May 2021 at 13:47, Alex G. mr.nuke.me@gmail.com wrote:
On 5/17/21 11:38 AM, Alexandru Gagniuc wrote:
image-sig.c is used to map a hash or crypto algorithm name to a handler of that algorithm. There is some similarity between the host and target variants, with the differences worked out by #ifdefs. The purpose of this change is to remove those ifdefs.
First, copy the file to a host-only version, and remove target specific code. Although it looks like we are duplicating code, subsequent patches will change the way target algorithms are searched. Besides we are only duplicating three string to struct mapping functions. This isn't something to fuss about.
Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com
tools/Makefile | 5 +- tools/image-sig-host.c | 133 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 136 insertions(+), 2 deletions(-) create mode 100644 tools/image-sig-host.c
diff --git a/tools/Makefile b/tools/Makefile index d020c55d66..e39006b6f6 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -57,8 +57,9 @@ hostprogs-$(CONFIG_FIT_SIGNATURE) += fit_info fit_check_sign
hostprogs-$(CONFIG_CMD_BOOTEFI_SELFTEST) += file2include
-FIT_OBJS-$(CONFIG_FIT) := fit_common.o fit_image.o image-host.o common/image-fit.o -FIT_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := common/image-sig.o common/image-fit-sig.o +FIT_OBJS-$(CONFIG_FIT) := image-sig-host.o fit_common.o fit_image.o \
image-host.o common/image-fit.o+FIT_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := common/image-fit-sig.o FIT_CIPHER_OBJS-$(CONFIG_FIT_CIPHER) := common/image-cipher.o
This may cause a build failure with FIT_SIGNATURE disabled. I will have this fixed in v2. The correction is trivial.
I see a build warning for an unused variable 'i', if that is what you mean.
Correct diff below for reference:
FIT_OBJS-$(CONFIG_FIT) := fit_common.o fit_image.o image-host.o common/image-fit.o -FIT_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := common/image-sig.o common/image-fit-sig.o +FIT_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := image-sig-host.o common/image-fit-sig.o
Reviewed-by: Simon Glass sjg@chromium.org
Remove any ifdefs in image-sig.c that were previously used to differentiate from the host code. Note that all code dedicated to relocating ->sign() and ->add_verify_data)_ can be safely removed, as signing is not supported target-side.
NOTE that although it appears we are removing ecdsa256 support, this is intentional. ecdsa_verify() is a no-op on the target, and is currently only used by host code.
Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com Reviewed-by: Simon Glass sjg@chromium.org --- common/image-sig.c | 39 ++------------------------------------- 1 file changed, 2 insertions(+), 37 deletions(-)
diff --git a/common/image-sig.c b/common/image-sig.c index 8b5cecbfa4..5e2d171975 100644 --- a/common/image-sig.c +++ b/common/image-sig.c @@ -3,18 +3,11 @@ * Copyright (c) 2013, Google Inc. */
-#ifdef USE_HOSTCC -#include "mkimage.h" -#include <fdt_support.h> -#include <time.h> -#include <linux/libfdt.h> -#else #include <common.h> #include <log.h> #include <malloc.h> #include <asm/global_data.h> DECLARE_GLOBAL_DATA_PTR; -#endif /* !USE_HOSTCC*/ #include <image.h> #include <u-boot/ecdsa.h> #include <u-boot/rsa.h> @@ -28,9 +21,6 @@ struct checksum_algo checksum_algos[] = { .checksum_len = SHA1_SUM_LEN, .der_len = SHA1_DER_LEN, .der_prefix = sha1_der_prefix, -#if IMAGE_ENABLE_SIGN - .calculate_sign = EVP_sha1, -#endif .calculate = hash_calculate, }, { @@ -38,9 +28,6 @@ struct checksum_algo checksum_algos[] = { .checksum_len = SHA256_SUM_LEN, .der_len = SHA256_DER_LEN, .der_prefix = sha256_der_prefix, -#if IMAGE_ENABLE_SIGN - .calculate_sign = EVP_sha256, -#endif .calculate = hash_calculate, }, #ifdef CONFIG_SHA384 @@ -49,9 +36,6 @@ struct checksum_algo checksum_algos[] = { .checksum_len = SHA384_SUM_LEN, .der_len = SHA384_DER_LEN, .der_prefix = sha384_der_prefix, -#if IMAGE_ENABLE_SIGN - .calculate_sign = EVP_sha384, -#endif .calculate = hash_calculate, }, #endif @@ -61,9 +45,6 @@ struct checksum_algo checksum_algos[] = { .checksum_len = SHA512_SUM_LEN, .der_len = SHA512_DER_LEN, .der_prefix = sha512_der_prefix, -#if IMAGE_ENABLE_SIGN - .calculate_sign = EVP_sha512, -#endif .calculate = hash_calculate, }, #endif @@ -74,24 +55,13 @@ struct crypto_algo crypto_algos[] = { { .name = "rsa2048", .key_len = RSA2048_BYTES, - .sign = rsa_sign, - .add_verify_data = rsa_add_verify_data, .verify = rsa_verify, }, { .name = "rsa4096", .key_len = RSA4096_BYTES, - .sign = rsa_sign, - .add_verify_data = rsa_add_verify_data, .verify = rsa_verify, }, - { - .name = "ecdsa256", - .key_len = ECDSA256_BYTES, - .sign = ecdsa_sign, - .add_verify_data = ecdsa_add_verify_data, - .verify = ecdsa_verify, - }, };
struct padding_algo padding_algos[] = { @@ -112,16 +82,13 @@ struct checksum_algo *image_get_checksum_algo(const char *full_name) int i; const char *name;
-#if !defined(USE_HOSTCC) && defined(CONFIG_NEEDS_MANUAL_RELOC) +#if defined(CONFIG_NEEDS_MANUAL_RELOC) static bool done;
if (!done) { done = true; for (i = 0; i < ARRAY_SIZE(checksum_algos); i++) { checksum_algos[i].name += gd->reloc_off; -#if IMAGE_ENABLE_SIGN - checksum_algos[i].calculate_sign += gd->reloc_off; -#endif checksum_algos[i].calculate += gd->reloc_off; } } @@ -143,15 +110,13 @@ struct crypto_algo *image_get_crypto_algo(const char *full_name) int i; const char *name;
-#if !defined(USE_HOSTCC) && defined(CONFIG_NEEDS_MANUAL_RELOC) +#if defined(CONFIG_NEEDS_MANUAL_RELOC) static bool done;
if (!done) { done = true; for (i = 0; i < ARRAY_SIZE(crypto_algos); i++) { crypto_algos[i].name += gd->reloc_off; - crypto_algos[i].sign += gd->reloc_off; - crypto_algos[i].add_verify_data += gd->reloc_off; crypto_algos[i].verify += gd->reloc_off; } }
The purpose of this change is to enable crypto algorithms to be placed in linker lists, rather than be declared as a static array. The goal is to remove the crypto_algos array in a subsequent patch.
Create a new linker list named "cryptos", and search it when image_get_crypto_algo() is invoked.
NOTE that adding support for manual relocation of crypto_algos within linker lists is beyond the scope of this patch.
Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com Reviewed-by: Simon Glass sjg@chromium.org --- common/image-sig.c | 9 +++++++++ include/image.h | 5 +++++ 2 files changed, 14 insertions(+)
diff --git a/common/image-sig.c b/common/image-sig.c index 5e2d171975..81a3b739fe 100644 --- a/common/image-sig.c +++ b/common/image-sig.c @@ -107,6 +107,7 @@ struct checksum_algo *image_get_checksum_algo(const char *full_name)
struct crypto_algo *image_get_crypto_algo(const char *full_name) { + struct crypto_algo *crypto, *end; int i; const char *name;
@@ -133,6 +134,14 @@ struct crypto_algo *image_get_crypto_algo(const char *full_name) return &crypto_algos[i]; }
+ crypto = ll_entry_start(struct crypto_algo, cryptos); + end = ll_entry_end(struct crypto_algo, cryptos); + for (; crypto < end; crypto++) { + if (!strcmp(crypto->name, name)) + return crypto; + } + + /* Not found */ return NULL; }
diff --git a/include/image.h b/include/image.h index dbb24993f1..f7f8f8a029 100644 --- a/include/image.h +++ b/include/image.h @@ -47,6 +47,7 @@ struct fdt_region; #include <lmb.h> #include <asm/u-boot.h> #include <command.h> +#include <linker_lists.h>
/* Take notice of the 'ignore' property for hashes */ #define IMAGE_ENABLE_IGNORE 1 @@ -1328,6 +1329,10 @@ struct crypto_algo { uint8_t *sig, uint sig_len); };
+/* Declare a new U-Boot crypto algorithm handler */ +#define U_BOOT_CRYPTO_ALGO(__name) \ +ll_entry_declare(struct crypto_algo, __name, cryptos) + struct padding_algo { const char *name; int (*verify)(struct image_sign_info *info,
Move the RSA verification crytpo_algo structure out of the crypto_algos array, and into a linker list.
Although it appears we are adding an #ifdef to rsa-verify.c, the gains outweigh this small inconvenience. This is because rsa_verify() is defined differently based on #ifdefs. This change allows us to have a single definition of rsa_verify().
Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com Reviewed-by: Simon Glass sjg@chromium.org --- common/image-sig.c | 9 --------- lib/rsa/rsa-verify.c | 16 ++++++++++++++++ 2 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/common/image-sig.c b/common/image-sig.c index 81a3b739fe..d996b7ba50 100644 --- a/common/image-sig.c +++ b/common/image-sig.c @@ -53,15 +53,6 @@ struct checksum_algo checksum_algos[] = {
struct crypto_algo crypto_algos[] = { { - .name = "rsa2048", - .key_len = RSA2048_BYTES, - .verify = rsa_verify, - }, - { - .name = "rsa4096", - .key_len = RSA4096_BYTES, - .verify = rsa_verify, - }, };
struct padding_algo padding_algos[] = { diff --git a/lib/rsa/rsa-verify.c b/lib/rsa/rsa-verify.c index 1998c773fc..bb8cc61d94 100644 --- a/lib/rsa/rsa-verify.c +++ b/lib/rsa/rsa-verify.c @@ -571,3 +571,19 @@ int rsa_verify(struct image_sign_info *info,
return rsa_verify_hash(info, hash, sig, sig_len); } + +#ifndef USE_HOSTCC + +U_BOOT_CRYPTO_ALGO(rsa2048) = { + .name = "rsa2048", + .key_len = RSA2048_BYTES, + .verify = rsa_verify, +}; + +U_BOOT_CRYPTO_ALGO(rsa4096) = { + .name = "rsa4096", + .key_len = RSA4096_BYTES, + .verify = rsa_verify, +}; + +#endif
Crytographic algorithms (currently RSA), are stored in linker lists. The crypto_algos array is unused, so remove it, and any logic associated with it.
Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com Reviewed-by: Simon Glass sjg@chromium.org --- common/image-sig.c | 21 --------------------- 1 file changed, 21 deletions(-)
diff --git a/common/image-sig.c b/common/image-sig.c index d996b7ba50..498cd78af4 100644 --- a/common/image-sig.c +++ b/common/image-sig.c @@ -51,10 +51,6 @@ struct checksum_algo checksum_algos[] = {
};
-struct crypto_algo crypto_algos[] = { - { -}; - struct padding_algo padding_algos[] = { { .name = "pkcs-1.5", @@ -102,29 +98,12 @@ struct crypto_algo *image_get_crypto_algo(const char *full_name) int i; const char *name;
-#if defined(CONFIG_NEEDS_MANUAL_RELOC) - static bool done; - - if (!done) { - done = true; - for (i = 0; i < ARRAY_SIZE(crypto_algos); i++) { - crypto_algos[i].name += gd->reloc_off; - crypto_algos[i].verify += gd->reloc_off; - } - } -#endif - /* Move name to after the comma */ name = strchr(full_name, ','); if (!name) return NULL; name += 1;
- for (i = 0; i < ARRAY_SIZE(crypto_algos); i++) { - if (!strcmp(crypto_algos[i].name, name)) - return &crypto_algos[i]; - } - crypto = ll_entry_start(struct crypto_algo, cryptos); end = ll_entry_end(struct crypto_algo, cryptos); for (; crypto < end; crypto++) {
It is no longer necessary to implement ecdsa_() functions as no-ops depending on config options. It is merely sufficient to provide the prototypes, as the ecdsa code is no longer linked when unused.
Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com Reviewed-by: Simon Glass sjg@chromium.org --- include/u-boot/ecdsa.h | 25 ------------------------- 1 file changed, 25 deletions(-)
diff --git a/include/u-boot/ecdsa.h b/include/u-boot/ecdsa.h index 979690d966..f6951c7346 100644 --- a/include/u-boot/ecdsa.h +++ b/include/u-boot/ecdsa.h @@ -15,7 +15,6 @@ * @see "struct crypto_algo" * @{ */ -#if IMAGE_ENABLE_SIGN /** * sign() - calculate and return signature for given input data * @@ -49,22 +48,7 @@ int ecdsa_sign(struct image_sign_info *info, const struct image_region region[], * other -ve value on error */ int ecdsa_add_verify_data(struct image_sign_info *info, void *keydest); -#else -static inline -int ecdsa_sign(struct image_sign_info *info, const struct image_region region[], - int region_count, uint8_t **sigp, uint *sig_len) -{ - return -ENXIO; -} - -static inline -int ecdsa_add_verify_data(struct image_sign_info *info, void *keydest) -{ - return -ENXIO; -} -#endif
-#if IMAGE_ENABLE_VERIFY_ECDSA /** * verify() - Verify a signature against some data * @@ -78,15 +62,6 @@ int ecdsa_add_verify_data(struct image_sign_info *info, void *keydest) int ecdsa_verify(struct image_sign_info *info, const struct image_region region[], int region_count, uint8_t *sig, uint sig_len); -#else -static inline -int ecdsa_verify(struct image_sign_info *info, - const struct image_region region[], int region_count, - uint8_t *sig, uint sig_len) -{ - return -ENXIO; -} -#endif /** @} */
#define ECDSA256_BYTES (256 / 8)
It is no longer necessary to implement rsa_() functions as no-ops depending on config options. It is merely sufficient to provide the prototypes, as the rsa code is no longer linked when unused.
Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com Reviewed-by: Simon Glass sjg@chromium.org --- include/u-boot/rsa.h | 47 -------------------------------------------- 1 file changed, 47 deletions(-)
diff --git a/include/u-boot/rsa.h b/include/u-boot/rsa.h index bc564d56fa..89a9c4caa0 100644 --- a/include/u-boot/rsa.h +++ b/include/u-boot/rsa.h @@ -31,7 +31,6 @@ struct rsa_public_key {
struct image_sign_info;
-#if IMAGE_ENABLE_SIGN /** * sign() - calculate and return signature for given input data * @@ -66,22 +65,7 @@ int rsa_sign(struct image_sign_info *info, other -ve value on error */ int rsa_add_verify_data(struct image_sign_info *info, void *keydest); -#else -static inline int rsa_sign(struct image_sign_info *info, - const struct image_region region[], int region_count, - uint8_t **sigp, uint *sig_len) -{ - return -ENXIO; -} - -static inline int rsa_add_verify_data(struct image_sign_info *info, - void *keydest) -{ - return -ENXIO; -} -#endif
-#if IMAGE_ENABLE_VERIFY /** * rsa_verify_hash() - Verify a signature against a hash * @@ -124,37 +108,6 @@ int padding_pss_verify(struct image_sign_info *info, uint8_t *msg, int msg_len, const uint8_t *hash, int hash_len); #endif /* CONFIG_FIT_RSASSA_PSS */ -#else -static inline int rsa_verify_hash(struct image_sign_info *info, - const uint8_t *hash, - uint8_t *sig, uint sig_len) -{ - return -ENXIO; -} - -static inline int rsa_verify(struct image_sign_info *info, - const struct image_region region[], int region_count, - uint8_t *sig, uint sig_len) -{ - return -ENXIO; -} - -static inline int padding_pkcs_15_verify(struct image_sign_info *info, - uint8_t *msg, int msg_len, - const uint8_t *hash, int hash_len) -{ - return -ENXIO; -} - -#ifdef CONFIG_FIT_RSASSA_PSS -static inline int padding_pss_verify(struct image_sign_info *info, - uint8_t *msg, int msg_len, - const uint8_t *hash, int hash_len) -{ - return -ENXIO; -} -#endif /* CONFIG_FIT_RSASSA_PSS */ -#endif
#define RSA_DEFAULT_PADDING_NAME "pkcs-1.5"
This macro is no longer needed for code flow or #ifdefs. Remove it.
Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com Reviewed-by: Simon Glass sjg@chromium.org --- include/image.h | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/include/image.h b/include/image.h index f7f8f8a029..ee930b0265 100644 --- a/include/image.h +++ b/include/image.h @@ -1196,19 +1196,16 @@ int calculate_hash(const void *data, int data_len, const char *algo, #if defined(USE_HOSTCC) # if defined(CONFIG_FIT_SIGNATURE) # define IMAGE_ENABLE_SIGN 1 -# define IMAGE_ENABLE_VERIFY 1 # define IMAGE_ENABLE_VERIFY_ECDSA 1 # define FIT_IMAGE_ENABLE_VERIFY 1 # include <openssl/evp.h> # else # define IMAGE_ENABLE_SIGN 0 -# define IMAGE_ENABLE_VERIFY 0 # define IMAGE_ENABLE_VERIFY_ECDSA 0 # define FIT_IMAGE_ENABLE_VERIFY 0 # endif #else # define IMAGE_ENABLE_SIGN 0 -# define IMAGE_ENABLE_VERIFY CONFIG_IS_ENABLED(RSA_VERIFY) # define IMAGE_ENABLE_VERIFY_ECDSA 0 # define FIT_IMAGE_ENABLE_VERIFY CONFIG_IS_ENABLED(FIT_SIGNATURE) #endif @@ -1260,7 +1257,7 @@ struct image_region { int size; };
-#if IMAGE_ENABLE_VERIFY +#if FIT_IMAGE_ENABLE_VERIFY # include <u-boot/hash-checksum.h> #endif struct checksum_algo {
This macro is no longer needed for code flow or #ifdefs. Remove it.
Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com Reviewed-by: Simon Glass sjg@chromium.org --- include/image.h | 3 --- 1 file changed, 3 deletions(-)
diff --git a/include/image.h b/include/image.h index ee930b0265..2f48a6eecf 100644 --- a/include/image.h +++ b/include/image.h @@ -1196,17 +1196,14 @@ int calculate_hash(const void *data, int data_len, const char *algo, #if defined(USE_HOSTCC) # if defined(CONFIG_FIT_SIGNATURE) # define IMAGE_ENABLE_SIGN 1 -# define IMAGE_ENABLE_VERIFY_ECDSA 1 # define FIT_IMAGE_ENABLE_VERIFY 1 # include <openssl/evp.h> # else # define IMAGE_ENABLE_SIGN 0 -# define IMAGE_ENABLE_VERIFY_ECDSA 0 # define FIT_IMAGE_ENABLE_VERIFY 0 # endif #else # define IMAGE_ENABLE_SIGN 0 -# define IMAGE_ENABLE_VERIFY_ECDSA 0 # define FIT_IMAGE_ENABLE_VERIFY CONFIG_IS_ENABLED(FIT_SIGNATURE) #endif
Function pointers from crypto_algos array are relocated, when NEEDS_MANUAL_RELOC is set. This relocation doesn't happen if the algo is placed in a linker list. Implement this relocation.
Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com Reviewed-by: Simon Glass sjg@chromium.org --- common/image-sig.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/common/image-sig.c b/common/image-sig.c index 498cd78af4..5c7ddd984d 100644 --- a/common/image-sig.c +++ b/common/image-sig.c @@ -98,6 +98,19 @@ struct crypto_algo *image_get_crypto_algo(const char *full_name) int i; const char *name;
+#if defined(CONFIG_NEEDS_MANUAL_RELOC) + static bool done; + + if (!done) { + crypto = ll_entry_start(struct crypto_algo, cryptos); + end = ll_entry_end(struct crypto_algo, cryptos); + for (; crypto < end; crypto++) { + crypto->name += gd->reloc_off; + crypto->verify += gd->reloc_off; + } + } +#endif + /* Move name to after the comma */ name = strchr(full_name, ','); if (!name)
participants (4)
-
Alex G -
Alex G.
-
Alexandru Gagniuc
-
Simon Glass