[PATCH RFC 0/2] lib: Remove the need for a HASH_CALCULATE config

[PATCH RFC 0/2] lib: Remove the need for a HASH_CALCULATE config

[Alexandru Gagniuc]

I had accidentally noticed commit 87316da05f2f ("lib: introduce
HASH_CALCULATE option"), when rebasing an unrelated series. It
immediately caught my attention because It seemed to me to increase
complexity, without any actual benefit.

In this series, I present an alternative approach, which solves the
problem by leveraging existing infrastructure instead of adding more
Kconfig variables.

Alexandru Gagniuc (2):
  Revert "lib: introduce HASH_CALCULATE option"
  efi_loader: Work-around build issue due to missing hash_calculate()

 common/Kconfig.boot    | 1 -
 lib/Kconfig            | 3 ---
 lib/Makefile           | 2 +-
 lib/efi_loader/Kconfig | 4 ++--
 4 files changed, 3 insertions(+), 7 deletions(-)

-- 
2.31.1

[PATCH RFC 1/2] Revert "lib: introduce HASH_CALCULATE option"

[Alexandru Gagniuc]

When we think of Kconfig, we usually think of features that we like
to enable or not. Ideally, we wouldn't use Kconfig to fix a build
issue, although sometimes it might make sense. With Kconfig it's hard
to guarantee that the fix is universal. We can only say that it works
for the set of tested configurations. In the majority of cases, it's
preferable to let the linker figure things out for us.

The reverted commit attempted to fix a build issue by adding an
invisible Kconfig option. This is wrong in several ways:

It invents a new Kconfig variable when CONFIG_HASH already
exists for the same purpose.
Second, hash-checksum.c makes use of the hash_progressive_lookup_algo()
symbol, which is only provided with CONFIG_HASH, but this dependency
was not expressed in the reverted patch.

It feels like Kconfig is turning into a listing of all available
source files, and a buffet to 'select' which ones to compile. The
purpose of this revert is to enable the next change to make use of
CONFIG_HASH instead of adding to Kconfig.

This reverts commit 87316da05f2fd49d3709275e64ef0c5980366ade.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 common/Kconfig.boot    | 1 -
 lib/Kconfig            | 3 ---
 lib/Makefile           | 2 +-
 lib/efi_loader/Kconfig | 2 --
 4 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/common/Kconfig.boot b/common/Kconfig.boot
index 3c6e77d099..89a3161f1f 100644
--- a/common/Kconfig.boot
+++ b/common/Kconfig.boot
@@ -80,7 +80,6 @@ config FIT_SIGNATURE
 	select RSA_VERIFY
 	select IMAGE_SIGN_INFO
 	select FIT_FULL_CHECK
-	select HASH_CALCULATE
 	help
 	  This option enables signature verification of FIT uImages,
 	  using a hash signed and verified using RSA. If
diff --git a/lib/Kconfig b/lib/Kconfig
index d675ab1d82..15019d2c65 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -439,9 +439,6 @@ config CRC32C
 config XXHASH
 	bool
 
-config HASH_CALCULATE
-	bool
-
 endmenu
 
 menu "Compression Support"
diff --git a/lib/Makefile b/lib/Makefile
index 0835ea292c..6825671955 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -61,7 +61,7 @@ endif
 obj-$(CONFIG_$(SPL_)ACPIGEN) += acpi/
 obj-$(CONFIG_$(SPL_)MD5) += md5.o
 obj-$(CONFIG_$(SPL_)RSA) += rsa/
-obj-$(CONFIG_HASH_CALCULATE) += hash-checksum.o
+obj-$(CONFIG_FIT_SIGNATURE) += hash-checksum.o
 obj-$(CONFIG_SHA1) += sha1.o
 obj-$(CONFIG_SHA256) += sha256.o
 obj-$(CONFIG_SHA512_ALGO) += sha512.o
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index eb5c4d6f29..c259abe033 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -174,7 +174,6 @@ config EFI_CAPSULE_AUTHENTICATE
 	select PKCS7_MESSAGE_PARSER
 	select PKCS7_VERIFY
 	select IMAGE_SIGN_INFO
-	select HASH_CALCULATE
 	default n
 	help
 	  Select this option if you want to enable capsule
@@ -343,7 +342,6 @@ config EFI_SECURE_BOOT
 	select X509_CERTIFICATE_PARSER
 	select PKCS7_MESSAGE_PARSER
 	select PKCS7_VERIFY
-	select HASH_CALCULATE
 	default n
 	help
 	  Select this option to enable EFI secure boot support.
-- 
2.31.1

[PATCH RFC 2/2] efi_loader: Work-around build issue due to missing hash_calculate()

[Alexandru Gagniuc]

The hash_calculate() symbol is provided by hash-checksum.c. It depends
on hash_progressive_lookup_algo(), provided when CONFIG_HASH=y.

The issue is that hash_calculate() is used by the efi_loader,
irregardless of CONFIG_FIT_SIGNATURE. As pointed out in
commit 87316da05f2f ("lib: introduce HASH_CALCULATE option"),
enabling hash_calculate() based on CONFIG_FIT_SIGNATURE is incorrect.

To resolve this, use CONFIG_HASH as the compile switch for
hash-checksum.c. This ensures that all dependencies are compiled, and
is the most natural Kconfig to use.

There is the issue of having to 'select HASH' in a couple of places
that already 'select SHA256'. This is a deeper problem with how hashes
are organized, and fixing it is beyonf the scope of this change.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 lib/Makefile           | 2 +-
 lib/efi_loader/Kconfig | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/Makefile b/lib/Makefile
index 6825671955..b4795a62a0 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -61,7 +61,7 @@ endif
 obj-$(CONFIG_$(SPL_)ACPIGEN) += acpi/
 obj-$(CONFIG_$(SPL_)MD5) += md5.o
 obj-$(CONFIG_$(SPL_)RSA) += rsa/
-obj-$(CONFIG_FIT_SIGNATURE) += hash-checksum.o
+obj-$(CONFIG_HASH) += hash-checksum.o
 obj-$(CONFIG_SHA1) += sha1.o
 obj-$(CONFIG_SHA256) += sha256.o
 obj-$(CONFIG_SHA512_ALGO) += sha512.o
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index c259abe033..b112e62334 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -166,6 +166,7 @@ config EFI_CAPSULE_AUTHENTICATE
 	depends on EFI_CAPSULE_FIRMWARE
 	depends on EFI_CAPSULE_ON_DISK
 	depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
+	select HASH
 	select SHA256
 	select RSA
 	select RSA_VERIFY
@@ -333,6 +334,7 @@ config EFI_LOAD_FILE2_INITRD
 config EFI_SECURE_BOOT
 	bool "Enable EFI secure boot support"
 	depends on EFI_LOADER
+	select HASH
 	select SHA256
 	select RSA
 	select RSA_VERIFY_WITH_PKEY
-- 
2.31.1

Re: [PATCH RFC 1/2] Revert "lib: introduce HASH_CALCULATE option"

[Heinrich Schuchardt]

On 5/24/21 9:28 PM, Alexandru Gagniuc wrote:
> When we think of Kconfig, we usually think of features that we like
> to enable or not. Ideally, we wouldn't use Kconfig to fix a build
> issue, although sometimes it might make sense. With Kconfig it's hard
> to guarantee that the fix is universal. We can only say that it works
> for the set of tested configurations. In the majority of cases, it's
> preferable to let the linker figure things out for us.
>
> The reverted commit attempted to fix a build issue by adding an
> invisible Kconfig option. This is wrong in several ways:
>
> It invents a new Kconfig variable when CONFIG_HASH already
> exists for the same purpose.
> Second, hash-checksum.c makes use of the hash_progressive_lookup_algo()
> symbol, which is only provided with CONFIG_HASH, but this dependency
> was not expressed in the reverted patch.
>
> It feels like Kconfig is turning into a listing of all available
> source files, and a buffet to 'select' which ones to compile. The
> purpose of this revert is to enable the next change to make use of
> CONFIG_HASH instead of adding to Kconfig.

See upcoming patch
efi_loader: add PE/COFF image measurement
https://patchwork.ozlabs.org/project/uboot/patch/20210526030958.15701-2-masahisa.kojima@linaro.org/

Here we need to compile hash-checksum.o, but don't need FIT image support.

Best regards

Heinrich

>
> This reverts commit 87316da05f2fd49d3709275e64ef0c5980366ade.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  common/Kconfig.boot    | 1 -
>  lib/Kconfig            | 3 ---
>  lib/Makefile           | 2 +-
>  lib/efi_loader/Kconfig | 2 --
>  4 files changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/common/Kconfig.boot b/common/Kconfig.boot
> index 3c6e77d099..89a3161f1f 100644
> --- a/common/Kconfig.boot
> +++ b/common/Kconfig.boot
> @@ -80,7 +80,6 @@ config FIT_SIGNATURE
>  	select RSA_VERIFY
>  	select IMAGE_SIGN_INFO
>  	select FIT_FULL_CHECK
> -	select HASH_CALCULATE
>  	help
>  	  This option enables signature verification of FIT uImages,
>  	  using a hash signed and verified using RSA. If
> diff --git a/lib/Kconfig b/lib/Kconfig
> index d675ab1d82..15019d2c65 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -439,9 +439,6 @@ config CRC32C
>  config XXHASH
>  	bool
>
> -config HASH_CALCULATE
> -	bool
> -
>  endmenu
>
>  menu "Compression Support"
> diff --git a/lib/Makefile b/lib/Makefile
> index 0835ea292c..6825671955 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -61,7 +61,7 @@ endif
>  obj-$(CONFIG_$(SPL_)ACPIGEN) += acpi/
>  obj-$(CONFIG_$(SPL_)MD5) += md5.o
>  obj-$(CONFIG_$(SPL_)RSA) += rsa/
> -obj-$(CONFIG_HASH_CALCULATE) += hash-checksum.o
> +obj-$(CONFIG_FIT_SIGNATURE) += hash-checksum.o
>  obj-$(CONFIG_SHA1) += sha1.o
>  obj-$(CONFIG_SHA256) += sha256.o
>  obj-$(CONFIG_SHA512_ALGO) += sha512.o
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index eb5c4d6f29..c259abe033 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -174,7 +174,6 @@ config EFI_CAPSULE_AUTHENTICATE
>  	select PKCS7_MESSAGE_PARSER
>  	select PKCS7_VERIFY
>  	select IMAGE_SIGN_INFO
> -	select HASH_CALCULATE
>  	default n
>  	help
>  	  Select this option if you want to enable capsule
> @@ -343,7 +342,6 @@ config EFI_SECURE_BOOT
>  	select X509_CERTIFICATE_PARSER
>  	select PKCS7_MESSAGE_PARSER
>  	select PKCS7_VERIFY
> -	select HASH_CALCULATE
>  	default n
>  	help
>  	  Select this option to enable EFI secure boot support.
>

Re: [PATCH RFC 1/2] Revert "lib: introduce HASH_CALCULATE option"

[Alex G.]

On 5/26/21 11:06 AM, Heinrich Schuchardt wrote:
> On 5/24/21 9:28 PM, Alexandru Gagniuc wrote:
>> When we think of Kconfig, we usually think of features that we like
>> to enable or not. Ideally, we wouldn't use Kconfig to fix a build
>> issue, although sometimes it might make sense. With Kconfig it's hard
>> to guarantee that the fix is universal. We can only say that it works
>> for the set of tested configurations. In the majority of cases, it's
>> preferable to let the linker figure things out for us.
>>
>> The reverted commit attempted to fix a build issue by adding an
>> invisible Kconfig option. This is wrong in several ways:
>>
>> It invents a new Kconfig variable when CONFIG_HASH already
>> exists for the same purpose.
>> Second, hash-checksum.c makes use of the hash_progressive_lookup_algo()
>> symbol, which is only provided with CONFIG_HASH, but this dependency
>> was not expressed in the reverted patch.
>>
>> It feels like Kconfig is turning into a listing of all available
>> source files, and a buffet to 'select' which ones to compile. The
>> purpose of this revert is to enable the next change to make use of
>> CONFIG_HASH instead of adding to Kconfig.
> 
> See upcoming patch
> efi_loader: add PE/COFF image measurement
> https://patchwork.ozlabs.org/project/uboot/patch/20210526030958.15701-2-masahisa.kojima@linaro.org/
> 
> Here we need to compile hash-checksum.o, but don't need FIT image support.

You can take the nest patch in this series and "select HASH".

Alex

> Best regards
> 
> Heinrich
> 
>>
>> This reverts commit 87316da05f2fd49d3709275e64ef0c5980366ade.
>>
>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>> ---
>>   common/Kconfig.boot    | 1 -
>>   lib/Kconfig            | 3 ---
>>   lib/Makefile           | 2 +-
>>   lib/efi_loader/Kconfig | 2 --
>>   4 files changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/common/Kconfig.boot b/common/Kconfig.boot
>> index 3c6e77d099..89a3161f1f 100644
>> --- a/common/Kconfig.boot
>> +++ b/common/Kconfig.boot
>> @@ -80,7 +80,6 @@ config FIT_SIGNATURE
>>   	select RSA_VERIFY
>>   	select IMAGE_SIGN_INFO
>>   	select FIT_FULL_CHECK
>> -	select HASH_CALCULATE
>>   	help
>>   	  This option enables signature verification of FIT uImages,
>>   	  using a hash signed and verified using RSA. If
>> diff --git a/lib/Kconfig b/lib/Kconfig
>> index d675ab1d82..15019d2c65 100644
>> --- a/lib/Kconfig
>> +++ b/lib/Kconfig
>> @@ -439,9 +439,6 @@ config CRC32C
>>   config XXHASH
>>   	bool
>>
>> -config HASH_CALCULATE
>> -	bool
>> -
>>   endmenu
>>
>>   menu "Compression Support"
>> diff --git a/lib/Makefile b/lib/Makefile
>> index 0835ea292c..6825671955 100644
>> --- a/lib/Makefile
>> +++ b/lib/Makefile
>> @@ -61,7 +61,7 @@ endif
>>   obj-$(CONFIG_$(SPL_)ACPIGEN) += acpi/
>>   obj-$(CONFIG_$(SPL_)MD5) += md5.o
>>   obj-$(CONFIG_$(SPL_)RSA) += rsa/
>> -obj-$(CONFIG_HASH_CALCULATE) += hash-checksum.o
>> +obj-$(CONFIG_FIT_SIGNATURE) += hash-checksum.o
>>   obj-$(CONFIG_SHA1) += sha1.o
>>   obj-$(CONFIG_SHA256) += sha256.o
>>   obj-$(CONFIG_SHA512_ALGO) += sha512.o
>> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
>> index eb5c4d6f29..c259abe033 100644
>> --- a/lib/efi_loader/Kconfig
>> +++ b/lib/efi_loader/Kconfig
>> @@ -174,7 +174,6 @@ config EFI_CAPSULE_AUTHENTICATE
>>   	select PKCS7_MESSAGE_PARSER
>>   	select PKCS7_VERIFY
>>   	select IMAGE_SIGN_INFO
>> -	select HASH_CALCULATE
>>   	default n
>>   	help
>>   	  Select this option if you want to enable capsule
>> @@ -343,7 +342,6 @@ config EFI_SECURE_BOOT
>>   	select X509_CERTIFICATE_PARSER
>>   	select PKCS7_MESSAGE_PARSER
>>   	select PKCS7_VERIFY
>> -	select HASH_CALCULATE
>>   	default n
>>   	help
>>   	  Select this option to enable EFI secure boot support.
>>
> 

Re: [PATCH RFC 0/2] lib: Remove the need for a HASH_CALCULATE config

[Masahisa Kojima]

Hi Alexandru,

I agree with this series.
Use CONFIG_HASH is more general than adding new CONFIG_HASH_CALCULATE.

Thanks,
Masahisa Kojima

On Tue, 25 May 2021 at 04:28, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> I had accidentally noticed commit 87316da05f2f ("lib: introduce
> HASH_CALCULATE option"), when rebasing an unrelated series. It
> immediately caught my attention because It seemed to me to increase
> complexity, without any actual benefit.
>
> In this series, I present an alternative approach, which solves the
> problem by leveraging existing infrastructure instead of adding more
> Kconfig variables.
>
> Alexandru Gagniuc (2):
>   Revert "lib: introduce HASH_CALCULATE option"
>   efi_loader: Work-around build issue due to missing hash_calculate()
>
>  common/Kconfig.boot    | 1 -
>  lib/Kconfig            | 3 ---
>  lib/Makefile           | 2 +-
>  lib/efi_loader/Kconfig | 4 ++--
>  4 files changed, 3 insertions(+), 7 deletions(-)
>
> --
> 2.31.1
>