Hello,

This is a resping of Alejandro's series fixing SG Accumulative Hash
Calculations. See [1] for more details. 

The goal of this patch series is to fix accumulative hashing support
in the Aspeed HACE module. The issue that stemmed this patch was a
failure to boot an OpenBMC image using the "ast2600-evb" machine. The
U-boot 2019.04 loader failed to verify image hashes.

These incorrect image hashes given by the HACE to the U-boot guest are
due to an oversight in the HACE module. Previously when operating in
scatter-gather accumulative mode, the HACE would cache the address
provided by the guest which contained the source data. However, there
was no deep copy, so when HACE generated the digest upon the reception
of the final accumulative chunk the digest was incorrect, as the
addresses provided had their regions overwritten by that time.

This fix consists of two main steps:
* Add an accumulative hashing function to the qcrypto library
* Modify the HACE module to use the accumulative hashing functions

All the crypto library backends (nettle, gnutls, etc.) support
accumulative hashing, so it was trivial to create wrappers for those
functions.

Changes in v5 (clg):

 - Changed documentation "non-zero on error" -> "-1 on error" 
 - Dropped qcrypto_hash_supports() in qcrypto_glib_hash_new()
 - Removed superfluous cast (GChecksum *) in qcrypto_glib_hash_free()
 - Reworked qcrypto_glib_hash_finalize() 
 - Dropped qcrypto_hash_supports() in qcrypto_gcrypt_hash_new()
 - Reworked qcrypto_gcrypt_hash_finalize()
 - Handled gcry_md_open() errors in qcrypto_gcrypt_hash_new()
 - Dropped qcrypto_hash_supports() in qcrypto_gnutls_hash_new()
 - Reworked qcrypto_gnutls_hash_finalize()
 - Handled gnutls_hash_init() errors in qcrypto_gnutls_hash_new()
 - Replaced gnutls_hash_deinit() by gnutls_hash_output() in
   qcrypto_gnutls_hash_finalize()
 - Freed resources with gnutls_hash_deinit() in qcrypto_gnutls_hash_free() 
 - Dropped qcrypto_hash_supports() in qcrypto_nettle_hash_new() 
 - Split iov changes from original patch
 - Handled qcrypto_afalg_hash_ctx_new() errors in qcrypto_afalg_hash_new()
 - Freed alg_name in qcrypto_afalg_hash_new()
 - Reworked qcrypto_afalg_recv_from_kernel()
 - Split iov changes from original patch 
 - Reworked qcrypto_hash_bytesv() error handling
 - Used hash->driver int qcrypto_hash_new(), qcrypto_hash_free() qcrypto_hash_updatev()
 - Introduced qcrypto_hash_supports() check in  qcrypto_hash_new()
 - Introduced g_autofree variables in qcrypto_hash_finalize_digest()
   and qcrypto_hash_finalize_base64()
 - Re-arrranged code in qcrypto_hash_digestv() and qcrypto_hash_digest()
 - Improved test_hash_accumulate() with g_autofree variables 
 - Fixed spelling in commit log 
 - Checkpatch fixes 

Changes in V4:
* Restructured patches so unit tests pass for each independently.
* Freeing hash context is now a void function.
* Added autoptr cleanup function definition for qcrypto_hash_free.
* Separated qcrypto_hash_update into qcrypto_hash_update and
  qcrypto_hash_updatev.
* Changed public hash functions to use afalg implementation correctly if support
  is enabled.
* Fixed accumulative hashing in afalg driver (pass MSG_MORE socket flag).

Changes in V3:
* Reworked crypto hash API with comments from Daniel
  * Creation/Deletion of contexts, updating, and finalizing
  * Modified existing API functions to use the new 4 main core functions
  * Added test for accumulative hashing
  * Added afalg driver implementation
* Fixed bug in HACE module where hash context fails to allocate,
  causing the HACE internal state to be incorrect and segfault.

Changes in V2:
* Fixed error checking bug in libgcrypt crypto backend of
  accumulate_bytesv

Alejandro Zeise (16):
  crypto: accumulative hashing API
  crypto/hash-glib: Implement new hash API
  crypto/hash-gcrypt: Implement new hash API
  crypto/hash-gnutls: Implement new hash API
  crypto/hash-nettle: Implement new hash API
  util/iov: Introduce iov_send_recv_with_flags()
  crypto/hash-afalg: Implement new hash API
  crypto/hash: Implement and use new hash API
  tests/unit/test-crypto-hash: accumulative hashing
  crypto/hash-glib: Remove old hash API functions
  crypto/hash-gcrypt: Remove old hash API functions
  crypto/hash-gnutls: Remove old hash API functions
  crypto/hash-nettle: Remove old hash API functions
  crypto/hash-afalg: Remove old hash API functions
  crypto/hashpriv: Remove old hash API function
  hw/misc/aspeed_hace: Fix SG Accumulative hashing

 crypto/hashpriv.h             |  13 ++-
 include/crypto/hash.h         | 119 ++++++++++++++++++++++++
 include/hw/misc/aspeed_hace.h |   4 +
 include/qemu/iov.h            |  27 ++++++
 crypto/hash-afalg.c           | 167 ++++++++++++++++++++++++----------
 crypto/hash-gcrypt.c          | 112 +++++++++++++----------
 crypto/hash-glib.c            |  92 +++++++++++--------
 crypto/hash-gnutls.c          |  97 +++++++++++++-------
 crypto/hash-nettle.c          |  81 ++++++++++-------
 crypto/hash.c                 | 161 ++++++++++++++++++++++++++------
 hw/misc/aspeed_hace.c         |  96 ++++++++++---------
 tests/unit/test-crypto-hash.c |  46 ++++++++++
 util/iov.c                    |  25 +++--
 13 files changed, 756 insertions(+), 284 deletions(-)

