diff mbox series

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

Message ID 20240807195122.2827364-8-alejandro.zeise@seagate.com (mailing list archive)
State New, archived
Headers show
Series hw/misc/aspeed_hace: Fix SG Accumulative Hash Calculations | expand

Commit Message

Alejandro Zeise Aug. 7, 2024, 7:51 p.m. UTC 
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(-)

Comments

Daniel P. Berrangé Aug. 8, 2024, 4:21 p.m. UTC | #1 
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
diff mbox series

Patch

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,