Re: [PATCH v5 16/16] hw/misc/aspeed_hace: Fix SG Accumulative hashing

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

On Tue, Oct 08, 2024 at 09:57:23AM +0200, Cédric Le Goater wrote:
> From: Alejandro Zeise <alejandro.zeise@seagate.com>
> 
> Make the Aspeed HACE module use the new qcrypto accumulative hashing functions
> when in scatter-gather accumulative mode. A hash context will maintain a
> "running-hash" as each scatter-gather chunk is received.
> 
> Previously each scatter-gather "chunk" was cached
> so the hash could be computed once the final chunk was received.
> However, the cache was a shallow copy, so once the guest overwrote the
> memory provided to HACE the final hash would not be correct.
> 
> Possibly related to: https://gitlab.com/qemu-project/qemu/-/issues/1121
> Buglink: https://github.com/openbmc/qemu/issues/36
> 
> Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
> [ clg: - Checkpatch fixes ]
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>  include/hw/misc/aspeed_hace.h |  4 ++
>  hw/misc/aspeed_hace.c         | 96 +++++++++++++++++++----------------
>  2 files changed, 56 insertions(+), 44 deletions(-)
> 

>  static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode,
>                                bool acc_mode)
>  {
>      struct iovec iov[ASPEED_HACE_MAX_SG];
> +    uint32_t total_msg_len;
> +    uint32_t pad_offset;
>      g_autofree uint8_t *digest_buf = NULL;
>      size_t digest_len = 0;
> -    int niov = 0;
> +    bool sg_acc_mode_final_request = false;
>      int i;
>      void *haddr;
>  
> +    if (acc_mode && s->hash_ctx == NULL) {

  Error local_err = NULL;

> +        s->hash_ctx = qcrypto_hash_new(algo, NULL);

  &local_err;

> +        if (s->hash_ctx == NULL) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "%s: qcrypto failed to create hash context\n",
> +                          __func__);

Add error_get_pretty() output to the message so we get useful
information reported, and then error_free,.

> +            return;
> +        }
> +    }
> +
>      if (sg_mode) {
>          uint32_t len = 0;
>  


> -    if (niov) {
> -        i = niov;
> -    }
> +    if (acc_mode) {
> +        if (qcrypto_hash_updatev(s->hash_ctx, iov, i, NULL) < 0) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "%s: qcrypto hash update failed\n", __func__);
> +            return;
> +        }
> +
> +        if (sg_acc_mode_final_request) {
> +            if (qcrypto_hash_finalize_bytes(s->hash_ctx, &digest_buf,
> +                                            &digest_len, NULL)) {
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                              "%s: qcrypto failed to finalize hash\n",
> +                              __func__);
> +            }
>  
> -    if (qcrypto_hash_bytesv(algo, iov, i, &digest_buf, &digest_len, NULL) < 0) {
> +            qcrypto_hash_free(s->hash_ctx);
> +
> +            s->hash_ctx = NULL;
> +            s->iov_count = 0;
> +            s->total_req_len = 0;
> +        }
> +    } else if (qcrypto_hash_bytesv(algo, iov, i, &digest_buf,
> +                                   &digest_len, NULL) < 0) {
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: qcrypto failed\n", __func__);
>          return;
>      }

Same comment about passing an Error object to all these methods
and logging the useful error message.



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