-- 
2.46.2

On 10/8/24 09:57, Cédric Le Goater wrote:
> Hello,
> 
> This is a resping of Alejandro's series fixing SG Accumulative Hash
> Calculations. See [1] for more details.

arf.

[1] https://lore.kernel.org/all/20240807195122.2827364-1-alejandro.zeise@seagate.com/

C.

> 
> The goal of this patch series is to fix accumulative hashing support
> in the Aspeed HACE module. The issue that stemmed this patch was a
> failure to boot an OpenBMC image using the "ast2600-evb" machine. The
> U-boot 2019.04 loader failed to verify image hashes.
> 
> These incorrect image hashes given by the HACE to the U-boot guest are
> due to an oversight in the HACE module. Previously when operating in
> scatter-gather accumulative mode, the HACE would cache the address
> provided by the guest which contained the source data. However, there
> was no deep copy, so when HACE generated the digest upon the reception
> of the final accumulative chunk the digest was incorrect, as the
> addresses provided had their regions overwritten by that time.
> 
> This fix consists of two main steps:
> * Add an accumulative hashing function to the qcrypto library
> * Modify the HACE module to use the accumulative hashing functions
> 
> All the crypto library backends (nettle, gnutls, etc.) support
> accumulative hashing, so it was trivial to create wrappers for those
> functions.
> 
> Changes in v5 (clg):
> 
>   - Changed documentation "non-zero on error" -> "-1 on error"
>   - Dropped qcrypto_hash_supports() in qcrypto_glib_hash_new()
>   - Removed superfluous cast (GChecksum *) in qcrypto_glib_hash_free()
>   - Reworked qcrypto_glib_hash_finalize()
>   - Dropped qcrypto_hash_supports() in qcrypto_gcrypt_hash_new()
>   - Reworked qcrypto_gcrypt_hash_finalize()
>   - Handled gcry_md_open() errors in qcrypto_gcrypt_hash_new()
>   - Dropped qcrypto_hash_supports() in qcrypto_gnutls_hash_new()
>   - Reworked qcrypto_gnutls_hash_finalize()
>   - Handled gnutls_hash_init() errors in qcrypto_gnutls_hash_new()
>   - Replaced gnutls_hash_deinit() by gnutls_hash_output() in
>     qcrypto_gnutls_hash_finalize()
>   - Freed resources with gnutls_hash_deinit() in qcrypto_gnutls_hash_free()
>   - Dropped qcrypto_hash_supports() in qcrypto_nettle_hash_new()
>   - Split iov changes from original patch
>   - Handled qcrypto_afalg_hash_ctx_new() errors in qcrypto_afalg_hash_new()
>   - Freed alg_name in qcrypto_afalg_hash_new()
>   - Reworked qcrypto_afalg_recv_from_kernel()
>   - Split iov changes from original patch
>   - Reworked qcrypto_hash_bytesv() error handling
>   - Used hash->driver int qcrypto_hash_new(), qcrypto_hash_free() qcrypto_hash_updatev()
>   - Introduced qcrypto_hash_supports() check in  qcrypto_hash_new()
>   - Introduced g_autofree variables in qcrypto_hash_finalize_digest()
>     and qcrypto_hash_finalize_base64()
>   - Re-arrranged code in qcrypto_hash_digestv() and qcrypto_hash_digest()
>   - Improved test_hash_accumulate() with g_autofree variables
>   - Fixed spelling in commit log
>   - Checkpatch fixes
> 
> Changes in V4:
> * Restructured patches so unit tests pass for each independently.
> * Freeing hash context is now a void function.
> * Added autoptr cleanup function definition for qcrypto_hash_free.
> * Separated qcrypto_hash_update into qcrypto_hash_update and
>    qcrypto_hash_updatev.
> * Changed public hash functions to use afalg implementation correctly if support
>    is enabled.
> * Fixed accumulative hashing in afalg driver (pass MSG_MORE socket flag).
> 
> Changes in V3:
> * Reworked crypto hash API with comments from Daniel
>    * Creation/Deletion of contexts, updating, and finalizing
>    * Modified existing API functions to use the new 4 main core functions
>    * Added test for accumulative hashing
>    * Added afalg driver implementation
> * Fixed bug in HACE module where hash context fails to allocate,
>    causing the HACE internal state to be incorrect and segfault.
> 
> Changes in V2:
> * Fixed error checking bug in libgcrypt crypto backend of
>    accumulate_bytesv
> 
> Alejandro Zeise (16):
>    crypto: accumulative hashing API
>    crypto/hash-glib: Implement new hash API
>    crypto/hash-gcrypt: Implement new hash API
>    crypto/hash-gnutls: Implement new hash API
>    crypto/hash-nettle: Implement new hash API
>    util/iov: Introduce iov_send_recv_with_flags()
>    crypto/hash-afalg: Implement new hash API
>    crypto/hash: Implement and use new hash API
>    tests/unit/test-crypto-hash: accumulative hashing
>    crypto/hash-glib: Remove old hash API functions
>    crypto/hash-gcrypt: Remove old hash API functions
>    crypto/hash-gnutls: Remove old hash API functions
>    crypto/hash-nettle: Remove old hash API functions
>    crypto/hash-afalg: Remove old hash API functions
>    crypto/hashpriv: Remove old hash API function
>    hw/misc/aspeed_hace: Fix SG Accumulative hashing
> 
>   crypto/hashpriv.h             |  13 ++-
>   include/crypto/hash.h         | 119 ++++++++++++++++++++++++
>   include/hw/misc/aspeed_hace.h |   4 +
>   include/qemu/iov.h            |  27 ++++++
>   crypto/hash-afalg.c           | 167 ++++++++++++++++++++++++----------
>   crypto/hash-gcrypt.c          | 112 +++++++++++++----------
>   crypto/hash-glib.c            |  92 +++++++++++--------
>   crypto/hash-gnutls.c          |  97 +++++++++++++-------
>   crypto/hash-nettle.c          |  81 ++++++++++-------
>   crypto/hash.c                 | 161 ++++++++++++++++++++++++++------
>   hw/misc/aspeed_hace.c         |  96 ++++++++++---------
>   tests/unit/test-crypto-hash.c |  46 ++++++++++
>   util/iov.c                    |  25 +++--
>   13 files changed, 756 insertions(+), 284 deletions(-)
> 

