Hello Cédric, > -----Original Message----- > From: Cédric Le Goater <c...@kaod.org> > Sent: Tuesday, July 30, 2024 5:55 AM > To: Alejandro Zeise <alejandro.ze...@seagate.com>; qemu-...@nongnu.org > Cc: qemu-devel@nongnu.org; peter.mayd...@linaro.org; berra...@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
