[PATCH v4 00/15] hw/misc/aspeed_hace: Fix SG Accumulative Hash Calculations

[PATCH v4 00/15] hw/misc/aspeed_hace: Fix SG Accumulative Hash Calculations

[Alejandro Zeise]

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 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 (15):
  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
  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/hash-afalg.c           | 171 ++++++++++++++++++++++++----------
 crypto/hash-gcrypt.c          | 110 ++++++++++++----------
 crypto/hash-glib.c            |  96 ++++++++++++-------
 crypto/hash-gnutls.c          |  92 +++++++++++-------
 crypto/hash-nettle.c          |  78 ++++++++++------
 crypto/hash.c                 | 163 +++++++++++++++++++++++++-------
 crypto/hashpriv.h             |  13 ++-
 hw/misc/aspeed_hace.c         |  94 ++++++++++---------
 include/crypto/hash.h         | 119 +++++++++++++++++++++++
 include/hw/misc/aspeed_hace.h |   4 +
 include/qemu/iov.h            |  26 ++++++
 tests/unit/test-crypto-hash.c |  48 ++++++++++
 util/iov.c                    |  22 +++--
 13 files changed, 753 insertions(+), 283 deletions(-)

-- 
2.34.1

[PATCH v4 01/15] crypto: accumulative hashing API

[Alejandro Zeise]

Changes the hash API to support accumulative hashing.
Hash objects are created with "qcrypto_hash_new",
updated with data with "qcrypto_hash_update", and
the hash obtained with "qcrypto_hash_finalize".

These changes bring the hashing API more in line with the
hmac API.

Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
---
 crypto/hashpriv.h     |  13 +++++
 include/crypto/hash.h | 119 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 132 insertions(+)

diff --git a/crypto/hashpriv.h b/crypto/hashpriv.h
index cee26ccb47..02f17ee99f 100644
--- a/crypto/hashpriv.h
+++ b/crypto/hashpriv.h
@@ -1,6 +1,7 @@
 /*
  * QEMU Crypto hash driver supports
  *
+ * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates
  * Copyright (c) 2017 HUAWEI TECHNOLOGIES CO., LTD.
  *
  * Authors:
@@ -15,6 +16,8 @@
 #ifndef QCRYPTO_HASHPRIV_H
 #define QCRYPTO_HASHPRIV_H
 
+#include "crypto/hash.h"
+
 typedef struct QCryptoHashDriver QCryptoHashDriver;
 
 struct QCryptoHashDriver {
@@ -24,6 +27,16 @@ struct QCryptoHashDriver {
                        uint8_t **result,
                        size_t *resultlen,
                        Error **errp);
+    QCryptoHash *(*hash_new)(QCryptoHashAlgorithm alg, Error **errp);
+    int (*hash_update)(QCryptoHash *hash,
+                       const struct iovec *iov,
+                       size_t niov,
+                       Error **errp);
+    int (*hash_finalize)(QCryptoHash *hash,
+                         uint8_t **result,
+                         size_t *resultlen,
+                         Error **errp);
+    void (*hash_free)(QCryptoHash *hash);
 };
 
 extern QCryptoHashDriver qcrypto_hash_lib_driver;
diff --git a/include/crypto/hash.h b/include/crypto/hash.h
index 54d87aa2a1..6d7222867e 100644
--- a/include/crypto/hash.h
+++ b/include/crypto/hash.h
@@ -1,6 +1,7 @@
 /*
  * QEMU Crypto hash algorithms
  *
+ * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates
  * Copyright (c) 2015 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
@@ -25,6 +26,13 @@
 
 /* See also "QCryptoHashAlgorithm" defined in qapi/crypto.json */
 
+typedef struct QCryptoHash QCryptoHash;
+struct QCryptoHash {
+    QCryptoHashAlgorithm alg;
+    void *opaque;
+    void *driver;
+};
+
 /**
  * qcrypto_hash_supports:
  * @alg: the hash algorithm
@@ -120,6 +128,117 @@ int qcrypto_hash_digestv(QCryptoHashAlgorithm alg,
                          char **digest,
                          Error **errp);
 
+/**
+ * qcrypto_hash_updatev:
+ * @hash: hash object from qcrypto_hash_new
+ * @iov: the array of memory regions to hash
+ * @niov: the length of @iov
+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Updates the given hash object with all the memory regions
+ * present in @iov.
+ *
+ * Returns: 0 on success, non-zero on error
+ */
+int qcrypto_hash_updatev(QCryptoHash *hash,
+                         const struct iovec *iov,
+                         size_t niov,
+                         Error **errp);
+/**
+ * qcrypto_hash_update:
+ * @hash: hash object from qcrypto_hash_new
+ * @buf: the memory region to hash
+ * @len: the length of @buf
+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Updates the given hash object with the data from
+ * the given buffer.
+ *
+ * Returns: 0 on success, non-zero on error
+ */
+int qcrypto_hash_update(QCryptoHash *hash,
+                        const char *buf,
+                        size_t len,
+                        Error **errp);
+
+/**
+ * qcrypto_hash_finalize_digest:
+ * @hash: the hash object to finalize
+ * @digest: pointer to hold output hash
+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Computes the hash from the given hash object. Hash object
+ * is expected to have its data updated from the qcrypto_hash_update function.
+ * The @digest pointer will be filled with the printable hex digest of the
+ * computed hash, which will be terminated by '\0'. The memory pointer
+ * in @digest must be released with a call to g_free() when
+ * no longer required.
+ *
+ * Returns: 0 on success, non-zero on error
+ */
+int qcrypto_hash_finalize_digest(QCryptoHash *hash,
+                                 char **digest,
+                                 Error **errp);
+
+/**
+ * qcrypto_hash_finalize_base64:
+ * @hash_ctx: hash object to finalize
+ * @base64: pointer to store the hash result in
+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Computes the hash from the given hash object. Hash object
+ * is expected to have it's data updated from the qcrypto_hash_update function.
+ * The @base64 pointer will be filled with the base64 encoding of the computed
+ * hash, which will be terminated by '\0'. The memory pointer in @base64
+ * must be released with a call to g_free() when no longer required.
+ *
+ * Returns: 0 on success, non-zero on error
+ */
+int qcrypto_hash_finalize_base64(QCryptoHash *hash,
+                                 char **base64,
+                                 Error **errp);
+
+/**
+ * qcrypto_hash_finalize_bytes:
+ * @hash_ctx: hash object to finalize
+ * @result: pointer to store the hash result in
+ * @result_len: Pointer to store the length of the result in
+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Computes the hash from the given hash object. Hash object
+ * is expected to have it's data updated from the qcrypto_hash_update function.
+ * The memory pointer in @result must be released with a call to g_free()
+ * when no longer required.
+ *
+ * Returns: 0 on success, non-zero on error
+ */
+int qcrypto_hash_finalize_bytes(QCryptoHash *hash,
+                                uint8_t **result,
+                                size_t *result_len,
+                                Error **errp);
+
+/**
+ * qcrypto_hash_new:
+ * @alg: the hash algorithm
+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Creates a new hashing context for the chosen algorithm for
+ * usage with qcrypto_hash_update.
+ *
+ * Returns: New hash object with the given algorithm, or NULL on error.
+ */
+QCryptoHash *qcrypto_hash_new(QCryptoHashAlgorithm alg, Error **errp);
+
+/**
+ * qcrypto_hash_free:
+ * @hash: hash object to free
+ *
+ * Frees a hashing context for the chosen algorithm.
+ */
+void qcrypto_hash_free(QCryptoHash *hash);
+
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(QCryptoHash, qcrypto_hash_free)
+
 /**
  * qcrypto_hash_digest:
  * @alg: the hash algorithm
-- 
2.34.1

[PATCH v4 02/15] crypto/hash-glib: Implement new hash API

[Alejandro Zeise]

Implements the new hashing API in the GLib hash driver.
Supports creating/destroying a context, updating the context
with input data and obtaining an output hash.

Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
---
 crypto/hash-glib.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/crypto/hash-glib.c b/crypto/hash-glib.c
index 82de9db705..9f4490762a 100644
--- a/crypto/hash-glib.c
+++ b/crypto/hash-glib.c
@@ -1,6 +1,7 @@
 /*
  * QEMU Crypto hash algorithms
  *
+ * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates
  * Copyright (c) 2016 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
@@ -95,6 +96,81 @@ qcrypto_glib_hash_bytesv(QCryptoHashAlgorithm alg,
 }
 
 
+static
+QCryptoHash *qcrypto_glib_hash_new(QCryptoHashAlgorithm alg,
+                                   Error **errp)
+{
+    QCryptoHash *hash = NULL;
+
+    if (!qcrypto_hash_supports(alg)) {
+        error_setg(errp,
+                   "Unknown hash algorithm %d",
+                   alg);
+    } else {
+        hash = g_new(QCryptoHash, 1);
+        hash->alg = alg;
+        hash->opaque = g_checksum_new(qcrypto_hash_alg_map[alg]);
+    }
+
+    return hash;
+}
+
+static
+void qcrypto_glib_hash_free(QCryptoHash *hash)
+{
+    if (hash->opaque) {
+        g_checksum_free((GChecksum *) hash->opaque);
+    }
+
+    g_free(hash);
+}
+
+
+static
+int qcrypto_glib_hash_update(QCryptoHash *hash,
+                             const struct iovec *iov,
+                             size_t niov,
+                             Error **errp)
+{
+    GChecksum *ctx = hash->opaque;
+
+    for (int i = 0; i < niov; i++) {
+        g_checksum_update(ctx, iov[i].iov_base, iov[i].iov_len);
+    }
+
+    return 0;
+}
+
+static
+int qcrypto_glib_hash_finalize(QCryptoHash *hash,
+                               uint8_t **result,
+                               size_t *result_len,
+                               Error **errp)
+{
+    int ret;
+    GChecksum *ctx = hash->opaque;
+
+    ret = g_checksum_type_get_length(qcrypto_hash_alg_map[hash->alg]);
+    if (ret < 0) {
+        error_setg(errp, "%s",
+                   "Unable to get hash length");
+        *result_len = 0;
+        ret = -1;
+    } else {
+        *result_len = ret;
+        *result = g_new(uint8_t, *result_len);
+
+        g_checksum_get_digest(ctx, *result, result_len);
+        ret = 0;
+    }
+
+    return ret;
+}
+
 QCryptoHashDriver qcrypto_hash_lib_driver = {
     .hash_bytesv = qcrypto_glib_hash_bytesv,
+    .hash_new      = qcrypto_glib_hash_new,
+    .hash_update   = qcrypto_glib_hash_update,
+    .hash_finalize = qcrypto_glib_hash_finalize,
+    .hash_free     = qcrypto_glib_hash_free,
 };
-- 
2.34.1

[PATCH v4 03/15] crypto/hash-gcrypt: Implement new hash API

[Alejandro Zeise]

Implements the new hashing API in the gcrypt hash driver.
Supports creating/destroying a context, updating the context
with input data and obtaining an output hash.

Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
---
 crypto/hash-gcrypt.c | 79 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/crypto/hash-gcrypt.c b/crypto/hash-gcrypt.c
index 829e48258d..e05511cafa 100644
--- a/crypto/hash-gcrypt.c
+++ b/crypto/hash-gcrypt.c
@@ -1,6 +1,7 @@
 /*
  * QEMU Crypto hash algorithms
  *
+ * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates
  * Copyright (c) 2016 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
@@ -110,7 +111,85 @@ qcrypto_gcrypt_hash_bytesv(QCryptoHashAlgorithm alg,
     return -1;
 }
 
+static
+QCryptoHash *qcrypto_gcrypt_hash_new(QCryptoHashAlgorithm alg, Error **errp)
+{
+    QCryptoHash *hash = NULL;
+
+    if (!qcrypto_hash_supports(alg)) {
+        error_setg(errp,
+                   "Unknown hash algorithm %d",
+                   alg);
+    } else {
+        hash = g_new(QCryptoHash, 1);
+        hash->alg = alg;
+        hash->opaque = g_new(gcry_md_hd_t, 1);
+
+        gcry_md_open((gcry_md_hd_t *) hash->opaque, qcrypto_hash_alg_map[alg], 0);
+    }
+
+    return hash;
+}
+
+static
+void qcrypto_gcrypt_hash_free(QCryptoHash *hash)
+{
+    gcry_md_hd_t *ctx = hash->opaque;
+
+    if (ctx) {
+        gcry_md_close(*ctx);
+        g_free(ctx);
+    }
+
+    g_free(hash);
+}
+
+
+static
+int qcrypto_gcrypt_hash_update(QCryptoHash *hash,
+                               const struct iovec *iov,
+                               size_t niov,
+                               Error **errp)
+{
+    gcry_md_hd_t *ctx = hash->opaque;
+
+    for (int i = 0; i < niov; i++) {
+        gcry_md_write(*ctx, iov[i].iov_base, iov[i].iov_len);
+    }
+
+    return 0;
+}
+
+static
+int qcrypto_gcrypt_hash_finalize(QCryptoHash *hash,
+                                 uint8_t **result,
+                                 size_t *result_len,
+                                 Error **errp)
+{
+    unsigned char *digest;
+    int ret = 0;
+    gcry_md_hd_t *ctx = hash->opaque;
+
+    *result_len = gcry_md_get_algo_dlen(qcrypto_hash_alg_map[hash->alg]);
+    if (*result_len == 0) {
+        error_setg(errp, "%s",
+                   "Unable to get hash length");
+        ret = -1;
+    } else {
+        *result = g_new(uint8_t, *result_len);
+
+        /* Digest is freed by gcry_md_close(), copy it */
+        digest = gcry_md_read(*ctx, 0);
+        memcpy(*result, digest, *result_len);
+    }
+
+    return ret;
+}
 
 QCryptoHashDriver qcrypto_hash_lib_driver = {
     .hash_bytesv = qcrypto_gcrypt_hash_bytesv,
+    .hash_new      = qcrypto_gcrypt_hash_new,
+    .hash_update   = qcrypto_gcrypt_hash_update,
+    .hash_finalize = qcrypto_gcrypt_hash_finalize,
+    .hash_free     = qcrypto_gcrypt_hash_free,
 };
-- 
2.34.1

[PATCH v4 04/15] crypto/hash-gnutls: Implement new hash API

[Alejandro Zeise]

Implements the new hashing API in the gnutls hash driver.
Supports creating/destroying a context, updating the context
with input data and obtaining an output hash.

Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
---
 crypto/hash-gnutls.c | 73 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/crypto/hash-gnutls.c b/crypto/hash-gnutls.c
