Re: [PATCH v5 10/29] image: Use Kconfig to enable FIT_RSASSA_PSS on host

From: "Alex G." <mr.nuke.me@gmail.com>
To: Simon Glass <sjg@chromium.org>,
	U-Boot Mailing List <u-boot@lists.denx.de>
Cc: Andre Przywara <andre.przywara@arm.com>,
	Rasmus Villemoes <rasmus.villemoes@prevas.dk>,
	Robert Marko <robert.marko@sartura.hr>,
	Masahiro Yamada <masahiroy@kernel.org>,
	Tom Rini <trini@konsulko.com>,
	Joe Hershberger <joe.hershberger@ni.com>,
	Marek Vasut <marex@denx.de>
Subject: Re: [PATCH v5 10/29] image: Use Kconfig to enable FIT_RSASSA_PSS on host
Date: Tue, 5 Oct 2021 13:32:15 -0500	[thread overview]
Message-ID: <1bf0c28c-12db-df80-51dc-5855e1974aa5@gmail.com> (raw)
In-Reply-To: <20210925194327.v5.10.I0481c8d9b6f4bd7e467d0324c81295dd0a9bbc96@changeid>

On 9/25/21 8:43 PM, Simon Glass wrote:
> Add a host Kconfig for FIT_RSASSA_PSS. With this we can use
> CONFIG_IS_ENABLED(FIT_RSASSA_PSS) directly in the host build, so drop the
> forcing of this in the image.h header.
> 
> Drop the #ifdef around padding_pss_verify() too since it is not needed.
> Use the compiler to check the config where possible, instead of the
> preprocessor.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>

> ---
> 
> Changes in v5:
> - Avoid preprocessor in a few more places
> - Use TOOLS_ instead of HOST_
> 
>   include/image.h      |  3 ---
>   include/u-boot/rsa.h |  2 --
>   lib/rsa/rsa-sign.c   |  5 ++---
>   lib/rsa/rsa-verify.c | 16 +++-------------
>   tools/Kconfig        |  5 +++++
>   5 files changed, 10 insertions(+), 21 deletions(-)

Now that's what I'm talking about! deletions > insertions
> 
> diff --git a/include/image.h b/include/image.h
> index 6efbef06e64..dc872ef5b24 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -27,9 +27,6 @@ struct fdt_region;
>   #include <sys/types.h>
>   #include <linux/kconfig.h>
>   
> -/* new uImage format support enabled on host */
> -#define CONFIG_FIT_RSASSA_PSS 1
> -
>   #define IMAGE_ENABLE_IGNORE	0
>   #define IMAGE_INDENT_STRING	""
>   
> diff --git a/include/u-boot/rsa.h b/include/u-boot/rsa.h
> index 89a9c4caa0a..7556aa5b4b7 100644
> --- a/include/u-boot/rsa.h
> +++ b/include/u-boot/rsa.h
> @@ -103,11 +103,9 @@ 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_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_RSASSA_PSS */
>   
>   #define RSA_DEFAULT_PADDING_NAME		"pkcs-1.5"
>   
> diff --git a/lib/rsa/rsa-sign.c b/lib/rsa/rsa-sign.c
> index c27a784c429..0579e5294ee 100644
> --- a/lib/rsa/rsa-sign.c
> +++ b/lib/rsa/rsa-sign.c
> @@ -401,15 +401,14 @@ static int rsa_sign_with_key(EVP_PKEY *pkey, struct padding_algo *padding_algo,
>   		goto err_sign;
>   	}
>   
> -#ifdef CONFIG_FIT_RSASSA_PSS
> -	if (padding_algo && !strcmp(padding_algo->name, "pss")) {
> +	if (CONFIG_IS_ENABLED(FIT_RSASSA_PSS) && padding_algo &&
> +	    !strcmp(padding_algo->name, "pss")) {
>   		if (EVP_PKEY_CTX_set_rsa_padding(ckey,
>   						 RSA_PKCS1_PSS_PADDING) <= 0) {
>   			ret = rsa_err("Signer padding setup failed");
>   			goto err_sign;
>   		}
>   	}
> -#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 ad6d33d043a..9e522d210d7 100644
> --- a/lib/rsa/rsa-verify.c
> +++ b/lib/rsa/rsa-verify.c
> @@ -102,7 +102,6 @@ U_BOOT_PADDING_ALGO(pkcs_15) = {
>   };
>   #endif
>   
> -#ifdef CONFIG_FIT_RSASSA_PSS
>   static void u32_i2osp(uint32_t val, uint8_t *buf)
>   {
>   	buf[0] = (uint8_t)((val >> 24) & 0xff);
> @@ -311,9 +310,6 @@ U_BOOT_PADDING_ALGO(pss) = {
>   };
>   #endif
>   
> -#endif
> -
> -#if CONFIG_IS_ENABLED(FIT_SIGNATURE) || CONFIG_IS_ENABLED(RSA_VERIFY_WITH_PKEY)
>   /**
>    * rsa_verify_key() - Verify a signature against some data using RSA Key
>    *
> @@ -385,9 +381,7 @@ static int rsa_verify_key(struct image_sign_info *info,
>   
>   	return 0;
>   }
> -#endif
>   
> -#if CONFIG_IS_ENABLED(RSA_VERIFY_WITH_PKEY)
>   /**
>    * rsa_verify_with_pkey() - Verify a signature against some data using
>    * only modulus and exponent as RSA key properties.
> @@ -408,6 +402,9 @@ int rsa_verify_with_pkey(struct image_sign_info *info,
>   	struct key_prop *prop;
>   	int ret;
>   
> +	if (!CONFIG_IS_ENABLED(RSA_VERIFY_WITH_PKEY))
> +		return -EACCES;
> +
>   	/* Public key is self-described to fill key_prop */
>   	ret = rsa_gen_key_prop(info->key, info->keylen, &prop);
>   	if (ret) {
> @@ -422,13 +419,6 @@ int rsa_verify_with_pkey(struct image_sign_info *info,
>   
>   	return ret;
>   }
> -#else
> -int rsa_verify_with_pkey(struct image_sign_info *info,
> -			 const void *hash, uint8_t *sig, uint sig_len)
> -{
> -	return -EACCES;
> -}
> -#endif
>   
>   #if CONFIG_IS_ENABLED(FIT_SIGNATURE)
>   /**
> diff --git a/tools/Kconfig b/tools/Kconfig
> index 9d1c0efd40c..8685c800f93 100644
> --- a/tools/Kconfig
> +++ b/tools/Kconfig
> @@ -35,6 +35,11 @@ config TOOLS_FIT_PRINT
>   	help
>   	  Print the content of the FIT verbosely in the tools builds
>   
> +config TOOLS_FIT_RSASSA_PSS
> +	def_bool y
> +	help
> +	  Support the rsassa-pss signature scheme in the tools builds

If we're going to have these TOOLS_ configs always on, what's the point 
in adding a help text?

> +
>   config TOOLS_FIT_SIGNATURE
>   	def_bool y
>   	help
> 