Re: [PATCH v3 01/12] crypto: accumulative hashing API

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Aug 05, 2024 at 03:50:36PM +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/hash.c         | 136 +++++++++++++++++++++++++++++++-----------
>  crypto/hashpriv.h     |  19 ++++--
>  include/crypto/hash.h | 106 ++++++++++++++++++++++++++++++++
>  3 files changed, 220 insertions(+), 41 deletions(-)
> 
> diff --git a/crypto/hash.c b/crypto/hash.c
> index b0f8228bdc..5c60973bde 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_update(ctx, iov, niov, errp) ||
> +               qcrypto_hash_finalize_bytes(ctx, result, resultlen, errp);
> +
> +        /* Ensure context is always freed regardless of error */
> +        fail = qcrypto_hash_free(ctx) || fail;
> +    } else {
> +        fail = -1;
>      }
> -#endif
>  
> -    return qcrypto_hash_lib_driver.hash_bytesv(alg, iov, niov,
> -                                               result, resultlen,
> -                                               errp);
> +    return fail;
>  }

You can't do this conversion in this patch, because all the hash impls
are still using the old driver API, and haven't implemented the new
API yet.

QEMU requires "make check" succeed for *every* individual patch in
a series, so that 'git bisect' can be used in future.

> diff --git a/crypto/hashpriv.h b/crypto/hashpriv.h
> index cee26ccb47..8a7d80619e 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,15 +16,21 @@
>  #ifndef QCRYPTO_HASHPRIV_H
>  #define QCRYPTO_HASHPRIV_H
>  
> +#include "crypto/hash.h"
> +
>  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);

Keep this present. It can only be removed at the very end of the
series once all the drivers are converted.

> +   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);
> +   int (*hash_free)(QCryptoHash *hash);

I'd expect 'free' functions to always be 'void'

> diff --git a/include/crypto/hash.h b/include/crypto/hash.h
> index 54d87aa2a1..96d080eeb5 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,102 @@ int qcrypto_hash_digestv(QCryptoHashAlgorithm alg,
>                           char **digest,
>                           Error **errp);
>  
> +/**
> + * qcrypto_hash_update:
> + * @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, -1 on error
> + */
> +int qcrypto_hash_update(QCryptoHash *hash,
> +                        const struct iovec *iov,
> +                        size_t niov,
> +                        Error **errp);

This should be renamed 'qcrypto_hash_updatev', and we should have a
separate non-iovec variant

 int qcrypto_hash_update(QCryptoHash *hash,
                         const char *data,
                         size_t len,
                         Error **errp);

This can simply pack data+len into an iovec, and then call
qcrypto_hash_updatev.


> +
> +/**
> + * 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, -1 on error
> + */
> +int qcrypto_hash_finalize_digest(QCryptoHash *hash,
> +                                 char **digest,
> +                                 Error **errp);
> +
> +/**
> + * qcrypto_hash_finalize_base64:
> + * @hash_ctx: hash object to finalize

s/hash_ctx/hash/

> + * @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, -1 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, -1 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.
> + *
> + * Returns: 0 on success, -1 on error
> + */
> +int qcrypto_hash_free(QCryptoHash *hash);

Again I'd expect this to be 'void' and have a g_autoptr support
added using:

  G_DEFINE_AUTOPTR_CLEANUP_FUNC(QCryptoHash, qcrypto_hash_free)

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