Re: [PATCH v2 2/2] hw/misc/aspeed_hace: Fix SG Accumulative hashing

[Philippe Mathieu-Daudé]

Hi Alejandro,

On 29/7/24 21:00, Alejandro Zeise wrote:
> 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

Likely, Cc'ing John.

Reported-by: John Wang <wangzq.jn@gmail.com>

> Buglink: https://github.com/openbmc/qemu/issues/36
>
> Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com>
> ---
>   hw/misc/aspeed_hace.c         | 91 ++++++++++++++++++-----------------
>   include/hw/misc/aspeed_hace.h |  4 ++
>   2 files changed, 51 insertions(+), 44 deletions(-)


> @@ -252,20 +228,42 @@ static void do_hash_operation(AspeedHACEState *s, int 
> algo, bool sg_mode,


> -    if (niov) {
> -        i = niov;
> -    }
> +    if (acc_mode) {
> +        if (s->qcrypto_hash_context == NULL &&
> +            qcrypto_hash_accumulate_new_ctx(algo, &s->qcrypto_hash_context, 
> NULL)) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "%s: qcrypto failed to create hash context\n",
> +                          __func__);
> +            return;
> +        }

Using a instance_init() handler, ...

> @@ -397,6 +395,11 @@ static void aspeed_hace_reset(DeviceState *dev)
>   {
>       struct AspeedHACEState *s = ASPEED_HACE(dev);
>   
> +    if (s->qcrypto_hash_context != NULL) {
> +        qcrypto_hash_accumulate_free_ctx(s->qcrypto_hash_context, NULL);
> +        s->qcrypto_hash_context = NULL;
> +    }

... and instance_finalize() could simplify a bit.

Regards,

Phil.