index 17911ac5d1..15fc630a11 100644
--- a/crypto/hash-gnutls.c
+++ b/crypto/hash-gnutls.c
@@ -1,6 +1,7 @@
 /*
  * QEMU Crypto hash algorithms
  *
+ * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates
  * Copyright (c) 2021 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
@@ -98,7 +99,79 @@ qcrypto_gnutls_hash_bytesv(QCryptoHashAlgorithm alg,
     return 0;
 }
 
+static
+QCryptoHash *qcrypto_gnutls_hash_new(QCryptoHashAlgorithm alg, Error **errp)
+{
+    QCryptoHash *hash = NULL;
+
+    if (!qcrypto_hash_supports(alg)) {
+        error_setg(errp,
+                   "Unknown hash algorithm %d",
+                   alg);
+    } else {
+        hash = g_new(QCryptoHash, 1);
+        hash->alg = alg;
+        hash->opaque = g_new(gnutls_hash_hd_t, 1);
+
+        gnutls_hash_init(hash->opaque, qcrypto_hash_alg_map[alg]);
+    }
+
+    return hash;
+}
+
+static
+void qcrypto_gnutls_hash_free(QCryptoHash *hash)
+{
+    gnutls_hash_hd_t *ctx = hash->opaque;
+
+    g_free(ctx);
+    g_free(hash);
+}
+
+
+static
+int qcrypto_gnutls_hash_update(QCryptoHash *hash,
+                               const struct iovec *iov,
+                               size_t niov,
+                               Error **errp)
+{
+    int fail = 0;
+    gnutls_hash_hd_t *ctx = hash->opaque;
+
+    for (int i = 0; i < niov; i++) {
+        fail = gnutls_hash(*ctx, iov[i].iov_base, iov[i].iov_len) || fail;
+    }
+
+    return fail;
+}
+
+static
+int qcrypto_gnutls_hash_finalize(QCryptoHash *hash,
+                                 uint8_t **result,
+                                 size_t *result_len,
+                                 Error **errp)
+{
+    int ret = 0;
+    gnutls_hash_hd_t *ctx = hash->opaque;
+
+    *result_len = gnutls_hash_get_len(qcrypto_hash_alg_map[hash->alg]);
+    if (*result_len == 0) {
+        error_setg(errp, "%s",
+                   "Unable to get hash length");
+        ret = -1;
+    } else {
+        *result = g_new(uint8_t, *result_len);
+
+        gnutls_hash_deinit(*ctx, *result);
+    }
+
+    return ret;
+}
 
 QCryptoHashDriver qcrypto_hash_lib_driver = {
     .hash_bytesv = qcrypto_gnutls_hash_bytesv,
+    .hash_new      = qcrypto_gnutls_hash_new,
+    .hash_update   = qcrypto_gnutls_hash_update,
+    .hash_finalize = qcrypto_gnutls_hash_finalize,
+    .hash_free     = qcrypto_gnutls_hash_free,
 };
-- 
2.34.1

[PATCH v4 05/15] crypto/hash-nettle: Implement new hash API

[Alejandro Zeise]

Implements the new hashing API in the nettle hash driver.
Supports creating/destroying a context, updating the context
with input data and obtaining an output hash.

Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
---
 crypto/hash-nettle.c | 77 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/crypto/hash-nettle.c b/crypto/hash-nettle.c
index 1ca1a41062..894c7b4fc3 100644
--- a/crypto/hash-nettle.c
+++ b/crypto/hash-nettle.c
@@ -1,6 +1,7 @@
 /*
  * QEMU Crypto hash algorithms
  *
+ * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates
  * Copyright (c) 2016 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
@@ -155,7 +156,83 @@ qcrypto_nettle_hash_bytesv(QCryptoHashAlgorithm alg,
     return 0;
 }
 
+static
+QCryptoHash *qcrypto_nettle_hash_new(QCryptoHashAlgorithm alg, Error **errp)
+{
+    QCryptoHash *hash = NULL;
+
+    if (!qcrypto_hash_supports(alg)) {
+        error_setg(errp,
+                   "Unknown hash algorithm %d",
+                   alg);
+    } else {
+        hash = g_new(QCryptoHash, 1);
+        hash->alg = alg;
+        hash->opaque = g_new(union qcrypto_hash_ctx, 1);
+
+        qcrypto_hash_alg_map[alg].init(hash->opaque);
+    }
+
+    return hash;
+}
+
+static
+void qcrypto_nettle_hash_free(QCryptoHash *hash)
+{
+    union qcrypto_hash_ctx *ctx = hash->opaque;
+
+    g_free(ctx);
+    g_free(hash);
+}
+
+static
+int qcrypto_nettle_hash_update(QCryptoHash *hash,
+                               const struct iovec *iov,
+                               size_t niov,
+                               Error **errp)
+{
+    union qcrypto_hash_ctx *ctx = hash->opaque;
+
+    for (int i = 0; i < niov; i++) {
+        /*
+         * Some versions of nettle have functions
+         * declared with 'int' instead of 'size_t'
+         * so to be safe avoid writing more than
+         * UINT_MAX bytes at a time
+         */
+        size_t len = iov[i].iov_len;
+        uint8_t *base = iov[i].iov_base;
+        while (len) {
+            size_t shortlen = MIN(len, UINT_MAX);
+            qcrypto_hash_alg_map[hash->alg].write(ctx, len, base);
+            len -= shortlen;
+            base += len;
+        }
+    }
+
+    return 0;
+}
+
+static
+int qcrypto_nettle_hash_finalize(QCryptoHash *hash,
+                                 uint8_t **result,
+                                 size_t *result_len,
+                                 Error **errp)
+{
+    union qcrypto_hash_ctx *ctx = hash->opaque;
+
+    *result_len = qcrypto_hash_alg_map[hash->alg].len;
+    *result = g_new(uint8_t, *result_len);
+
+    qcrypto_hash_alg_map[hash->alg].result(ctx, *result_len, *result);
+
+    return 0;
+}
 
 QCryptoHashDriver qcrypto_hash_lib_driver = {
     .hash_bytesv = qcrypto_nettle_hash_bytesv,
+    .hash_new      = qcrypto_nettle_hash_new,
+    .hash_update   = qcrypto_nettle_hash_update,
+    .hash_finalize = qcrypto_nettle_hash_finalize,
+    .hash_free     = qcrypto_nettle_hash_free,
 };
-- 
2.34.1

[PATCH v4 06/15] crypto/hash-afalg: Implement new hash API

[Alejandro Zeise]

Updates the afalg hash driver to support the new accumulative
hashing changes as part of the patch series.

Implements opening/closing of contexts, updating hash data
and finalizing the hash digest.

In order to support the update function, a flag needs to be passed
to the kernel via the socket send call (MSG_MORE) to notify it that more
data is to be expected to calculate the hash correctly.
As a result, a new function was added to the iov helper utils to allow
passing a flag to the socket send call.

Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
---
 crypto/hash-afalg.c | 126 ++++++++++++++++++++++++++++++++++++++++++++
 include/qemu/iov.h  |  26 +++++++++
 util/iov.c          |  22 +++++---
 3 files changed, 167 insertions(+), 7 deletions(-)

