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