On Tue, Oct 08, 2024 at 09:57:07AM +0200, Cédric Le Goater wrote:
> Hello,
> 
> This is a resping of Alejandro's series fixing SG Accumulative Hash
> Calculations. See [1] for more details. 
> 
> The goal of this patch series is to fix accumulative hashing support
> in the Aspeed HACE module. The issue that stemmed this patch was a
> failure to boot an OpenBMC image using the "ast2600-evb" machine. The
> U-boot 2019.04 loader failed to verify image hashes.
> 
> These incorrect image hashes given by the HACE to the U-boot guest are
> due to an oversight in the HACE module. Previously when operating in
> scatter-gather accumulative mode, the HACE would cache the address
> provided by the guest which contained the source data. However, there
> was no deep copy, so when HACE generated the digest upon the reception
> of the final accumulative chunk the digest was incorrect, as the
> addresses provided had their regions overwritten by that time.
> 
> This fix consists of two main steps:
> * Add an accumulative hashing function to the qcrypto library
> * Modify the HACE module to use the accumulative hashing functions
> 
> All the crypto library backends (nettle, gnutls, etc.) support
> accumulative hashing, so it was trivial to create wrappers for those
> functions.

I'll queue all except patch 16, and send a PULL request
for it, and let you handle the last patch.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On 10/10/24 13:28, Daniel P. Berrangé wrote:
> On Tue, Oct 08, 2024 at 09:57:07AM +0200, Cédric Le Goater wrote:
>> Hello,
>>
>> This is a resping of Alejandro's series fixing SG Accumulative Hash
>> Calculations. See [1] for more details.
>>
>> The goal of this patch series is to fix accumulative hashing support
>> in the Aspeed HACE module. The issue that stemmed this patch was a
>> failure to boot an OpenBMC image using the "ast2600-evb" machine. The
>> U-boot 2019.04 loader failed to verify image hashes.
>>
>> These incorrect image hashes given by the HACE to the U-boot guest are
>> due to an oversight in the HACE module. Previously when operating in
>> scatter-gather accumulative mode, the HACE would cache the address
>> provided by the guest which contained the source data. However, there
>> was no deep copy, so when HACE generated the digest upon the reception
>> of the final accumulative chunk the digest was incorrect, as the
>> addresses provided had their regions overwritten by that time.
>>
>> This fix consists of two main steps:
>> * Add an accumulative hashing function to the qcrypto library
>> * Modify the HACE module to use the accumulative hashing functions
>>
>> All the crypto library backends (nettle, gnutls, etc.) support
>> accumulative hashing, so it was trivial to create wrappers for those
>> functions.
> 
> I'll queue all except patch 16, and send a PULL request
> for it, and let you handle the last patch.

yes. Thanks for the comments,

C.