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

[Alejandro Zeise]

Hello Cédric,

> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org> 
> Sent: Tuesday, July 30, 2024 5:55 AM
> To: Alejandro Zeise <alejandro.zeise@seagate.com>; qemu-arm@nongnu.org
> Cc: qemu-devel@nongnu.org; peter.maydell@linaro.org; berrange@redhat.com
> Subject: Re: [PATCH v2 2/2] hw/misc/aspeed_hace: Fix SG Accumulative hashing


> This message has originated from an External Source. Please use proper 
> judgment and caution when opening attachments, clicking links, or responding 
> to this email.


> Hello Alejandro,

> On 7/29/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://secure-web.cisco.com/1m1Z2YCrV2vhocDftauu92ZeOegqeZfIRFUAJJwVk
>> F9fI5CEmq9q8M0oYYTKLIBE7H7Anc9W8PG2iYcK9L1uzXdg80qNty6Zu1X6QN7rQvV39cA
>> qS8AoWcsYMaBzVo6gGKY2upM6_8eJmiOy2M7vrGmEaaUE-SnW6zCD1bbH3et6nErWgGfgs
>> TIxuiRzgwvE7wrTPewA6isDxOsWeEcldCKuKbmYH2sSnQ3jpdZMKmpgX9tVZfICI3nwhwb
>> wz8RYBg_4OH-8kl9ECEW6grKDPPi8Y0ni3zO8HPywr36nwkhAuYmK8hC27vBbf7qfloMu5
>> tzRGJBUPQcdSjWeK_4MtrxQCoIpUFEKPMqf-ielVMsCTsuYh0ABhB9ur84fcQpnXtTFuib
>> WvRU6x-AQGW1WmZzgxUArNn7_Ha_Kf0PknI2b9LJIKaSCuNa4y37x0aQpO_BYL0LBaQEKA
>> cUedbaxB-Q/https%3A%2F%2Fgitlab.com%2Fqemu-project%2Fqemu%2F-%2Fissues
>> %2F1121
>> Buglink: 
>> https://secure-web.cisco.com/1ofiYFe1SNQgzRQ3E3PS9LITMhUsDJZIVWuOog_Vj
>> tgnTabxX9NqW5LYr4EPFrVCzPtMf-hAx87qIkUirL6wMbnKyhgCeobULUm0wBYi9botfSo
>> DKFXa6vBO6GgNdVBAbGMSCO1IgmogBY_Q33lM6M7OFbMipxxsGDhIBJMeYneaqgr5Qatdc
>> gcmCgju8b2v60q-f7xxTJFtDihOD9rfCzPkspw9y9McZyzUqVOb-Fin-SYQEaC9sEN7pFC
>> Zu59djaWocCIRIudGSVPubNCyV6-AfUNlAL6Bfh-e99_VoDpCMv6KCZGVooRjcdCvUW4uJ
>> _qhVa7VQKJMQbHJEE-Qq1hdOlPbR2Ae5dQoizEatClH05JZLE1ljgu2JtAIrioZ7JyfmMn
>> j-w8-ChDDGYPdtVNLKk0_Trhz4Z9WvvlC-gAWirx_aEwzAYF7FRC8I_oqAZUoDDxJRcSYd
>> Yv4XjCcqGA/https%3A%2F%2Fgithub.com%2Fopenbmc%2Fqemu%2Fissues%2F36


> Thanks for these changes.

> However, this introduces a regression when compiling QEMU with 
> --disable-gcrypt. It can be reproduced with :

   > make check-avocado AVOCADO_TAGS=machine:ast1030-evb

> or download :

>    
> https://secure-web.cisco.com/1umbBg5FhtHfBrEEv8iImyA69NcvzgLwutRYKPVhm9UjD3u1ETDUrEMfnNU50zJVD70Q2QUuebx6IuTRbReF-w23bV5fr6yHwO72IdXBxZSrPuLFZT-sPiIz3uiOByzLlWL6mU4IP1dZ6nxfTqGY2QTegv53M8dqNiUi3D_StfcLIC56_bcgZr2LTW86FAJKy7jACaJsk8AyvYeSrC--YZe_bpzP7TY2eigBe4Qg_BHUcIWDTR0aXyrxQZjIgxx05bXQ0lq9tQuQUYF7d9LoTsFiJ22xrX_gKU7mTcGbc1I1W4nJ1KG69UDBltjnjgPCimqKx0zWhkQncC3CpSwUrhNodbuJgMFKyn58q7WAdL7j0PyDq9kHt9drnai_4lND2mYhIFhQHfeGR6qwTRxh1p-i8EMARTHhMKst7Xu37wvzoFVt1PrHwzhsOD_xTr9q2r6DGK2zPXwp5Wz130qMc5w/https%3A%2F%2Fgithub.com%2FAspeedTech-BMC%2Fzephyr%2Freleases%2Fdownload%2Fv00.01.07%2Fast1030-evb-demo.zip

> unzip and run :

>    path/to/qemu-system-arm -M ast1030-evb -kernel ./zephyr.bin -nographic

> then, on the console :

>    uart:~$ hash test
>    sha256_test
>    tv[0]:PASS
>    tv[1]:PASS
>    tv[2]:PASS
>    tv[3]:PASS
>    tv[4]:PASS
>    sha384_test
>    tv[0]:hash_final error
>    sha512_test
>    tv[0]:Segmentation fault (core dumped)

>  I believe this is due to the change which now assigns s->total_req_len when 
> accumulation mode is desired. If the crypto context fails to allocate 
>  (no gcrypt), states are not cleared in the model and previous values are 
> used by the model when the OS runs the next test and QEMU segvs in 
> has_padding().

> To fix, I think we could move :

>          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;
>          }

>  at the beginning of do_hash_operation() ?

>  C.

Good catch. Moving the context allocation to the beginning of the function does 
appear to fix the issue.
I will submit a new patch series with that change implemented.

Thanks,
Alejandro