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

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

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