diff --git a/crypto/hash-afalg.c b/crypto/hash-afalg.c
index 3ebea39292..9548c04933 100644
--- a/crypto/hash-afalg.c
+++ b/crypto/hash-afalg.c
@@ -1,6 +1,7 @@
 /*
  * QEMU Crypto af_alg-backend hash/hmac support
  *
+ * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates
  * Copyright (c) 2017 HUAWEI TECHNOLOGIES CO., LTD.
  *
  * Authors:
@@ -113,6 +114,127 @@ qcrypto_afalg_hmac_ctx_new(QCryptoHashAlgorithm alg,
     return qcrypto_afalg_hash_hmac_ctx_new(alg, key, nkey, true, errp);
 }
 
+static
+QCryptoHash *qcrypto_afalg_hash_new(QCryptoHashAlgorithm alg, Error **errp)
+{
+    /* Check if hash algorithm is supported */
+    char *alg_name = qcrypto_afalg_hash_format_name(alg, false, NULL);
+    QCryptoHash *hash = NULL;
+
+    if (alg_name == NULL) {
+        error_setg(errp,
+                   "Unknown hash algorithm %d",
+                   alg);
+    } else {
+        hash = g_new(QCryptoHash, 1);
+        hash->alg = alg;
+        hash->opaque = qcrypto_afalg_hash_ctx_new(alg, errp);
+    }
+
+    return hash;
+}
+
+static
+void qcrypto_afalg_hash_free(QCryptoHash *hash)
+{
+    QCryptoAFAlg *ctx = hash->opaque;
+
+    if (ctx) {
+        qcrypto_afalg_comm_free(ctx);
+    }
+
+    g_free(hash);
+}
+
+/**
+ * Send data to the kernel's crypto core.
+ *
+ * The more_data parameter is used to notify the crypto engine
+ * that this is an "update" operation, and that more data will
+ * be provided to calculate the final hash.
+ */
+static
+int qcrypto_afalg_send_to_kernel(QCryptoAFAlg *afalg,
+                                 const struct iovec *iov,
+                                 size_t niov,
+                                 bool more_data,
+                                 Error **errp)
+{
+    int ret = 0;
+    int flags = (more_data ? MSG_MORE : 0);
+
+    /* send data to kernel's crypto core */
+    ret = iov_send_recv_with_flags(afalg->opfd, flags, iov, niov,
+                                   0, iov_size(iov, niov), true);
+    if (ret < 0) {
+        error_setg_errno(errp, errno, "Send data to afalg-core failed");
+        ret = -1;
+    } else {
+        /* No error, so return 0 */
+        ret = 0;
+    }
+
+    return ret;
+}
+
+static
+int qcrypto_afalg_recv_from_kernel(QCryptoAFAlg *afalg,
+                                   QCryptoHashAlgorithm alg,
+                                   uint8_t **result,
+                                   size_t *result_len,
+                                   Error **errp)
+{
+    struct iovec outv;
+    int ret = 0;
+    const int expected_len = qcrypto_hash_digest_len(alg);
+
+    if (*result_len == 0) {
+        *result_len = expected_len;
+        *result = g_new0(uint8_t, *result_len);
+    } else if (*result_len != expected_len) {
+        error_setg(errp,
+                   "Result buffer size %zu is not match hash %d",
+                   *result_len, expected_len);
+        ret = -1;
+    }
+
+    if (ret == 0) {
+        /* hash && get result */
+        outv.iov_base = *result;
+        outv.iov_len = *result_len;
+        ret = iov_send_recv(afalg->opfd, &outv, 1,
+                            0, iov_size(&outv, 1), false);
+        if (ret < 0) {
+            error_setg_errno(errp, errno, "Recv result from afalg-core failed");
+            ret = -1;
+        } else {
+            ret = 0;
+        }
+    }
+
+    return ret;
+}
+
+static
+int qcrypto_afalg_hash_update(QCryptoHash *hash,
+                              const struct iovec *iov,
+                              size_t niov,
+                              Error **errp)
+{
+    return qcrypto_afalg_send_to_kernel((QCryptoAFAlg *) hash->opaque,
+                                        iov, niov, true, errp);
+}
+
+static
+int qcrypto_afalg_hash_finalize(QCryptoHash *hash,
+                                 uint8_t **result,
+                                 size_t *result_len,
+                                 Error **errp)
+{
+    return qcrypto_afalg_recv_from_kernel((QCryptoAFAlg *) hash->opaque,
+                                          hash->alg, result, result_len, errp);
+}
+
 static int
 qcrypto_afalg_hash_hmac_bytesv(QCryptoAFAlg *hmac,
                                QCryptoHashAlgorithm alg,
@@ -205,6 +327,10 @@ static void qcrypto_afalg_hmac_ctx_free(QCryptoHmac *hmac)
 
 QCryptoHashDriver qcrypto_hash_afalg_driver = {
     .hash_bytesv = qcrypto_afalg_hash_bytesv,
+    .hash_new      = qcrypto_afalg_hash_new,
+    .hash_free     = qcrypto_afalg_hash_free,
+    .hash_update   = qcrypto_afalg_hash_update,
+    .hash_finalize = qcrypto_afalg_hash_finalize
 };
 
 QCryptoHmacDriver qcrypto_hmac_afalg_driver = {
diff --git a/include/qemu/iov.h b/include/qemu/iov.h
index 63a1c01965..43884cdd64 100644
--- a/include/qemu/iov.h
+++ b/include/qemu/iov.h
@@ -1,6 +1,7 @@
 /*
  * Helpers for using (partial) iovecs.
  *
+ * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates
  * Copyright (C) 2010 Red Hat, Inc.
  *
  * Author(s):
@@ -75,6 +76,31 @@ iov_to_buf(const struct iovec *iov, const unsigned int iov_cnt,
 size_t iov_memset(const struct iovec *iov, const unsigned int iov_cnt,
                   size_t offset, int fillc, size_t bytes);
 
+/*
+ * Send/recv data from/to iovec buffers directly, with the provided
+ * socket flags.
+ *
+ * `offset' bytes in the beginning of iovec buffer are skipped and
+ * next `bytes' bytes are used, which must be within data of iovec.
+ *
+ *   r = iov_send_recv_with_flags(sockfd, sockflags, iov, iovcnt, offset, bytes, true);
+ *
+ * is logically equivalent to
+ *
+ *   char *buf = malloc(bytes);
+ *   iov_to_buf(iov, iovcnt, offset, buf, bytes);
+ *   r = send(sockfd, buf, bytes, sockflags);
+ *   free(buf);
+ *
+ * For iov_send_recv_with_flags() _whole_ area being sent or received
+ * should be within the iovec, not only beginning of it.
+ */
+ssize_t iov_send_recv_with_flags(int sockfd, int sockflags,
+                                 const struct iovec *iov,
+                                 unsigned iov_cnt, size_t offset,
+                                 size_t bytes,
+                                 bool do_send);
+
 /*
  * Send/recv data from/to iovec buffers directly
  *
diff --git a/util/iov.c b/util/iov.c
index 7e73948f5e..5644e0b73c 100644
--- a/util/iov.c
+++ b/util/iov.c
@@ -3,6 +3,7 @@
  *
  * Copyright IBM, Corp. 2007, 2008
  * Copyright (C) 2010 Red Hat, Inc.
+ * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates
  *
  * Author(s):
  *  Anthony Liguori <aliguori@us.ibm.com>
@@ -92,7 +93,7 @@ size_t iov_size(const struct iovec *iov, const unsigned int iov_cnt)
 
 /* helper function for iov_send_recv() */
 static ssize_t
-do_send_recv(int sockfd, struct iovec *iov, unsigned iov_cnt, bool do_send)
+do_send_recv(int sockfd, int flags, struct iovec *iov, unsigned iov_cnt, bool do_send)
 {
 #ifdef CONFIG_POSIX
     ssize_t ret;
@@ -102,8 +103,8 @@ do_send_recv(int sockfd, struct iovec *iov, unsigned iov_cnt, bool do_send)
     msg.msg_iovlen = iov_cnt;
     do {
         ret = do_send
-            ? sendmsg(sockfd, &msg, 0)
-            : recvmsg(sockfd, &msg, 0);
+            ? sendmsg(sockfd, &msg, flags)
+            : recvmsg(sockfd, &msg, flags);
     } while (ret < 0 && errno == EINTR);
     return ret;
 #else
@@ -114,8 +115,8 @@ do_send_recv(int sockfd, struct iovec *iov, unsigned iov_cnt, bool do_send)
     ssize_t off = 0;
     while (i < iov_cnt) {
         ssize_t r = do_send
-            ? send(sockfd, iov[i].iov_base + off, iov[i].iov_len - off, 0)
-            : recv(sockfd, iov[i].iov_base + off, iov[i].iov_len - off, 0);
+            ? send(sockfd, iov[i].iov_base + off, iov[i].iov_len - off, flags)
+            : recv(sockfd, iov[i].iov_base + off, iov[i].iov_len - off, flags);
         if (r > 0) {
             ret += r;
             off += r;
@@ -144,6 +145,13 @@ do_send_recv(int sockfd, struct iovec *iov, unsigned iov_cnt, bool do_send)
 ssize_t iov_send_recv(int sockfd, const struct iovec *_iov, unsigned iov_cnt,
                       size_t offset, size_t bytes,
                       bool do_send)
+{
+    return iov_send_recv_with_flags(sockfd, 0, _iov, iov_cnt, offset, bytes, do_send);
+}
+
+ssize_t iov_send_recv_with_flags(int sockfd, int sockflags, const struct iovec *_iov,
+                                 unsigned iov_cnt, size_t offset, size_t bytes,
+                                 bool do_send)
 {
     ssize_t total = 0;
     ssize_t ret;
@@ -192,11 +200,11 @@ ssize_t iov_send_recv(int sockfd, const struct iovec *_iov, unsigned iov_cnt,
             assert(iov[niov].iov_len > tail);
             orig_len = iov[niov].iov_len;
             iov[niov++].iov_len = tail;
-            ret = do_send_recv(sockfd, iov, niov, do_send);
+            ret = do_send_recv(sockfd, sockflags, iov, niov, do_send);
             /* Undo the changes above before checking for errors */
             iov[niov-1].iov_len = orig_len;
         } else {
-            ret = do_send_recv(sockfd, iov, niov, do_send);
+            ret = do_send_recv(sockfd, sockflags, iov, niov, do_send);
         }
         if (offset) {
             iov[0].iov_base -= offset;
-- 
2.34.1

[PATCH v4 07/15] crypto/hash: Implement and use new hash API

[Alejandro Zeise]

Changes the public hash API implementation to support accumulative hashing.

Implementations for the public functions are added to call the new
driver functions that implement context creation, updating,
finalization, and destruction.

Additionally changes the "shortcut" functions to use these 4 new core
functions.

Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
---
 crypto/hash.c | 163 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 128 insertions(+), 35 deletions(-)

diff --git a/crypto/hash.c b/crypto/hash.c
index b0f8228bdc..f3c18cdd74 100644
--- a/crypto/hash.c
+++ b/crypto/hash.c
@@ -1,6 +1,7 @@
 /*
  * QEMU Crypto hash algorithms
  *
+ * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates
  * Copyright (c) 2015 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
@@ -45,23 +46,20 @@ int qcrypto_hash_bytesv(QCryptoHashAlgorithm alg,
                         size_t *resultlen,
                         Error **errp)
 {
-#ifdef CONFIG_AF_ALG
-    int ret;
-    /*
-     * TODO:
-     * Maybe we should treat some afalg errors as fatal
-     */
-    ret = qcrypto_hash_afalg_driver.hash_bytesv(alg, iov, niov,
-                                                result, resultlen,
-                                                NULL);
-    if (ret == 0) {
-        return ret;
+    int fail;
+    QCryptoHash *ctx = qcrypto_hash_new(alg, errp);
+
+    if (ctx) {
+        fail = qcrypto_hash_updatev(ctx, iov, niov, errp) ||
+               qcrypto_hash_finalize_bytes(ctx, result, resultlen, errp);
+
+        /* Ensure context is always freed regardless of error */
+        qcrypto_hash_free(ctx);
+    } else {
+        fail = -1;
     }
-#endif
 
-    return qcrypto_hash_lib_driver.hash_bytesv(alg, iov, niov,
-                                               result, resultlen,
-                                               errp);
+    return fail;
 }
 
 
@@ -77,30 +75,121 @@ int qcrypto_hash_bytes(QCryptoHashAlgorithm alg,
     return qcrypto_hash_bytesv(alg, &iov, 1, result, resultlen, errp);
 }
 
-static const char hex[] = "0123456789abcdef";
-
-int qcrypto_hash_digestv(QCryptoHashAlgorithm alg,
+int qcrypto_hash_updatev(QCryptoHash *hash,
                          const struct iovec *iov,
                          size_t niov,
-                         char **digest,
                          Error **errp)
 {
+#ifdef CONFIG_AF_ALG
+    return qcrypto_hash_afalg_driver.hash_update(hash, iov, niov, errp);
+#else
+    return qcrypto_hash_lib_driver.hash_update(hash, iov, niov, errp);
+#endif /* CONFIG_AF_ALG */
+}
+
+int qcrypto_hash_update(QCryptoHash *hash,
+                        const char *buf,
+                        size_t len,
+                        Error **errp)
+{
+    struct iovec iov = { .iov_base = (char *)buf, .iov_len = len };
+
+    return qcrypto_hash_updatev(hash, &iov, 1, errp);
+}
+
+QCryptoHash *qcrypto_hash_new(QCryptoHashAlgorithm alg, Error **errp)
+{
+#ifdef CONFIG_AF_ALG
+    return qcrypto_hash_afalg_driver.hash_new(alg, errp);
+#else
+    return qcrypto_hash_lib_driver.hash_new(alg, errp);
+#endif /* CONFIG_AF_ALG */
+}
+
+void qcrypto_hash_free(QCryptoHash *hash)
+{
+#ifdef CONFIG_AF_ALG
+    qcrypto_hash_afalg_driver.hash_free(hash);
+#else
+    qcrypto_hash_lib_driver.hash_free(hash);
+#endif /* CONFIG_AF_ALG */
+}
+
+int qcrypto_hash_finalize_bytes(QCryptoHash *hash,
+                                uint8_t **result,
+                                size_t *result_len,
+                                Error **errp)
+{
+#ifdef CONFIG_AF_ALG
+    return qcrypto_hash_afalg_driver.hash_finalize(hash, result, result_len,
+                                                   errp);
+#else
+    return qcrypto_hash_lib_driver.hash_finalize(hash, result, result_len, errp);
+#endif /* CONFIG_AF_ALG */
+}
+
+static const char hex[] = "0123456789abcdef";
+
+int qcrypto_hash_finalize_digest(QCryptoHash *hash,
+                                 char **digest,
+                                 Error **errp)
+{
+    int ret;
     uint8_t *result = NULL;
     size_t resultlen = 0;
     size_t i;
 
-    if (qcrypto_hash_bytesv(alg, iov, niov, &result, &resultlen, errp) < 0) {
-        return -1;
+    ret = qcrypto_hash_finalize_bytes(hash, &result, &resultlen, errp);
+    if (ret == 0) {
+        *digest = g_new0(char, (resultlen * 2) + 1);
+        for (i = 0 ; i < resultlen ; i++) {
+            (*digest)[(i * 2)] = hex[(result[i] >> 4) & 0xf];
+            (*digest)[(i * 2) + 1] = hex[result[i] & 0xf];
+        }
+        (*digest)[resultlen * 2] = '\0';
+        g_free(result);
+    }
+
+    return ret;
+}
+
+int qcrypto_hash_finalize_base64(QCryptoHash *hash,
+                                 char **base64,
+                                 Error **errp)
+{
+    int ret;
+    uint8_t *result = NULL;
+    size_t resultlen = 0;
+
+    ret = qcrypto_hash_finalize_bytes(hash, &result, &resultlen, errp);
+    if (ret == 0) {
+        *base64 = g_base64_encode(result, resultlen);
+        g_free(result);
     }
 
-    *digest = g_new0(char, (resultlen * 2) + 1);
-    for (i = 0 ; i < resultlen ; i++) {
-        (*digest)[(i * 2)] = hex[(result[i] >> 4) & 0xf];
-        (*digest)[(i * 2) + 1] = hex[result[i] & 0xf];
+    return ret;
+}
+
+int qcrypto_hash_digestv(QCryptoHashAlgorithm alg,
+                         const struct iovec *iov,
+                         size_t niov,
+                         char **digest,
+                         Error **errp)
+{
+    bool fail;
+    QCryptoHash *ctx = qcrypto_hash_new(alg, errp);
+
+    if (ctx) {
+        fail = qcrypto_hash_updatev(ctx, iov, niov, errp) ||
+               qcrypto_hash_finalize_digest(ctx, digest, errp);
+
+        /* Ensure context is always freed regardless of error */
+        qcrypto_hash_free(ctx);
+    } else {
+        fail = false;
     }
-    (*digest)[resultlen * 2] = '\0';
-    g_free(result);
-    return 0;
+
+    return fail;
 }
 
 int qcrypto_hash_digest(QCryptoHashAlgorithm alg,
@@ -120,16 +209,20 @@ int qcrypto_hash_base64v(QCryptoHashAlgorithm alg,
                          char **base64,
                          Error **errp)
 {
-    uint8_t *result = NULL;
-    size_t resultlen = 0;
+    bool fail;
+    QCryptoHash *ctx = qcrypto_hash_new(alg, errp);
+
+    if (ctx) {
+        fail = qcrypto_hash_updatev(ctx, iov, niov, errp) ||
+               qcrypto_hash_finalize_base64(ctx, base64, errp);
 
-    if (qcrypto_hash_bytesv(alg, iov, niov, &result, &resultlen, errp) < 0) {
-        return -1;
+        /* Ensure context is always freed regardless of error */
+        qcrypto_hash_free(ctx);
+    } else {
+        fail = 1;
     }
 
-    *base64 = g_base64_encode(result, resultlen);
-    g_free(result);
-    return 0;
+    return fail;
 }
 
 int qcrypto_hash_base64(QCryptoHashAlgorithm alg,
-- 
2.34.1

[PATCH v4 08/15] tests/unit/test-crypto-hash: accumulative hashing

[Alejandro Zeise]

Added an accumulative hashing test. Checks for functionality of
the new hash create, update, finalize and free functions.

Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
---
 tests/unit/test-crypto-hash.c | 48 +++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/tests/unit/test-crypto-hash.c b/tests/unit/test-crypto-hash.c
index 1f4abb822b..2bf9bcb6a0 100644
--- a/tests/unit/test-crypto-hash.c
+++ b/tests/unit/test-crypto-hash.c
@@ -1,6 +1,7 @@
 /*
  * QEMU Crypto hash algorithms
  *
+ * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates
  * Copyright (c) 2015 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
@@ -241,6 +242,52 @@ static void test_hash_base64(void)
     }
 }
 
+static void test_hash_accumulate(void)
+{
+    QCryptoHash *hash;
+    size_t i;
+
+    for (i = 0; i < G_N_ELEMENTS(expected_outputs) ; i++) {
+        struct iovec iov[3] = {
+            { .iov_base = (char *)INPUT_TEXT1, .iov_len = strlen(INPUT_TEXT1) },
+            { .iov_base = (char *)INPUT_TEXT2, .iov_len = strlen(INPUT_TEXT2) },
+            { .iov_base = (char *)INPUT_TEXT3, .iov_len = strlen(INPUT_TEXT3) },
+        };
+        uint8_t *result = NULL;
+        size_t resultlen = 0;
+        int ret;
+        size_t j;
+
+        if (!qcrypto_hash_supports(i)) {
+            continue;
+        }
+
+        hash = qcrypto_hash_new(i, &error_fatal);
+        g_assert(hash != NULL);
+
+        /* Add each iovec to the hash context separately */
+        for (j = 0; j < 3; j++) {
+            ret = qcrypto_hash_updatev(hash,
+                                      &iov[j], 1,
+                                      &error_fatal);
+
+            g_assert(ret == 0);
+        }
+
+        ret = qcrypto_hash_finalize_bytes(hash, &result, &resultlen,
+                                          &error_fatal);
+
+        g_assert(ret == 0);
+        g_assert(resultlen == expected_lens[i]);
+        for (j = 0; j < resultlen; j++) {
+            g_assert(expected_outputs[i][j * 2] == hex[(result[j] >> 4) & 0xf]);
+            g_assert(expected_outputs[i][j * 2 + 1] == hex[result[j] & 0xf]);
+        }
+        g_free(result);
+        qcrypto_hash_free(hash);
+    }
+}
+
 int main(int argc, char **argv)
 {
     int ret = qcrypto_init(&error_fatal);
@@ -252,5 +299,6 @@ int main(int argc, char **argv)
     g_test_add_func("/crypto/hash/prealloc", test_hash_prealloc);
     g_test_add_func("/crypto/hash/digest", test_hash_digest);
     g_test_add_func("/crypto/hash/base64", test_hash_base64);
+    g_test_add_func("/crypto/hash/accumulate", test_hash_accumulate);
     return g_test_run();
 }
-- 
2.34.1

[PATCH v4 09/15] crypto/hash-glib: Remove old hash API functions

[Alejandro Zeise]

Removes old hash implemention in the GLib hash driver.

Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
---
 crypto/hash-glib.c | 53 ----------------------------------------------
 1 file changed, 53 deletions(-)

diff --git a/crypto/hash-glib.c b/crypto/hash-glib.c
index 9f4490762a..d1dc00547d 100644
--- a/crypto/hash-glib.c
+++ b/crypto/hash-glib.c
@@ -44,58 +44,6 @@ gboolean qcrypto_hash_supports(QCryptoHashAlgorithm alg)
     return false;
 }
 
-
-static int
-qcrypto_glib_hash_bytesv(QCryptoHashAlgorithm alg,
-                         const struct iovec *iov,
-                         size_t niov,
-                         uint8_t **result,
-                         size_t *resultlen,
-                         Error **errp)
-{
-    int i, ret;
-    GChecksum *cs;
-
-    if (!qcrypto_hash_supports(alg)) {
-        error_setg(errp,
-                   "Unknown hash algorithm %d",
-                   alg);
-        return -1;
-    }
-
-    cs = g_checksum_new(qcrypto_hash_alg_map[alg]);
-
-    for (i = 0; i < niov; i++) {
-        g_checksum_update(cs, iov[i].iov_base, iov[i].iov_len);
-    }
-
-    ret = g_checksum_type_get_length(qcrypto_hash_alg_map[alg]);
-    if (ret < 0) {
-        error_setg(errp, "%s",
-                   "Unable to get hash length");
-        goto error;
-    }
-    if (*resultlen == 0) {
-        *resultlen = ret;
-        *result = g_new0(uint8_t, *resultlen);
-    } else if (*resultlen != ret) {
-        error_setg(errp,
-                   "Result buffer size %zu is smaller than hash %d",
-                   *resultlen, ret);
-        goto error;
-    }
-
-    g_checksum_get_digest(cs, *result, resultlen);
-
-    g_checksum_free(cs);
-    return 0;
-
- error:
-    g_checksum_free(cs);
-    return -1;
-}
-
-
 static
 QCryptoHash *qcrypto_glib_hash_new(QCryptoHashAlgorithm alg,
                                    Error **errp)
@@ -169,7 +117,6 @@ int qcrypto_glib_hash_finalize(QCryptoHash *hash,
 }
 
 QCryptoHashDriver qcrypto_hash_lib_driver = {
-    .hash_bytesv = qcrypto_glib_hash_bytesv,
     .hash_new      = qcrypto_glib_hash_new,
     .hash_update   = qcrypto_glib_hash_update,
     .hash_finalize = qcrypto_glib_hash_finalize,
-- 
2.34.1

[PATCH v4 10/15] crypto/hash-gcrypt: Remove old hash API functions

[Alejandro Zeise]

Removes old hash implemention in the gcrypt hash driver.

Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
---
 crypto/hash-gcrypt.c | 67 --------------------------------------------
 1 file changed, 67 deletions(-)

diff --git a/crypto/hash-gcrypt.c b/crypto/hash-gcrypt.c
index e05511cafa..7779ec8446 100644
--- a/crypto/hash-gcrypt.c
+++ b/crypto/hash-gcrypt.c
@@ -45,72 +45,6 @@ gboolean qcrypto_hash_supports(QCryptoHashAlgorithm alg)
     return false;
 }
 
-
-static int
-qcrypto_gcrypt_hash_bytesv(QCryptoHashAlgorithm alg,
-                           const struct iovec *iov,
-                           size_t niov,
-                           uint8_t **result,
-                           size_t *resultlen,
-                           Error **errp)
-{
-    int i, ret;
-    gcry_md_hd_t md;
-    unsigned char *digest;
-
-    if (!qcrypto_hash_supports(alg)) {
-        error_setg(errp,
-                   "Unknown hash algorithm %d",
-                   alg);
-        return -1;
-    }
-
-    ret = gcry_md_open(&md, qcrypto_hash_alg_map[alg], 0);
-
-    if (ret < 0) {
-        error_setg(errp,
-                   "Unable to initialize hash algorithm: %s",
-                   gcry_strerror(ret));
-        return -1;
-    }
-
-    for (i = 0; i < niov; i++) {
-        gcry_md_write(md, iov[i].iov_base, iov[i].iov_len);
-    }
-
-    ret = gcry_md_get_algo_dlen(qcrypto_hash_alg_map[alg]);
-    if (ret <= 0) {
-        error_setg(errp,
-                   "Unable to get hash length: %s",
-                   gcry_strerror(ret));
-        goto error;
-    }
-    if (*resultlen == 0) {
-        *resultlen = ret;
-        *result = g_new0(uint8_t, *resultlen);
-    } else if (*resultlen != ret) {
-        error_setg(errp,
-                   "Result buffer size %zu is smaller than hash %d",
-                   *resultlen, ret);
-        goto error;
-    }
-
-    digest = gcry_md_read(md, 0);
-    if (!digest) {
-        error_setg(errp,
-                   "No digest produced");
-        goto error;
-    }
-    memcpy(*result, digest, *resultlen);
-
-    gcry_md_close(md);
-    return 0;
-
- error:
-    gcry_md_close(md);
-    return -1;
-}
-
 static
 QCryptoHash *qcrypto_gcrypt_hash_new(QCryptoHashAlgorithm alg, Error **errp)
 {
@@ -187,7 +121,6 @@ int qcrypto_gcrypt_hash_finalize(QCryptoHash *hash,
 }
 
 QCryptoHashDriver qcrypto_hash_lib_driver = {
-    .hash_bytesv = qcrypto_gcrypt_hash_bytesv,
     .hash_new      = qcrypto_gcrypt_hash_new,
     .hash_update   = qcrypto_gcrypt_hash_update,
     .hash_finalize = qcrypto_gcrypt_hash_finalize,
-- 
2.34.1

[PATCH v4 11/15] crypto/hash-gnutls: Remove old hash API functions

[Alejandro Zeise]

Removes old hash implemention in the gnutls hash driver.

Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
---
 crypto/hash-gnutls.c | 47 --------------------------------------------
 1 file changed, 47 deletions(-)

diff --git a/crypto/hash-gnutls.c b/crypto/hash-gnutls.c
index 15fc630a11..0c24b0eb66 100644
--- a/crypto/hash-gnutls.c
+++ b/crypto/hash-gnutls.c
@@ -53,52 +53,6 @@ gboolean qcrypto_hash_supports(QCryptoHashAlgorithm alg)
     return false;
 }
 
-
-static int
-qcrypto_gnutls_hash_bytesv(QCryptoHashAlgorithm alg,
-                           const struct iovec *iov,
-                           size_t niov,
-                           uint8_t **result,
-                           size_t *resultlen,
-                           Error **errp)
-{
-    int i, ret;
-    gnutls_hash_hd_t hash;
-
-    if (!qcrypto_hash_supports(alg)) {
-        error_setg(errp,
-                   "Unknown hash algorithm %d",
-                   alg);
-        return -1;
-    }
-
-    ret = gnutls_hash_get_len(qcrypto_hash_alg_map[alg]);
-    if (*resultlen == 0) {
-        *resultlen = ret;
-        *result = g_new0(uint8_t, *resultlen);
-    } else if (*resultlen != ret) {
-        error_setg(errp,
-                   "Result buffer size %zu is smaller than hash %d",
-                   *resultlen, ret);
-        return -1;
-    }
-
-    ret = gnutls_hash_init(&hash, qcrypto_hash_alg_map[alg]);
-    if (ret < 0) {
-        error_setg(errp,
-                   "Unable to initialize hash algorithm: %s",
-                   gnutls_strerror(ret));
-        return -1;
-    }
-
-    for (i = 0; i < niov; i++) {
-        gnutls_hash(hash, iov[i].iov_base, iov[i].iov_len);
-    }
-
-    gnutls_hash_deinit(hash, *result);
-    return 0;
-}
-
 static
 QCryptoHash *qcrypto_gnutls_hash_new(QCryptoHashAlgorithm alg, Error **errp)
 {
@@ -169,7 +123,6 @@ int qcrypto_gnutls_hash_finalize(QCryptoHash *hash,
 }
 
 QCryptoHashDriver qcrypto_hash_lib_driver = {
-    .hash_bytesv = qcrypto_gnutls_hash_bytesv,
     .hash_new      = qcrypto_gnutls_hash_new,
     .hash_update   = qcrypto_gnutls_hash_update,
     .hash_finalize = qcrypto_gnutls_hash_finalize,
-- 
2.34.1

[PATCH v4 12/15] crypto/hash-nettle: Remove old hash API functions

[Alejandro Zeise]

Removes old hash implemention in the nettle hash driver.

Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
---
 crypto/hash-nettle.c | 53 --------------------------------------------
 1 file changed, 53 deletions(-)

diff --git a/crypto/hash-nettle.c b/crypto/hash-nettle.c
index 894c7b4fc3..bd489f865e 100644
--- a/crypto/hash-nettle.c
+++ b/crypto/hash-nettle.c
@@ -104,58 +104,6 @@ gboolean qcrypto_hash_supports(QCryptoHashAlgorithm alg)
     return false;
 }
 
-
-static int
-qcrypto_nettle_hash_bytesv(QCryptoHashAlgorithm alg,
-                           const struct iovec *iov,
-                           size_t niov,
-                           uint8_t **result,
-                           size_t *resultlen,
-                           Error **errp)
-{
-    size_t i;
-    union qcrypto_hash_ctx ctx;
-
-    if (!qcrypto_hash_supports(alg)) {
-        error_setg(errp,
-                   "Unknown hash algorithm %d",
-                   alg);
-        return -1;
-    }
-
-    qcrypto_hash_alg_map[alg].init(&ctx);
-
-    for (i = 0; i < niov; i++) {
-        /* Some versions of nettle have functions
-         * declared with 'int' instead of 'size_t'
-         * so to be safe avoid writing more than
-         * UINT_MAX bytes at a time
-         */
-        size_t len = iov[i].iov_len;
-        uint8_t *base = iov[i].iov_base;
-        while (len) {
-            size_t shortlen = MIN(len, UINT_MAX);
-            qcrypto_hash_alg_map[alg].write(&ctx, len, base);
-            len -= shortlen;
-            base += len;
-        }
-    }
-
-    if (*resultlen == 0) {
-        *resultlen = qcrypto_hash_alg_map[alg].len;
-        *result = g_new0(uint8_t, *resultlen);
-    } else if (*resultlen != qcrypto_hash_alg_map[alg].len) {
-        error_setg(errp,
-                   "Result buffer size %zu is smaller than hash %zu",
-                   *resultlen, qcrypto_hash_alg_map[alg].len);
-        return -1;
-    }
-
-    qcrypto_hash_alg_map[alg].result(&ctx, *resultlen, *result);
-
-    return 0;
-}
-
 static
 QCryptoHash *qcrypto_nettle_hash_new(QCryptoHashAlgorithm alg, Error **errp)
 {
@@ -230,7 +178,6 @@ int qcrypto_nettle_hash_finalize(QCryptoHash *hash,
 }
 
 QCryptoHashDriver qcrypto_hash_lib_driver = {
-    .hash_bytesv = qcrypto_nettle_hash_bytesv,
     .hash_new      = qcrypto_nettle_hash_new,
     .hash_update   = qcrypto_nettle_hash_update,
     .hash_finalize = qcrypto_nettle_hash_finalize,
-- 
2.34.1

[PATCH v4 13/15] crypto/hash-afalg: Remove old hash API functions

[Alejandro Zeise]

Removes the old hash API functions in the afalg driver,
and modifies the hmac function to use the new helper functions.

Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
---
 crypto/hash-afalg.c | 59 +++------------------------------------------
 1 file changed, 3 insertions(+), 56 deletions(-)

diff --git a/crypto/hash-afalg.c b/crypto/hash-afalg.c
index 9548c04933..351f931995 100644
--- a/crypto/hash-afalg.c
+++ b/crypto/hash-afalg.c
@@ -243,68 +243,16 @@ qcrypto_afalg_hash_hmac_bytesv(QCryptoAFAlg *hmac,
                                size_t *resultlen,
                                Error **errp)
 {
-    QCryptoAFAlg *afalg;
-    struct iovec outv;
     int ret = 0;
-    bool is_hmac = (hmac != NULL) ? true : false;
-    const int expect_len = qcrypto_hash_digest_len(alg);
-
-    if (*resultlen == 0) {
-        *resultlen = expect_len;
-        *result = g_new0(uint8_t, *resultlen);
-    } else if (*resultlen != expect_len) {
-        error_setg(errp,
-                   "Result buffer size %zu is not match hash %d",
-                   *resultlen, expect_len);
-        return -1;
-    }
 
-    if (is_hmac) {
-        afalg = hmac;
-    } else {
-        afalg = qcrypto_afalg_hash_ctx_new(alg, errp);
-        if (!afalg) {
-            return -1;
-        }
-    }
-
-    /* send data to kernel's crypto core */
-    ret = iov_send_recv(afalg->opfd, iov, niov,
-                        0, iov_size(iov, niov), true);
-    if (ret < 0) {
-        error_setg_errno(errp, errno, "Send data to afalg-core failed");
-        goto out;
-    }
-
-    /* hash && get result */
-    outv.iov_base = *result;
-    outv.iov_len = *resultlen;
-    ret = iov_send_recv(afalg->opfd, &outv, 1,
-                        0, iov_size(&outv, 1), false);
-    if (ret < 0) {
-        error_setg_errno(errp, errno, "Recv result from afalg-core failed");
-    } else {
-        ret = 0;
+    ret = qcrypto_afalg_send_to_kernel(hmac, iov, niov, false, errp);
+    if (ret == 0) {
+        ret = qcrypto_afalg_recv_from_kernel(hmac, alg, result, resultlen, errp);
     }
 
-out:
-    if (!is_hmac) {
-        qcrypto_afalg_comm_free(afalg);
-    }
     return ret;
 }
 
-static int
-qcrypto_afalg_hash_bytesv(QCryptoHashAlgorithm alg,
-                          const struct iovec *iov,
-                          size_t niov, uint8_t **result,
-                          size_t *resultlen,
-                          Error **errp)
-{
-    return qcrypto_afalg_hash_hmac_bytesv(NULL, alg, iov, niov, result,
-                                          resultlen, errp);
-}
-
 static int
 qcrypto_afalg_hmac_bytesv(QCryptoHmac *hmac,
                           const struct iovec *iov,
@@ -326,7 +274,6 @@ static void qcrypto_afalg_hmac_ctx_free(QCryptoHmac *hmac)
 }
 
 QCryptoHashDriver qcrypto_hash_afalg_driver = {
-    .hash_bytesv = qcrypto_afalg_hash_bytesv,
     .hash_new      = qcrypto_afalg_hash_new,
     .hash_free     = qcrypto_afalg_hash_free,
     .hash_update   = qcrypto_afalg_hash_update,
-- 
2.34.1

[PATCH v4 14/15] crypto/hashpriv: Remove old hash API function

[Alejandro Zeise]

Remove old hash_bytesv function, as it was replaced by the 4
new functions.

Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
---
 crypto/hashpriv.h | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/crypto/hashpriv.h b/crypto/hashpriv.h
index 02f17ee99f..aec29b3ec3 100644
--- a/crypto/hashpriv.h
+++ b/crypto/hashpriv.h
@@ -21,12 +21,6 @@
 typedef struct QCryptoHashDriver QCryptoHashDriver;
 
 struct QCryptoHashDriver {
-    int (*hash_bytesv)(QCryptoHashAlgorithm alg,
-                       const struct iovec *iov,
-                       size_t niov,
-                       uint8_t **result,
-                       size_t *resultlen,
-                       Error **errp);
     QCryptoHash *(*hash_new)(QCryptoHashAlgorithm alg, Error **errp);
     int (*hash_update)(QCryptoHash *hash,
                        const struct iovec *iov,
-- 
2.34.1

[PATCH v4 15/15] hw/misc/aspeed_hace: Fix SG Accumulative hashing

[Alejandro Zeise]

Make the Aspeed HACE module use the new qcrypto accumulative hashing functions
when in scatter-gather accumulative mode. A hash context will maintain a
"running-hash" as each scatter-gather chunk is received.

Previously each scatter-gather "chunk" was cached
so the hash could be computed once the final chunk was received.
However, the cache was a shallow copy, so once the guest overwrote the
memory provided to HACE the final hash would not be correct.

Possibly related to: https://gitlab.com/qemu-project/qemu/-/issues/1121
Buglink: https://github.com/openbmc/qemu/issues/36

Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
---
 hw/misc/aspeed_hace.c         | 94 +++++++++++++++++++----------------
 include/hw/misc/aspeed_hace.h |  4 ++
 2 files changed, 54 insertions(+), 44 deletions(-)

diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
index c06c04ddc6..4247403d45 100644
--- a/hw/misc/aspeed_hace.c
+++ b/hw/misc/aspeed_hace.c
@@ -1,6 +1,7 @@
 /*
  * ASPEED Hash and Crypto Engine
  *
+ * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates
  * Copyright (C) 2021 IBM Corp.
  *
  * Joel Stanley <joel@jms.id.au>
@@ -151,50 +152,28 @@ static int reconstruct_iov(AspeedHACEState *s, struct iovec *iov, int id,
     return iov_count;
 }
 
-/**
- * Generate iov for accumulative mode.
- *
- * @param s             aspeed hace state object
- * @param iov           iov of the current request
- * @param id            index of the current iov
- * @param req_len       length of the current request
- *
- * @return count of iov
- */
-static int gen_acc_mode_iov(AspeedHACEState *s, struct iovec *iov, int id,
-                            hwaddr *req_len)
-{
-    uint32_t pad_offset;
-    uint32_t total_msg_len;
-    s->total_req_len += *req_len;
-
-    if (has_padding(s, &iov[id], *req_len, &total_msg_len, &pad_offset)) {
-        if (s->iov_count) {
-            return reconstruct_iov(s, iov, id, &pad_offset);
-        }
-
-        *req_len -= s->total_req_len - total_msg_len;
-        s->total_req_len = 0;
-        iov[id].iov_len = *req_len;
-    } else {
-        s->iov_cache[s->iov_count].iov_base = iov->iov_base;
-        s->iov_cache[s->iov_count].iov_len = *req_len;
-        ++s->iov_count;
-    }
-
-    return id + 1;
-}
-
 static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode,
                               bool acc_mode)
 {
     struct iovec iov[ASPEED_HACE_MAX_SG];
+    uint32_t total_msg_len;
+    uint32_t pad_offset;
     g_autofree uint8_t *digest_buf = NULL;
     size_t digest_len = 0;
-    int niov = 0;
+    bool sg_acc_mode_final_request = false;
     int i;
     void *haddr;
 
+    if (acc_mode && s->hash_ctx == NULL) {
+        s->hash_ctx = qcrypto_hash_new(algo, NULL);
+        if (s->hash_ctx == NULL) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: qcrypto failed to create hash context\n",
+                          __func__);
+            return;
+        }
+    }
+
     if (sg_mode) {
         uint32_t len = 0;
 
@@ -226,8 +205,15 @@ static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode,
             }
             iov[i].iov_base = haddr;
             if (acc_mode) {
-                niov = gen_acc_mode_iov(s, iov, i, &plen);
-
+                s->total_req_len += plen;
+
+                if (has_padding(s, &iov[i], plen, &total_msg_len, &pad_offset)) {
+                    /* Padding being present indicates the final request */
+                    sg_acc_mode_final_request = true;
+                    iov[i].iov_len = pad_offset;
+                } else {
+                    iov[i].iov_len = plen;
+                }
             } else {
                 iov[i].iov_len = plen;
             }
@@ -252,20 +238,35 @@ static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode,
              * required to check whether cache is empty. If no, we should
              * combine cached iov and the current iov.
              */
-            uint32_t total_msg_len;
-            uint32_t pad_offset;
             s->total_req_len += len;
             if (has_padding(s, iov, len, &total_msg_len, &pad_offset)) {
-                niov = reconstruct_iov(s, iov, 0, &pad_offset);
+                i = reconstruct_iov(s, iov, 0, &pad_offset);
             }
         }
     }
 
-    if (niov) {
-        i = niov;
-    }
+    if (acc_mode) {
+        if (qcrypto_hash_updatev(s->hash_ctx, iov, i, NULL) < 0) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: qcrypto hash update failed\n", __func__);
+            return;
+        }
+
+        if (sg_acc_mode_final_request) {
+            if (qcrypto_hash_finalize_bytes(s->hash_ctx, &digest_buf,
+                                            &digest_len, NULL)) {
+                qemu_log_mask(LOG_GUEST_ERROR,
+                              "%s: qcrypto failed to finalize hash\n", __func__);
+            }
 
-    if (qcrypto_hash_bytesv(algo, iov, i, &digest_buf, &digest_len, NULL) < 0) {
+            qcrypto_hash_free(s->hash_ctx);
+
+            s->hash_ctx = NULL;
+            s->iov_count = 0;
+            s->total_req_len = 0;
+        }
+    } else if (qcrypto_hash_bytesv(algo, iov, i, &digest_buf,
+                                   &digest_len, NULL) < 0) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: qcrypto failed\n", __func__);
         return;
     }
@@ -397,6 +398,11 @@ static void aspeed_hace_reset(DeviceState *dev)
 {
     struct AspeedHACEState *s = ASPEED_HACE(dev);
 
+    if (s->hash_ctx != NULL) {
+        qcrypto_hash_free(s->hash_ctx);
+        s->hash_ctx = NULL;
+    }
+
     memset(s->regs, 0, sizeof(s->regs));
     s->iov_count = 0;
     s->total_req_len = 0;
diff --git a/include/hw/misc/aspeed_hace.h b/include/hw/misc/aspeed_hace.h
index ecb1b67de8..4af9919195 100644
--- a/include/hw/misc/aspeed_hace.h
+++ b/include/hw/misc/aspeed_hace.h
@@ -1,6 +1,7 @@
 /*
  * ASPEED Hash and Crypto Engine
  *
+ * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates
  * Copyright (C) 2021 IBM Corp.
  *
  * SPDX-License-Identifier: GPL-2.0-or-later
@@ -10,6 +11,7 @@
 #define ASPEED_HACE_H
 
 #include "hw/sysbus.h"
+#include "crypto/hash.h"
 
 #define TYPE_ASPEED_HACE "aspeed.hace"
 #define TYPE_ASPEED_AST2400_HACE TYPE_ASPEED_HACE "-ast2400"
@@ -35,6 +37,8 @@ struct AspeedHACEState {
 
     MemoryRegion *dram_mr;
     AddressSpace dram_as;
+
+    QCryptoHash *hash_ctx;
 };
 
 
-- 
2.34.1

Re: [PATCH v4 00/15] hw/misc/aspeed_hace: Fix SG Accumulative Hash Calculations

[Philippe Mathieu-Daudé]

Hi Alejandro,

On 7/8/24 21:51, Alejandro Zeise wrote:
> 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.


> Alejandro Zeise (15):
>    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
>    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

>   13 files changed, 753 insertions(+), 283 deletions(-)

Even without the unit test this is still more than 700 LoC,
which seems a huge patchset to merge while we are in freeze
period. Do you expect this to be in the next v9.1.0 release?

Regards,

Phil.

RE: [PATCH v4 00/15] hw/misc/aspeed_hace: Fix SG Accumulative Hash Calculations

[Alejandro Zeise]

Hi Phil,

> This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
>
>
> Hi Alejandro,
>
> On 7/8/24 21:51, Alejandro Zeise wrote:
> > 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.
>
>
> > Alejandro Zeise (15):
> >    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
> >    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
>
> >   13 files changed, 753 insertions(+), 283 deletions(-)

> Even without the unit test this is still more than 700 LoC, which seems a huge patchset to merge while we are in freeze period. Do you expect this to be in the next v9.1.0 release?
>
> Regards,
>
>  Phil.

I do understand this involves of changes, and don't expect this to be in the next release.
I'm not quite familiar with the process regarding versioning (this is my first ever patch), but I do not see a need to rush these changes.

Thanks,
Alejandro

Re: [PATCH v4 00/15] hw/misc/aspeed_hace: Fix SG Accumulative Hash Calculations

[Daniel P. Berrangé]

On Wed, Aug 07, 2024 at 10:01:40PM +0200, Philippe Mathieu-Daudé wrote:
> Hi Alejandro,
> 
> On 7/8/24 21:51, Alejandro Zeise wrote:
> > 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.
> 
> 
> > Alejandro Zeise (15):
> >    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
> >    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
> 
> >   13 files changed, 753 insertions(+), 283 deletions(-)
> 
> Even without the unit test this is still more than 700 LoC,
> which seems a huge patchset to merge while we are in freeze
> period. Do you expect this to be in the next v9.1.0 release?

No, I'm not going to queue it for this release, I will put it
into my queue for 9.2.0

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 :|

Re: [PATCH v4 01/15] crypto: accumulative hashing API

[Daniel P. Berrangé]

On Wed, Aug 07, 2024 at 07:51:08PM +0000, Alejandro Zeise wrote:
> Changes the hash API to support accumulative hashing.
> Hash objects are created with "qcrypto_hash_new",
> updated with data with "qcrypto_hash_update", and
> the hash obtained with "qcrypto_hash_finalize".
> 
> These changes bring the hashing API more in line with the
> hmac API.
> 
> Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
> ---
>  crypto/hashpriv.h     |  13 +++++
>  include/crypto/hash.h | 119 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 132 insertions(+)
> 
> diff --git a/crypto/hashpriv.h b/crypto/hashpriv.h
> index cee26ccb47..02f17ee99f 100644
> --- a/crypto/hashpriv.h
> +++ b/crypto/hashpriv.h
> @@ -1,6 +1,7 @@
>  /*
>   * QEMU Crypto hash driver supports
>   *
> + * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates
>   * Copyright (c) 2017 HUAWEI TECHNOLOGIES CO., LTD.
>   *
>   * Authors:
> @@ -15,6 +16,8 @@
>  #ifndef QCRYPTO_HASHPRIV_H
>  #define QCRYPTO_HASHPRIV_H
>  
> +#include "crypto/hash.h"
> +
>  typedef struct QCryptoHashDriver QCryptoHashDriver;
>  
>  struct QCryptoHashDriver {
> @@ -24,6 +27,16 @@ struct QCryptoHashDriver {
>                         uint8_t **result,
>                         size_t *resultlen,
>                         Error **errp);
> +    QCryptoHash *(*hash_new)(QCryptoHashAlgorithm alg, Error **errp);
> +    int (*hash_update)(QCryptoHash *hash,
> +                       const struct iovec *iov,
> +                       size_t niov,
> +                       Error **errp);
> +    int (*hash_finalize)(QCryptoHash *hash,
> +                         uint8_t **result,
> +                         size_t *resultlen,
> +                         Error **errp);
> +    void (*hash_free)(QCryptoHash *hash);
>  };
>  
>  extern QCryptoHashDriver qcrypto_hash_lib_driver;
> diff --git a/include/crypto/hash.h b/include/crypto/hash.h
> index 54d87aa2a1..6d7222867e 100644
> --- a/include/crypto/hash.h
> +++ b/include/crypto/hash.h
> @@ -1,6 +1,7 @@
>  /*
>   * QEMU Crypto hash algorithms
>   *
> + * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates
>   * Copyright (c) 2015 Red Hat, Inc.
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -25,6 +26,13 @@
>  
>  /* See also "QCryptoHashAlgorithm" defined in qapi/crypto.json */
>  
> +typedef struct QCryptoHash QCryptoHash;
> +struct QCryptoHash {
> +    QCryptoHashAlgorithm alg;
> +    void *opaque;
> +    void *driver;
> +};
> +
>  /**
>   * qcrypto_hash_supports:
>   * @alg: the hash algorithm
> @@ -120,6 +128,117 @@ int qcrypto_hash_digestv(QCryptoHashAlgorithm alg,
>                           char **digest,
>                           Error **errp);
>  
> +/**
> + * qcrypto_hash_updatev:
> + * @hash: hash object from qcrypto_hash_new
> + * @iov: the array of memory regions to hash
> + * @niov: the length of @iov
> + * @errp: pointer to a NULL-initialized error object
> + *
> + * Updates the given hash object with all the memory regions
> + * present in @iov.
> + *
> + * Returns: 0 on success, non-zero on error

Minor point, this and all the other APIs should be saying
'or -1 on error' to follow QEMU's error reporting standards.


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 :|

Re: [PATCH v4 07/15] crypto/hash: Implement and use new hash API

[Daniel P. Berrangé]

On Wed, Aug 07, 2024 at 07:51:14PM +0000, Alejandro Zeise wrote:
> Changes the public hash API implementation to support accumulative hashing.
> 
> Implementations for the public functions are added to call the new
> driver functions that implement context creation, updating,
> finalization, and destruction.
> 
> Additionally changes the "shortcut" functions to use these 4 new core
> functions.
> 
> Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
> ---
>  crypto/hash.c | 163 +++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 128 insertions(+), 35 deletions(-)
> 
> diff --git a/crypto/hash.c b/crypto/hash.c
> index b0f8228bdc..f3c18cdd74 100644
> --- a/crypto/hash.c
> +++ b/crypto/hash.c
> @@ -1,6 +1,7 @@
>  /*
>   * QEMU Crypto hash algorithms
>   *
> + * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates
>   * Copyright (c) 2015 Red Hat, Inc.
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -45,23 +46,20 @@ int qcrypto_hash_bytesv(QCryptoHashAlgorithm alg,
>                          size_t *resultlen,
>                          Error **errp)
>  {
> -#ifdef CONFIG_AF_ALG
> -    int ret;
> -    /*
> -     * TODO:
> -     * Maybe we should treat some afalg errors as fatal
> -     */
> -    ret = qcrypto_hash_afalg_driver.hash_bytesv(alg, iov, niov,
> -                                                result, resultlen,
> -                                                NULL);
> -    if (ret == 0) {
> -        return ret;
> +    int fail;
> +    QCryptoHash *ctx = qcrypto_hash_new(alg, errp);
> +
> +    if (ctx) {
> +        fail = qcrypto_hash_updatev(ctx, iov, niov, errp) ||
> +               qcrypto_hash_finalize_bytes(ctx, result, resultlen, errp);
> +
> +        /* Ensure context is always freed regardless of error */
> +        qcrypto_hash_free(ctx);
> +    } else {
> +        fail = -1;
>      }
> -#endif

Generally I'd prefer immediate return on error, and you can
use g_autoptr to free the object too. With that you'll end
up without the extra 'fail' variable:

    g_autoptr(QCryptoHash) ctx = qcrypto_hash_new(alg, errp);

    if (!ctx) {
        return -1;
    }

    if (qcrypto_hash_updatev(ctx, iov, niov, errp) < 0 ||
        qcrypto_hash_finalize_bytes(ctx, result, resultlen, errp) < 0) {
	return -1;
    }

    return 0;

> @@ -77,30 +75,121 @@ int qcrypto_hash_bytes(QCryptoHashAlgorithm alg,
>      return qcrypto_hash_bytesv(alg, &iov, 1, result, resultlen, errp);
>  }
>  
> -static const char hex[] = "0123456789abcdef";
> -
> -int qcrypto_hash_digestv(QCryptoHashAlgorithm alg,
> +int qcrypto_hash_updatev(QCryptoHash *hash,
>                           const struct iovec *iov,
>                           size_t niov,
> -                         char **digest,
>                           Error **errp)
>  {
> +#ifdef CONFIG_AF_ALG
> +    return qcrypto_hash_afalg_driver.hash_update(hash, iov, niov, errp);
> +#else
> +    return qcrypto_hash_lib_driver.hash_update(hash, iov, niov, errp);
> +#endif /* CONFIG_AF_ALG */
> +}

This isn't quite the same as the old code. That would try afalg, and then
dynamically fallback to the userspace driver.

I think we need to deal with this all in the qcrypto_hash_new() method.
Attempt to create an AF_ALG driver instance via its 'hash_new' method,
and if that fails create the normal driver 'hash_new' method. We need
to record which we created in QCryptoHash struct, and then in the
update, finalize & free methods we need to call the corresponding
driver.

> +
> +int qcrypto_hash_update(QCryptoHash *hash,
> +                        const char *buf,
> +                        size_t len,
> +                        Error **errp)
> +{
> +    struct iovec iov = { .iov_base = (char *)buf, .iov_len = len };
> +
> +    return qcrypto_hash_updatev(hash, &iov, 1, errp);
> +}
> +
> +QCryptoHash *qcrypto_hash_new(QCryptoHashAlgorithm alg, Error **errp)
> +{

Here we should call

   if (!qcrypto_hash_supports(alg)) {
      error_setg(errp, "Unsupported hash algorithm %s",
                 QCryptoHashAlgorithm_lookup[alg]);
      return NULL;
   }

That way, we avoid repeating this check in every single
'hash_new' method impl

> +#ifdef CONFIG_AF_ALG
> +    return qcrypto_hash_afalg_driver.hash_new(alg, errp);
> +#else
> +    return qcrypto_hash_lib_driver.hash_new(alg, errp);
> +#endif /* CONFIG_AF_ALG */
> +}
> +
> +void qcrypto_hash_free(QCryptoHash *hash)
> +{
> +#ifdef CONFIG_AF_ALG
> +    qcrypto_hash_afalg_driver.hash_free(hash);
> +#else
> +    qcrypto_hash_lib_driver.hash_free(hash);
> +#endif /* CONFIG_AF_ALG */
> +}
> +
> +int qcrypto_hash_finalize_bytes(QCryptoHash *hash,
> +                                uint8_t **result,
> +                                size_t *result_len,
> +                                Error **errp)
> +{
> +#ifdef CONFIG_AF_ALG
> +    return qcrypto_hash_afalg_driver.hash_finalize(hash, result, result_len,
> +                                                   errp);
> +#else
> +    return qcrypto_hash_lib_driver.hash_finalize(hash, result, result_len, errp);
> +#endif /* CONFIG_AF_ALG */
> +}
> +
> +static const char hex[] = "0123456789abcdef";
> +
> +int qcrypto_hash_finalize_digest(QCryptoHash *hash,
> +                                 char **digest,
> +                                 Error **errp)
> +{
> +    int ret;
>      uint8_t *result = NULL;

declare this

  g_autofree uint8_t *result = NULL;

then you can drop the later g_free(result)

>      size_t resultlen = 0;
>      size_t i;
>  
> -    if (qcrypto_hash_bytesv(alg, iov, niov, &result, &resultlen, errp) < 0) {
> -        return -1;
> +    ret = qcrypto_hash_finalize_bytes(hash, &result, &resultlen, errp);
> +    if (ret == 0) {
> +        *digest = g_new0(char, (resultlen * 2) + 1);
> +        for (i = 0 ; i < resultlen ; i++) {
> +            (*digest)[(i * 2)] = hex[(result[i] >> 4) & 0xf];
> +            (*digest)[(i * 2) + 1] = hex[result[i] & 0xf];
> +        }
> +        (*digest)[resultlen * 2] = '\0';
> +        g_free(result);
> +    }
> +
> +    return ret;
> +}
> +
> +int qcrypto_hash_finalize_base64(QCryptoHash *hash,
> +                                 char **base64,
> +                                 Error **errp)
> +{
> +    int ret;
> +    uint8_t *result = NULL;

g_autofree for this too

> +    size_t resultlen = 0;
> +
> +    ret = qcrypto_hash_finalize_bytes(hash, &result, &resultlen, errp);
> +    if (ret == 0) {
> +        *base64 = g_base64_encode(result, resultlen);
> +        g_free(result);
>      }
>  
> -    *digest = g_new0(char, (resultlen * 2) + 1);
> -    for (i = 0 ; i < resultlen ; i++) {
> -        (*digest)[(i * 2)] = hex[(result[i] >> 4) & 0xf];
> -        (*digest)[(i * 2) + 1] = hex[result[i] & 0xf];
> +    return ret;
> +}
> +
> +int qcrypto_hash_digestv(QCryptoHashAlgorithm alg,
> +                         const struct iovec *iov,
> +                         size_t niov,
> +                         char **digest,
> +                         Error **errp)
> +{
> +    bool fail;
> +    QCryptoHash *ctx = qcrypto_hash_new(alg, errp);
> +
> +    if (ctx) {
> +        fail = qcrypto_hash_updatev(ctx, iov, niov, errp) ||
> +               qcrypto_hash_finalize_digest(ctx, digest, errp);
> +
> +        /* Ensure context is always freed regardless of error */
> +        qcrypto_hash_free(ctx);
> +    } else {
> +        fail = false;
>      }
> -    (*digest)[resultlen * 2] = '\0';
> -    g_free(result);
> -    return 0;
> +
> +    return fail;
>  }

Same comment about re-arranging the code that I mentioned higher
up against qcrypto_hash_bytesv

>  
>  int qcrypto_hash_digest(QCryptoHashAlgorithm alg,
> @@ -120,16 +209,20 @@ int qcrypto_hash_base64v(QCryptoHashAlgorithm alg,
>                           char **base64,
>                           Error **errp)
>  {
> -    uint8_t *result = NULL;
> -    size_t resultlen = 0;
> +    bool fail;
> +    QCryptoHash *ctx = qcrypto_hash_new(alg, errp);
> +
> +    if (ctx) {
> +        fail = qcrypto_hash_updatev(ctx, iov, niov, errp) ||
> +               qcrypto_hash_finalize_base64(ctx, base64, errp);
>  
> -    if (qcrypto_hash_bytesv(alg, iov, niov, &result, &resultlen, errp) < 0) {
> -        return -1;
> +        /* Ensure context is always freed regardless of error */
> +        qcrypto_hash_free(ctx);
> +    } else {
> +        fail = 1;
>      }
>  
> -    *base64 = g_base64_encode(result, resultlen);
> -    g_free(result);
> -    return 0;
> +    return fail;
>  }

And same comment again.

>  
>  int qcrypto_hash_base64(QCryptoHashAlgorithm alg,
> -- 
> 2.34.1
> 

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 :|

Re: [PATCH v4 02/15] crypto/hash-glib: Implement new hash API

[Daniel P. Berrangé]

On Wed, Aug 07, 2024 at 07:51:09PM +0000, Alejandro Zeise wrote:
> Implements the new hashing API in the GLib hash driver.
> Supports creating/destroying a context, updating the context
> with input data and obtaining an output hash.
> 
> Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
> ---
>  crypto/hash-glib.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
> 
> diff --git a/crypto/hash-glib.c b/crypto/hash-glib.c
> index 82de9db705..9f4490762a 100644
> --- a/crypto/hash-glib.c
> +++ b/crypto/hash-glib.c
> @@ -1,6 +1,7 @@
>  /*
>   * QEMU Crypto hash algorithms
>   *
> + * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates
>   * Copyright (c) 2016 Red Hat, Inc.
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -95,6 +96,81 @@ qcrypto_glib_hash_bytesv(QCryptoHashAlgorithm alg,
>  }
>  
>  
> +static
> +QCryptoHash *qcrypto_glib_hash_new(QCryptoHashAlgorithm alg,
> +                                   Error **errp)
> +{
> +    QCryptoHash *hash = NULL;
> +
> +    if (!qcrypto_hash_supports(alg)) {
> +        error_setg(errp,
> +                   "Unknown hash algorithm %d",
> +                   alg);

This check should be removed from all the drivers, since we
can do it once in qcrypto_hash_new.

> +    } else {
> +        hash = g_new(QCryptoHash, 1);
> +        hash->alg = alg;
> +        hash->opaque = g_checksum_new(qcrypto_hash_alg_map[alg]);
> +    }
> +
> +    return hash;
> +}
> +
> +static
> +void qcrypto_glib_hash_free(QCryptoHash *hash)
> +{
> +    if (hash->opaque) {
> +        g_checksum_free((GChecksum *) hash->opaque);

The cast to "(GChecksum *)" is not required since 'opaque' is 'void *'
which auto-casts to anything in C, and we don't need to support C++
compilers.

> +    }
> +
> +    g_free(hash);
> +}
> +
> +
> +static
> +int qcrypto_glib_hash_update(QCryptoHash *hash,
> +                             const struct iovec *iov,
> +                             size_t niov,
> +                             Error **errp)
> +{
> +    GChecksum *ctx = hash->opaque;
> +
> +    for (int i = 0; i < niov; i++) {
> +        g_checksum_update(ctx, iov[i].iov_base, iov[i].iov_len);
> +    }
> +
> +    return 0;
> +}
> +
> +static
> +int qcrypto_glib_hash_finalize(QCryptoHash *hash,
> +                               uint8_t **result,
> +                               size_t *result_len,
> +                               Error **errp)
> +{
> +    int ret;
> +    GChecksum *ctx = hash->opaque;
> +
> +    ret = g_checksum_type_get_length(qcrypto_hash_alg_map[hash->alg]);
> +    if (ret < 0) {
> +        error_setg(errp, "%s",
> +                   "Unable to get hash length");
> +        *result_len = 0;
> +        ret = -1;

Just do an immediate 'return -1' here, and avoid the
'} else {' clause.

> +    } else {
> +        *result_len = ret;
> +        *result = g_new(uint8_t, *result_len);
> +
> +        g_checksum_get_digest(ctx, *result, result_len);
> +        ret = 0;
> +    }
> +
> +    return ret;
> +}
> +
>  QCryptoHashDriver qcrypto_hash_lib_driver = {
>      .hash_bytesv = qcrypto_glib_hash_bytesv,
> +    .hash_new      = qcrypto_glib_hash_new,
> +    .hash_update   = qcrypto_glib_hash_update,
> +    .hash_finalize = qcrypto_glib_hash_finalize,
> +    .hash_free     = qcrypto_glib_hash_free,
>  };
> -- 
> 2.34.1
> 

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 :|

Re: [PATCH v4 03/15] crypto/hash-gcrypt: Implement new hash API

[Daniel P. Berrangé]

On Wed, Aug 07, 2024 at 07:51:10PM +0000, Alejandro Zeise wrote:
> Implements the new hashing API in the gcrypt hash driver.
> Supports creating/destroying a context, updating the context
> with input data and obtaining an output hash.
> 
> Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
> ---
>  crypto/hash-gcrypt.c | 79 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
> 
> diff --git a/crypto/hash-gcrypt.c b/crypto/hash-gcrypt.c
> index 829e48258d..e05511cafa 100644
> --- a/crypto/hash-gcrypt.c
> +++ b/crypto/hash-gcrypt.c
> @@ -1,6 +1,7 @@
>  /*
>   * QEMU Crypto hash algorithms
>   *
> + * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates
>   * Copyright (c) 2016 Red Hat, Inc.
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -110,7 +111,85 @@ qcrypto_gcrypt_hash_bytesv(QCryptoHashAlgorithm alg,
>      return -1;
>  }
>  
> +static
> +QCryptoHash *qcrypto_gcrypt_hash_new(QCryptoHashAlgorithm alg, Error **errp)
> +{
> +    QCryptoHash *hash = NULL;
> +
> +    if (!qcrypto_hash_supports(alg)) {
> +        error_setg(errp,
> +                   "Unknown hash algorithm %d",
> +                   alg);
> +    } else {
> +        hash = g_new(QCryptoHash, 1);
> +        hash->alg = alg;
> +        hash->opaque = g_new(gcry_md_hd_t, 1);
> +
> +        gcry_md_open((gcry_md_hd_t *) hash->opaque, qcrypto_hash_alg_map[alg], 0);
> +    }
> +
> +    return hash;
> +}
> +
> +static
> +void qcrypto_gcrypt_hash_free(QCryptoHash *hash)
> +{
> +    gcry_md_hd_t *ctx = hash->opaque;
> +
> +    if (ctx) {
> +        gcry_md_close(*ctx);
> +        g_free(ctx);
> +    }
> +
> +    g_free(hash);
> +}
> +
> +
> +static
> +int qcrypto_gcrypt_hash_update(QCryptoHash *hash,
> +                               const struct iovec *iov,
> +                               size_t niov,
> +                               Error **errp)
> +{
> +    gcry_md_hd_t *ctx = hash->opaque;
> +
> +    for (int i = 0; i < niov; i++) {
> +        gcry_md_write(*ctx, iov[i].iov_base, iov[i].iov_len);
> +    }
> +
> +    return 0;
> +}
> +
> +static
> +int qcrypto_gcrypt_hash_finalize(QCryptoHash *hash,
> +                                 uint8_t **result,
> +                                 size_t *result_len,
> +                                 Error **errp)
> +{
> +    unsigned char *digest;
> +    int ret = 0;
> +    gcry_md_hd_t *ctx = hash->opaque;
> +
> +    *result_len = gcry_md_get_algo_dlen(qcrypto_hash_alg_map[hash->alg]);
> +    if (*result_len == 0) {
> +        error_setg(errp, "%s",
> +                   "Unable to get hash length");
> +        ret = -1;

Same note about doing an immediate 'return -1' in error paths,
to avoid extra else clauses.

> +    } else {
> +        *result = g_new(uint8_t, *result_len);
> +
> +        /* Digest is freed by gcry_md_close(), copy it */
> +        digest = gcry_md_read(*ctx, 0);
> +        memcpy(*result, digest, *result_len);
> +    }
> +
> +    return ret;
> +}
>  
>  QCryptoHashDriver qcrypto_hash_lib_driver = {
>      .hash_bytesv = qcrypto_gcrypt_hash_bytesv,
> +    .hash_new      = qcrypto_gcrypt_hash_new,
> +    .hash_update   = qcrypto_gcrypt_hash_update,
> +    .hash_finalize = qcrypto_gcrypt_hash_finalize,
> +    .hash_free     = qcrypto_gcrypt_hash_free,
>  };
> -- 
> 2.34.1
> 

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 :|

Re: [PATCH v4 01/15] crypto: accumulative hashing API

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Aug 07, 2024 at 07:51:08PM +0000, Alejandro Zeise wrote:
>> Changes the hash API to support accumulative hashing.
>> Hash objects are created with "qcrypto_hash_new",
>> updated with data with "qcrypto_hash_update", and
>> the hash obtained with "qcrypto_hash_finalize".
>> 
>> These changes bring the hashing API more in line with the
>> hmac API.
>> 
>> Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>

[...]

>> diff --git a/include/crypto/hash.h b/include/crypto/hash.h
>> index 54d87aa2a1..6d7222867e 100644
>> --- a/include/crypto/hash.h
>> +++ b/include/crypto/hash.h
>> @@ -1,6 +1,7 @@
>>  /*
>>   * QEMU Crypto hash algorithms
>>   *
>> + * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates
>>   * Copyright (c) 2015 Red Hat, Inc.
>>   *
>>   * This library is free software; you can redistribute it and/or
>> @@ -25,6 +26,13 @@
>>  
>>  /* See also "QCryptoHashAlgorithm" defined in qapi/crypto.json */
>>  
>> +typedef struct QCryptoHash QCryptoHash;
>> +struct QCryptoHash {
>> +    QCryptoHashAlgorithm alg;
>> +    void *opaque;
>> +    void *driver;
>> +};
>> +
>>  /**
>>   * qcrypto_hash_supports:
>>   * @alg: the hash algorithm
>> @@ -120,6 +128,117 @@ int qcrypto_hash_digestv(QCryptoHashAlgorithm alg,
>>                           char **digest,
>>                           Error **errp);
>>  
>> +/**
>> + * qcrypto_hash_updatev:
>> + * @hash: hash object from qcrypto_hash_new
>> + * @iov: the array of memory regions to hash
>> + * @niov: the length of @iov
>> + * @errp: pointer to a NULL-initialized error object
>> + *
>> + * Updates the given hash object with all the memory regions
>> + * present in @iov.
>> + *
>> + * Returns: 0 on success, non-zero on error
>
> Minor point, this and all the other APIs should be saying
> 'or -1 on error' to follow QEMU's error reporting standards.

Specifically, qapi/error.h:

 * - Whenever practical, also return a value that indicates success /
 *   failure.  This can make the error checking more concise, and can
 *   avoid useless error object creation and destruction.  Note that
 *   we still have many functions returning void.  We recommend
 *   • bool-valued functions return true on success / false on failure,
 *   • pointer-valued functions return non-null / null pointer, and
 *   • integer-valued functions return non-negative / negative.

Re: [PATCH v4 03/15] crypto/hash-gcrypt: Implement new hash API

[Daniel P. Berrangé]

On Wed, Aug 07, 2024 at 07:51:10PM +0000, Alejandro Zeise wrote:
> Implements the new hashing API in the gcrypt hash driver.
> Supports creating/destroying a context, updating the context
> with input data and obtaining an output hash.
> 
> Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
> ---
>  crypto/hash-gcrypt.c | 79 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
> 
> diff --git a/crypto/hash-gcrypt.c b/crypto/hash-gcrypt.c
> index 829e48258d..e05511cafa 100644
> --- a/crypto/hash-gcrypt.c
> +++ b/crypto/hash-gcrypt.c
> @@ -1,6 +1,7 @@
>  /*
>   * QEMU Crypto hash algorithms
>   *
> + * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates
>   * Copyright (c) 2016 Red Hat, Inc.
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -110,7 +111,85 @@ qcrypto_gcrypt_hash_bytesv(QCryptoHashAlgorithm alg,
>      return -1;
>  }
>  
> +static
> +QCryptoHash *qcrypto_gcrypt_hash_new(QCryptoHashAlgorithm alg, Error **errp)
> +{
> +    QCryptoHash *hash = NULL;
> +
> +    if (!qcrypto_hash_supports(alg)) {
> +        error_setg(errp,
> +                   "Unknown hash algorithm %d",
> +                   alg);
> +    } else {
> +        hash = g_new(QCryptoHash, 1);
> +        hash->alg = alg;
> +        hash->opaque = g_new(gcry_md_hd_t, 1);
> +
> +        gcry_md_open((gcry_md_hd_t *) hash->opaque, qcrypto_hash_alg_map[alg], 0);
> +    }
> +
> +    return hash;
> +}
> +
> +static
> +void qcrypto_gcrypt_hash_free(QCryptoHash *hash)
> +{
> +    gcry_md_hd_t *ctx = hash->opaque;
> +
> +    if (ctx) {
> +        gcry_md_close(*ctx);
> +        g_free(ctx);
> +    }
> +
> +    g_free(hash);
> +}
> +
> +
> +static
> +int qcrypto_gcrypt_hash_update(QCryptoHash *hash,
> +                               const struct iovec *iov,
> +                               size_t niov,
> +                               Error **errp)
> +{
> +    gcry_md_hd_t *ctx = hash->opaque;
> +
> +    for (int i = 0; i < niov; i++) {
> +        gcry_md_write(*ctx, iov[i].iov_base, iov[i].iov_len);

  int ret = gcry_md_write(*ctx, iov[i].iov_base, iov[i].iov_len);
  if (ret != 0) {
      error_setg(....)
      return -1;
  }


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 :|

Re: [PATCH v4 04/15] crypto/hash-gnutls: Implement new hash API

[Daniel P. Berrangé]

On Wed, Aug 07, 2024 at 07:51:11PM +0000, Alejandro Zeise wrote:
> Implements the new hashing API in the gnutls hash driver.
> Supports creating/destroying a context, updating the context
> with input data and obtaining an output hash.
> 
> Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
> ---
>  crypto/hash-gnutls.c | 73 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
> 
> diff --git a/crypto/hash-gnutls.c b/crypto/hash-gnutls.c
> index 17911ac5d1..15fc630a11 100644
> --- a/crypto/hash-gnutls.c
> +++ b/crypto/hash-gnutls.c
> @@ -1,6 +1,7 @@
>  /*
>   * QEMU Crypto hash algorithms
>   *
> + * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates
>   * Copyright (c) 2021 Red Hat, Inc.
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -98,7 +99,79 @@ qcrypto_gnutls_hash_bytesv(QCryptoHashAlgorithm alg,
>      return 0;
>  }
>  
> +static
> +QCryptoHash *qcrypto_gnutls_hash_new(QCryptoHashAlgorithm alg, Error **errp)
> +{
> +    QCryptoHash *hash = NULL;
> +
> +    if (!qcrypto_hash_supports(alg)) {
> +        error_setg(errp,
> +                   "Unknown hash algorithm %d",
> +                   alg);
> +    } else {
> +        hash = g_new(QCryptoHash, 1);
> +        hash->alg = alg;
> +        hash->opaque = g_new(gnutls_hash_hd_t, 1);
> +
> +        gnutls_hash_init(hash->opaque, qcrypto_hash_alg_map[alg]);

  int ret = gnutls_hash_init(...)
  if (ret < 0) {
      error_setg(errp, ....)
      g_free(hash->opaque);
      g_free(hash);
      return NULL;
  }

> +    }
> +
> +    return hash;
> +}
> +
> +static
> +void qcrypto_gnutls_hash_free(QCryptoHash *hash)
> +{
> +    gnutls_hash_hd_t *ctx = hash->opaque;
> +
> +    g_free(ctx);
> +    g_free(hash);
> +}
> +
> +
> +static
> +int qcrypto_gnutls_hash_update(QCryptoHash *hash,
> +                               const struct iovec *iov,
> +                               size_t niov,
> +                               Error **errp)
> +{
> +    int fail = 0;
> +    gnutls_hash_hd_t *ctx = hash->opaque;
> +
> +    for (int i = 0; i < niov; i++) {
> +        fail = gnutls_hash(*ctx, iov[i].iov_base, iov[i].iov_len) || fail;

Needs to report in 'errp'  when failure happens & return immediately. eg

   int ret = gnutls_hash(*ctx, iov[i].iov_base, iov[i].iov_len);
   if (ret != 0) {
       error_setg(errp, ....)
       return -1;
   }

> +    }
> +
> +    return fail;

Just 'return 0'

> +}
> +
> +static
> +int qcrypto_gnutls_hash_finalize(QCryptoHash *hash,
> +                                 uint8_t **result,
> +                                 size_t *result_len,
> +                                 Error **errp)
> +{
> +    int ret = 0;
> +    gnutls_hash_hd_t *ctx = hash->opaque;
> +
> +    *result_len = gnutls_hash_get_len(qcrypto_hash_alg_map[hash->alg]);
> +    if (*result_len == 0) {
> +        error_setg(errp, "%s",
> +                   "Unable to get hash length");
> +        ret = -1;
> +    } else {
> +        *result = g_new(uint8_t, *result_len);
> +
> +        gnutls_hash_deinit(*ctx, *result);

We should use  gnutls_hash_output() here instead, and call
gnutls_hash_deinit() in the qcrypto_gnutls_hash_free()
method.  That ensures all memory is freed if the user
never calls qcrypto_hash_finalize()

> +    }
> +
> +    return ret;
> +}
>  
>  QCryptoHashDriver qcrypto_hash_lib_driver = {
>      .hash_bytesv = qcrypto_gnutls_hash_bytesv,
> +    .hash_new      = qcrypto_gnutls_hash_new,
> +    .hash_update   = qcrypto_gnutls_hash_update,
> +    .hash_finalize = qcrypto_gnutls_hash_finalize,
> +    .hash_free     = qcrypto_gnutls_hash_free,
>  };
> -- 
> 2.34.1
> 

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 :|

Re: [PATCH v4 03/15] crypto/hash-gcrypt: Implement new hash API

[Daniel P. Berrangé]

On Wed, Aug 07, 2024 at 07:51:10PM +0000, Alejandro Zeise wrote:
> Implements the new hashing API in the gcrypt hash driver.
> Supports creating/destroying a context, updating the context
> with input data and obtaining an output hash.
> 
> Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
> ---
>  crypto/hash-gcrypt.c | 79 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
> 
> diff --git a/crypto/hash-gcrypt.c b/crypto/hash-gcrypt.c
> index 829e48258d..e05511cafa 100644
> --- a/crypto/hash-gcrypt.c
> +++ b/crypto/hash-gcrypt.c
> @@ -1,6 +1,7 @@
>  /*
>   * QEMU Crypto hash algorithms
>   *
> + * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates
>   * Copyright (c) 2016 Red Hat, Inc.
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -110,7 +111,85 @@ qcrypto_gcrypt_hash_bytesv(QCryptoHashAlgorithm alg,
>      return -1;
>  }
>  
> +static
> +QCryptoHash *qcrypto_gcrypt_hash_new(QCryptoHashAlgorithm alg, Error **errp)
> +{
> +    QCryptoHash *hash = NULL;
> +
> +    if (!qcrypto_hash_supports(alg)) {
> +        error_setg(errp,
> +                   "Unknown hash algorithm %d",
> +                   alg);
> +    } else {
> +        hash = g_new(QCryptoHash, 1);
> +        hash->alg = alg;
> +        hash->opaque = g_new(gcry_md_hd_t, 1);
> +
> +        gcry_md_open((gcry_md_hd_t *) hash->opaque, qcrypto_hash_alg_map[alg], 0);

Need to check for error from gcry_md_open and report it, and
free hash & hash->opaque

> +    }
> +
> +    return hash;
> +}

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 :|

Re: [PATCH v4 06/15] crypto/hash-afalg: Implement new hash API

[Daniel P. Berrangé]

On Wed, Aug 07, 2024 at 07:51:13PM +0000, Alejandro Zeise wrote:
> Updates the afalg hash driver to support the new accumulative
> hashing changes as part of the patch series.
> 
> Implements opening/closing of contexts, updating hash data
> and finalizing the hash digest.
> 
> In order to support the update function, a flag needs to be passed
> to the kernel via the socket send call (MSG_MORE) to notify it that more
> data is to be expected to calculate the hash correctly.
> As a result, a new function was added to the iov helper utils to allow
> passing a flag to the socket send call.
> 
> Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
> ---
>  crypto/hash-afalg.c | 126 ++++++++++++++++++++++++++++++++++++++++++++
>  include/qemu/iov.h  |  26 +++++++++
>  util/iov.c          |  22 +++++---
>  3 files changed, 167 insertions(+), 7 deletions(-)
> 
> diff --git a/crypto/hash-afalg.c b/crypto/hash-afalg.c
> index 3ebea39292..9548c04933 100644
> --- a/crypto/hash-afalg.c
> +++ b/crypto/hash-afalg.c
> @@ -1,6 +1,7 @@
>  /*
>   * QEMU Crypto af_alg-backend hash/hmac support
>   *
> + * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates
>   * Copyright (c) 2017 HUAWEI TECHNOLOGIES CO., LTD.
>   *
>   * Authors:
> @@ -113,6 +114,127 @@ qcrypto_afalg_hmac_ctx_new(QCryptoHashAlgorithm alg,
>      return qcrypto_afalg_hash_hmac_ctx_new(alg, key, nkey, true, errp);
>  }
>  
> +static
> +QCryptoHash *qcrypto_afalg_hash_new(QCryptoHashAlgorithm alg, Error **errp)
> +{
> +    /* Check if hash algorithm is supported */
> +    char *alg_name = qcrypto_afalg_hash_format_name(alg, false, NULL);
> +    QCryptoHash *hash = NULL;
> +
> +    if (alg_name == NULL) {
> +        error_setg(errp,
> +                   "Unknown hash algorithm %d",
> +                   alg);
> +    } else {
> +        hash = g_new(QCryptoHash, 1);
> +        hash->alg = alg;
> +        hash->opaque = qcrypto_afalg_hash_ctx_new(alg, errp);

This can return NULL, in which case we need to 'free(hash)' and
return NULL from this method.

> +    }
> +
> +    return hash;
> +}
> +
> +static
> +void qcrypto_afalg_hash_free(QCryptoHash *hash)
> +{
> +    QCryptoAFAlg *ctx = hash->opaque;
> +
> +    if (ctx) {
> +        qcrypto_afalg_comm_free(ctx);
> +    }
> +
> +    g_free(hash);
> +}
> +
> +/**
> + * Send data to the kernel's crypto core.
> + *
> + * The more_data parameter is used to notify the crypto engine
> + * that this is an "update" operation, and that more data will
> + * be provided to calculate the final hash.
> + */
> +static
> +int qcrypto_afalg_send_to_kernel(QCryptoAFAlg *afalg,
> +                                 const struct iovec *iov,
> +                                 size_t niov,
> +                                 bool more_data,
> +                                 Error **errp)
> +{
> +    int ret = 0;
> +    int flags = (more_data ? MSG_MORE : 0);
> +
> +    /* send data to kernel's crypto core */
> +    ret = iov_send_recv_with_flags(afalg->opfd, flags, iov, niov,
> +                                   0, iov_size(iov, niov), true);
> +    if (ret < 0) {
> +        error_setg_errno(errp, errno, "Send data to afalg-core failed");
> +        ret = -1;
> +    } else {
> +        /* No error, so return 0 */
> +        ret = 0;
> +    }
> +
> +    return ret;
> +}
> +
> +static
> +int qcrypto_afalg_recv_from_kernel(QCryptoAFAlg *afalg,
> +                                   QCryptoHashAlgorithm alg,
> +                                   uint8_t **result,
> +                                   size_t *result_len,
> +                                   Error **errp)
> +{
> +    struct iovec outv;
> +    int ret = 0;
> +    const int expected_len = qcrypto_hash_digest_len(alg);
> +
> +    if (*result_len == 0) {
> +        *result_len = expected_len;
> +        *result = g_new0(uint8_t, *result_len);
> +    } else if (*result_len != expected_len) {
> +        error_setg(errp,
> +                   "Result buffer size %zu is not match hash %d",
> +                   *result_len, expected_len);
> +        ret = -1;
> +    }

Check the error condition first, and do an immediate
'return -1', avoiding the following conditional

> +
> +    if (ret == 0) {
> +        /* hash && get result */
> +        outv.iov_base = *result;
> +        outv.iov_len = *result_len;
> +        ret = iov_send_recv(afalg->opfd, &outv, 1,
> +                            0, iov_size(&outv, 1), false);
> +        if (ret < 0) {
> +            error_setg_errno(errp, errno, "Recv result from afalg-core failed");
> +            ret = -1;

Again do an imediate 'return -1'.

> +        } else {
> +            ret = 0;
> +        }
> +    }
> +
> +    return ret;
> +}
> +
> +static
> +int qcrypto_afalg_hash_update(QCryptoHash *hash,
> +                              const struct iovec *iov,
> +                              size_t niov,
> +                              Error **errp)
> +{
> +    return qcrypto_afalg_send_to_kernel((QCryptoAFAlg *) hash->opaque,
> +                                        iov, niov, true, errp);
> +}
> +
> +static
> +int qcrypto_afalg_hash_finalize(QCryptoHash *hash,
> +                                 uint8_t **result,
> +                                 size_t *result_len,
> +                                 Error **errp)
> +{
> +    return qcrypto_afalg_recv_from_kernel((QCryptoAFAlg *) hash->opaque,
> +                                          hash->alg, result, result_len, errp);
> +}
> +
>  static int
>  qcrypto_afalg_hash_hmac_bytesv(QCryptoAFAlg *hmac,
>                                 QCryptoHashAlgorithm alg,
> @@ -205,6 +327,10 @@ static void qcrypto_afalg_hmac_ctx_free(QCryptoHmac *hmac)
>  
>  QCryptoHashDriver qcrypto_hash_afalg_driver = {
>      .hash_bytesv = qcrypto_afalg_hash_bytesv,
> +    .hash_new      = qcrypto_afalg_hash_new,
> +    .hash_free     = qcrypto_afalg_hash_free,
> +    .hash_update   = qcrypto_afalg_hash_update,
> +    .hash_finalize = qcrypto_afalg_hash_finalize
>  };
>  
>  QCryptoHmacDriver qcrypto_hmac_afalg_driver = {
> diff --git a/include/qemu/iov.h b/include/qemu/iov.h
> index 63a1c01965..43884cdd64 100644
> --- a/include/qemu/iov.h
> +++ b/include/qemu/iov.h

snip

> diff --git a/util/iov.c b/util/iov.c
> index 7e73948f5e..5644e0b73c 100644
> --- a/util/iov.c
> +++ b/util/iov.c

snip

Could you split the iov.h/.c changes into  a separate patch that comes
before this one, since they're under a separate maintainer, and thus
desirable to split out for visibility.


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 :|

Re: [PATCH v4 08/15] tests/unit/test-crypto-hash: accumulative hashing

[Daniel P. Berrangé]

On Wed, Aug 07, 2024 at 07:51:15PM +0000, Alejandro Zeise wrote:
> Added an accumulative hashing test. Checks for functionality of
> the new hash create, update, finalize and free functions.
> 
> Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
> ---
>  tests/unit/test-crypto-hash.c | 48 +++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/tests/unit/test-crypto-hash.c b/tests/unit/test-crypto-hash.c
> index 1f4abb822b..2bf9bcb6a0 100644
> --- a/tests/unit/test-crypto-hash.c
> +++ b/tests/unit/test-crypto-hash.c
> @@ -1,6 +1,7 @@
>  /*
>   * QEMU Crypto hash algorithms
>   *
> + * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates
>   * Copyright (c) 2015 Red Hat, Inc.
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -241,6 +242,52 @@ static void test_hash_base64(void)
>      }
>  }
>  
> +static void test_hash_accumulate(void)
> +{
> +    QCryptoHash *hash;

Move this inside the for() and use 'g_autoptr(QCryptoHash) hash = NULL'

> +    size_t i;
> +
> +    for (i = 0; i < G_N_ELEMENTS(expected_outputs) ; i++) {
> +        struct iovec iov[3] = {

Don't need to declare it '[3]' just '[]' as, then....

> +            { .iov_base = (char *)INPUT_TEXT1, .iov_len = strlen(INPUT_TEXT1) },
> +            { .iov_base = (char *)INPUT_TEXT2, .iov_len = strlen(INPUT_TEXT2) },
> +            { .iov_base = (char *)INPUT_TEXT3, .iov_len = strlen(INPUT_TEXT3) },
> +        };
> +        uint8_t *result = NULL;

g_autofree uint8_t *result = NULL;

> +        size_t resultlen = 0;
> +        int ret;
> +        size_t j;
> +
> +        if (!qcrypto_hash_supports(i)) {
> +            continue;
> +        }
> +
> +        hash = qcrypto_hash_new(i, &error_fatal);
> +        g_assert(hash != NULL);
> +
> +        /* Add each iovec to the hash context separately */
> +        for (j = 0; j < 3; j++) {

...instead of 'j < 3' use  'j < G_N_ELEMENTS(iov)'

> +            ret = qcrypto_hash_updatev(hash,
> +                                      &iov[j], 1,
> +                                      &error_fatal);
> +
> +            g_assert(ret == 0);
> +        }
> +
> +        ret = qcrypto_hash_finalize_bytes(hash, &result, &resultlen,
> +                                          &error_fatal);
> +
> +        g_assert(ret == 0);
> +        g_assert(resultlen == expected_lens[i]);
> +        for (j = 0; j < resultlen; j++) {
> +            g_assert(expected_outputs[i][j * 2] == hex[(result[j] >> 4) & 0xf]);
> +            g_assert(expected_outputs[i][j * 2 + 1] == hex[result[j] & 0xf]);
> +        }
> +        g_free(result);
> +        qcrypto_hash_free(hash);

Drop these two frees.

> +    }
> +}
> +
>  int main(int argc, char **argv)
>  {
>      int ret = qcrypto_init(&error_fatal);
> @@ -252,5 +299,6 @@ int main(int argc, char **argv)
>      g_test_add_func("/crypto/hash/prealloc", test_hash_prealloc);
>      g_test_add_func("/crypto/hash/digest", test_hash_digest);
>      g_test_add_func("/crypto/hash/base64", test_hash_base64);
> +    g_test_add_func("/crypto/hash/accumulate", test_hash_accumulate);
>      return g_test_run();
>  }
> -- 
> 2.34.1
> 

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 :|

Re: [PATCH v4 09/15] crypto/hash-glib: Remove old hash API functions

[Daniel P. Berrangé]

On Wed, Aug 07, 2024 at 07:51:16PM +0000, Alejandro Zeise wrote:
> Removes old hash implemention in the GLib hash driver.
> 
> Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
> ---
>  crypto/hash-glib.c | 53 ----------------------------------------------
>  1 file changed, 53 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 :|

Re: [PATCH v4 10/15] crypto/hash-gcrypt: Remove old hash API functions

[Daniel P. Berrangé]

On Wed, Aug 07, 2024 at 07:51:17PM +0000, Alejandro Zeise wrote:
> Removes old hash implemention in the gcrypt hash driver.
> 
> Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
> ---
>  crypto/hash-gcrypt.c | 67 --------------------------------------------
>  1 file changed, 67 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 :|

Re: [PATCH v4 11/15] crypto/hash-gnutls: Remove old hash API functions

[Daniel P. Berrangé]

On Wed, Aug 07, 2024 at 07:51:18PM +0000, Alejandro Zeise wrote:
> Removes old hash implemention in the gnutls hash driver.
> 
> Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
> ---
>  crypto/hash-gnutls.c | 47 --------------------------------------------
>  1 file changed, 47 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 :|

Re: [PATCH v4 12/15] crypto/hash-nettle: Remove old hash API functions

[Daniel P. Berrangé]

On Wed, Aug 07, 2024 at 07:51:19PM +0000, Alejandro Zeise wrote:
> Removes old hash implemention in the nettle hash driver.
> 
> Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
> ---
>  crypto/hash-nettle.c | 53 --------------------------------------------
>  1 file changed, 53 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 :|

Re: [PATCH v4 13/15] crypto/hash-afalg: Remove old hash API functions

[Daniel P. Berrangé]

On Wed, Aug 07, 2024 at 07:51:20PM +0000, Alejandro Zeise wrote:
> Removes the old hash API functions in the afalg driver,
> and modifies the hmac function to use the new helper functions.
> 
> Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
> ---
>  crypto/hash-afalg.c | 59 +++------------------------------------------
>  1 file changed, 3 insertions(+), 56 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 :|

Re: [PATCH v4 14/15] crypto/hashpriv: Remove old hash API function

[Daniel P. Berrangé]

On Wed, Aug 07, 2024 at 07:51:21PM +0000, Alejandro Zeise wrote:
> Remove old hash_bytesv function, as it was replaced by the 4
> new functions.
> 
> Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
> ---
>  crypto/hashpriv.h | 6 ------
>  1 file changed, 6 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 :|

RE: [PATCH v4 03/15] crypto/hash-gcrypt: Implement new hash API

[Alejandro Zeise]

> On Wed, Aug 07, 2024 at 07:51:10PM +0000, Alejandro Zeise wrote:
> > Implements the new hashing API in the gcrypt hash driver.
> > Supports creating/destroying a context, updating the context with 
> > input data and obtaining an output hash.
> >
> > Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
> > ---
> >  crypto/hash-gcrypt.c | 79 
> > ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 79 insertions(+)
> >
> > diff --git a/crypto/hash-gcrypt.c b/crypto/hash-gcrypt.c index 
> > 829e48258d..e05511cafa 100644
> > --- a/crypto/hash-gcrypt.c
> > +++ b/crypto/hash-gcrypt.c
> > @@ -1,6 +1,7 @@
> >  /*
> >   * QEMU Crypto hash algorithms
> >   *
> > + * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates
> >   * Copyright (c) 2016 Red Hat, Inc.
> >   *
> >   * This library is free software; you can redistribute it and/or @@ 
> > -110,7 +111,85 @@ qcrypto_gcrypt_hash_bytesv(QCryptoHashAlgorithm alg,
> >      return -1;
> >  }
> >
> > +static
> > +QCryptoHash *qcrypto_gcrypt_hash_new(QCryptoHashAlgorithm alg, Error 
> > +**errp) {
> > +    QCryptoHash *hash = NULL;
> > +
> > +    if (!qcrypto_hash_supports(alg)) {
> > +        error_setg(errp,
> > +                   "Unknown hash algorithm %d",
> > +                   alg);
> > +    } else {
> > +        hash = g_new(QCryptoHash, 1);
> > +        hash->alg = alg;
> > +        hash->opaque = g_new(gcry_md_hd_t, 1);
> > +
> > +        gcry_md_open((gcry_md_hd_t *) hash->opaque, qcrypto_hash_alg_map[alg], 0);
> > +    }
> > +
> > +    return hash;
> > +}
> > +
> > +static
> > +void qcrypto_gcrypt_hash_free(QCryptoHash *hash) {
> > +    gcry_md_hd_t *ctx = hash->opaque;
> > +
> > +    if (ctx) {
> > +        gcry_md_close(*ctx);
> > +        g_free(ctx);
> > +    }
> > +
> > +    g_free(hash);
> > +}
> > +
> > +
> > +static
> > +int qcrypto_gcrypt_hash_update(QCryptoHash *hash,
> > +                               const struct iovec *iov,
> > +                               size_t niov,
> > +                               Error **errp) {
> > +    gcry_md_hd_t *ctx = hash->opaque;
> > +
> > +    for (int i = 0; i < niov; i++) {
> > +        gcry_md_write(*ctx, iov[i].iov_base, iov[i].iov_len);
>
>   int ret = gcry_md_write(*ctx, iov[i].iov_base, iov[i].iov_len);
>   if (ret != 0) {
>       error_setg(....)
>       return -1;
>   }
>
>
> With regards,
> Daniel

gcry_md_write() is declared as void which is why I had no error variable present.

Thanks,
Alejandro

Re: [PATCH v4 03/15] crypto/hash-gcrypt: Implement new hash API

[Daniel P. Berrangé]

On Thu, Aug 08, 2024 at 06:24:10PM +0000, Alejandro Zeise wrote:
> > On Wed, Aug 07, 2024 at 07:51:10PM +0000, Alejandro Zeise wrote:
> > > Implements the new hashing API in the gcrypt hash driver.
> > > Supports creating/destroying a context, updating the context with 
> > > input data and obtaining an output hash.
> > >
> > > Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
> > > ---
> > >  crypto/hash-gcrypt.c | 79 
> > > ++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 79 insertions(+)
> > >
> > > diff --git a/crypto/hash-gcrypt.c b/crypto/hash-gcrypt.c index 
> > > 829e48258d..e05511cafa 100644
> > > --- a/crypto/hash-gcrypt.c
> > > +++ b/crypto/hash-gcrypt.c
> > > @@ -1,6 +1,7 @@
> > >  /*
> > >   * QEMU Crypto hash algorithms
> > >   *
> > > + * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates
> > >   * Copyright (c) 2016 Red Hat, Inc.
> > >   *
> > >   * This library is free software; you can redistribute it and/or @@ 
> > > -110,7 +111,85 @@ qcrypto_gcrypt_hash_bytesv(QCryptoHashAlgorithm alg,
> > >      return -1;
> > >  }
> > >
> > > +static
> > > +QCryptoHash *qcrypto_gcrypt_hash_new(QCryptoHashAlgorithm alg, Error 
> > > +**errp) {
> > > +    QCryptoHash *hash = NULL;
> > > +
> > > +    if (!qcrypto_hash_supports(alg)) {
> > > +        error_setg(errp,
> > > +                   "Unknown hash algorithm %d",
> > > +                   alg);
> > > +    } else {
> > > +        hash = g_new(QCryptoHash, 1);
> > > +        hash->alg = alg;
> > > +        hash->opaque = g_new(gcry_md_hd_t, 1);
> > > +
> > > +        gcry_md_open((gcry_md_hd_t *) hash->opaque, qcrypto_hash_alg_map[alg], 0);
> > > +    }
> > > +
> > > +    return hash;
> > > +}
> > > +
> > > +static
> > > +void qcrypto_gcrypt_hash_free(QCryptoHash *hash) {
> > > +    gcry_md_hd_t *ctx = hash->opaque;
> > > +
> > > +    if (ctx) {
> > > +        gcry_md_close(*ctx);
> > > +        g_free(ctx);
> > > +    }
> > > +
> > > +    g_free(hash);
> > > +}
> > > +
> > > +
> > > +static
> > > +int qcrypto_gcrypt_hash_update(QCryptoHash *hash,
> > > +                               const struct iovec *iov,
> > > +                               size_t niov,
> > > +                               Error **errp) {
> > > +    gcry_md_hd_t *ctx = hash->opaque;
> > > +
> > > +    for (int i = 0; i < niov; i++) {
> > > +        gcry_md_write(*ctx, iov[i].iov_base, iov[i].iov_len);
> >
> >   int ret = gcry_md_write(*ctx, iov[i].iov_base, iov[i].iov_len);
> >   if (ret != 0) {
> >       error_setg(....)
> >       return -1;
> >   }
> >
> >
> > With regards,
> > Daniel
> 
> gcry_md_write() is declared as void which is why I had no error variable present.

Opps, yes, I'm getting confused with the other APIs.


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 :|

Re: [PATCH v4 15/15] hw/misc/aspeed_hace: Fix SG Accumulative hashing

[Cédric Le Goater]

On 8/7/24 21:51, Alejandro Zeise wrote:
> Make the Aspeed HACE module use the new qcrypto accumulative hashing functions
> when in scatter-gather accumulative mode. A hash context will maintain a
> "running-hash" as each scatter-gather chunk is received.
> 
> Previously each scatter-gather "chunk" was cached
> so the hash could be computed once the final chunk was received.
> However, the cache was a shallow copy, so once the guest overwrote the
> memory provided to HACE the final hash would not be correct.
> 
> Possibly related to: https://gitlab.com/qemu-project/qemu/-/issues/1121
> Buglink: https://github.com/openbmc/qemu/issues/36
> 
> Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
> ---
>   hw/misc/aspeed_hace.c         | 94 +++++++++++++++++++----------------
>   include/hw/misc/aspeed_hace.h |  4 ++
>   2 files changed, 54 insertions(+), 44 deletions(-)
> 
> diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
> index c06c04ddc6..4247403d45 100644
> --- a/hw/misc/aspeed_hace.c
> +++ b/hw/misc/aspeed_hace.c
> @@ -1,6 +1,7 @@
>   /*
>    * ASPEED Hash and Crypto Engine
>    *
> + * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates
>    * Copyright (C) 2021 IBM Corp.
>    *
>    * Joel Stanley <joel@jms.id.au>
> @@ -151,50 +152,28 @@ static int reconstruct_iov(AspeedHACEState *s, struct iovec *iov, int id,
>       return iov_count;
>   }
>   
> -/**
> - * Generate iov for accumulative mode.
> - *
> - * @param s             aspeed hace state object
> - * @param iov           iov of the current request
> - * @param id            index of the current iov
> - * @param req_len       length of the current request
> - *
> - * @return count of iov
> - */
> -static int gen_acc_mode_iov(AspeedHACEState *s, struct iovec *iov, int id,
> -                            hwaddr *req_len)
> -{
> -    uint32_t pad_offset;
> -    uint32_t total_msg_len;
> -    s->total_req_len += *req_len;
> -
> -    if (has_padding(s, &iov[id], *req_len, &total_msg_len, &pad_offset)) {
> -        if (s->iov_count) {
> -            return reconstruct_iov(s, iov, id, &pad_offset);
> -        }
> -
> -        *req_len -= s->total_req_len - total_msg_len;
> -        s->total_req_len = 0;
> -        iov[id].iov_len = *req_len;
> -    } else {
> -        s->iov_cache[s->iov_count].iov_base = iov->iov_base;
> -        s->iov_cache[s->iov_count].iov_len = *req_len;
> -        ++s->iov_count;
> -    }
> -
> -    return id + 1;
> -}
> -
>   static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode,
>                                 bool acc_mode)
>   {
>       struct iovec iov[ASPEED_HACE_MAX_SG];
> +    uint32_t total_msg_len;
> +    uint32_t pad_offset;
>       g_autofree uint8_t *digest_buf = NULL;
>       size_t digest_len = 0;
> -    int niov = 0;
> +    bool sg_acc_mode_final_request = false;
>       int i;
>       void *haddr;
>   
> +    if (acc_mode && s->hash_ctx == NULL) {
> +        s->hash_ctx = qcrypto_hash_new(algo, NULL);

I wonder if we shouldn't use an 'Error *' argument instead and output
the literal error with error_get_pretty() in qemu_log_mask() ?

> +        if (s->hash_ctx == NULL) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "%s: qcrypto failed to create hash context\n",
> +                          __func__);
> +            return;
> +        }
> +    }
> +
>       if (sg_mode) {
>           uint32_t len = 0;
>   
> @@ -226,8 +205,15 @@ static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode,
>               }
>               iov[i].iov_base = haddr;
>               if (acc_mode) {
> -                niov = gen_acc_mode_iov(s, iov, i, &plen);
> -
> +                s->total_req_len += plen;
> +
> +                if (has_padding(s, &iov[i], plen, &total_msg_len, &pad_offset)) {
> +                    /* Padding being present indicates the final request */
> +                    sg_acc_mode_final_request = true;
> +                    iov[i].iov_len = pad_offset;
> +                } else {
> +                    iov[i].iov_len = plen;
> +                }
>               } else {
>                   iov[i].iov_len = plen;
>               }
> @@ -252,20 +238,35 @@ static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode,
>                * required to check whether cache is empty. If no, we should
>                * combine cached iov and the current iov.
>                */
> -            uint32_t total_msg_len;
> -            uint32_t pad_offset;
>               s->total_req_len += len;
>               if (has_padding(s, iov, len, &total_msg_len, &pad_offset)) {
> -                niov = reconstruct_iov(s, iov, 0, &pad_offset);
> +                i = reconstruct_iov(s, iov, 0, &pad_offset);
>               }
>           }
>       }
>   
> -    if (niov) {
> -        i = niov;
> -    }
> +    if (acc_mode) {
> +        if (qcrypto_hash_updatev(s->hash_ctx, iov, i, NULL) < 0) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "%s: qcrypto hash update failed\n", __func__);
> +            return;

This will drop the ending sequence: address_space_unmap() and
setting of the IRQ status register. I guess adding an 'exit'
goto label is preferable.

> +        }
> +
> +        if (sg_acc_mode_final_request) {
> +            if (qcrypto_hash_finalize_bytes(s->hash_ctx, &digest_buf,
> +                                            &digest_len, NULL)) {
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                              "%s: qcrypto failed to finalize hash\n", __func__);
> +            }
>   
> -    if (qcrypto_hash_bytesv(algo, iov, i, &digest_buf, &digest_len, NULL) < 0) {
> +            qcrypto_hash_free(s->hash_ctx);
> +
> +            s->hash_ctx = NULL;
> +            s->iov_count = 0;
> +            s->total_req_len = 0;
> +        }
> +    } else if (qcrypto_hash_bytesv(algo, iov, i, &digest_buf,
> +                                   &digest_len, NULL) < 0) {
>           qemu_log_mask(LOG_GUEST_ERROR, "%s: qcrypto failed\n", __func__);

Same issue here with the ending sequence. This should be fixed in
a separate patch.


Thanks,

C.



>           return;
>       }
> @@ -397,6 +398,11 @@ static void aspeed_hace_reset(DeviceState *dev)
>   {
>       struct AspeedHACEState *s = ASPEED_HACE(dev);
>   
> +    if (s->hash_ctx != NULL) {
> +        qcrypto_hash_free(s->hash_ctx);
> +        s->hash_ctx = NULL;
> +    }
> +
>       memset(s->regs, 0, sizeof(s->regs));
>       s->iov_count = 0;
>       s->total_req_len = 0;
> diff --git a/include/hw/misc/aspeed_hace.h b/include/hw/misc/aspeed_hace.h
> index ecb1b67de8..4af9919195 100644
> --- a/include/hw/misc/aspeed_hace.h
> +++ b/include/hw/misc/aspeed_hace.h
> @@ -1,6 +1,7 @@
>   /*
>    * ASPEED Hash and Crypto Engine
>    *
> + * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates
>    * Copyright (C) 2021 IBM Corp.
>    *
>    * SPDX-License-Identifier: GPL-2.0-or-later
> @@ -10,6 +11,7 @@
>   #define ASPEED_HACE_H
>   
>   #include "hw/sysbus.h"
> +#include "crypto/hash.h"
>   
>   #define TYPE_ASPEED_HACE "aspeed.hace"
>   #define TYPE_ASPEED_AST2400_HACE TYPE_ASPEED_HACE "-ast2400"
> @@ -35,6 +37,8 @@ struct AspeedHACEState {
>   
>       MemoryRegion *dram_mr;
>       AddressSpace dram_as;
> +
> +    QCryptoHash *hash_ctx;
>   };
>   
>