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 <[email protected]>
> ---
> 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 :|
