| Message ID | 20241008075724.2772149-1-clg@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | hw/misc/aspeed_hace: Fix SG Accumulative Hash Calculations | expand |
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
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.