| Message ID | 20241011053825.361544-1-clg@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v6] hw/misc/aspeed_hace: Fix SG Accumulative hashing | expand |
+ Aspeed reviewers. Sorry about that. C. On 10/11/24 07:38, 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 > - Reworked qcrypto_hash*() error reports in do_hash_operation() ] > Signed-off-by: Cédric Le Goater <clg@redhat.com> > --- > > Changes in v6: > - Reworked qcrypto_hash*() error reports in do_hash_operation() > > include/hw/misc/aspeed_hace.h | 4 ++ > hw/misc/aspeed_hace.c | 104 +++++++++++++++++++--------------- > 2 files changed, 63 insertions(+), 45 deletions(-) > > diff --git a/include/hw/misc/aspeed_hace.h b/include/hw/misc/aspeed_hace.h > index ecb1b67de816..4af99191955a 100644 > --- a/include/hw/misc/aspeed_hace.h > +++ b/include/hw/misc/aspeed_hace.h > @@ -1,6 +1,7 @@ > /* > * ASPEED Hash and Crypto Engine > * > + * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates > * Copyright (C) 2021 IBM Corp. > * > * SPDX-License-Identifier: GPL-2.0-or-later > @@ -10,6 +11,7 @@ > #define ASPEED_HACE_H > > #include "hw/sysbus.h" > +#include "crypto/hash.h" > > #define TYPE_ASPEED_HACE "aspeed.hace" > #define TYPE_ASPEED_AST2400_HACE TYPE_ASPEED_HACE "-ast2400" > @@ -35,6 +37,8 @@ struct AspeedHACEState { > > MemoryRegion *dram_mr; > AddressSpace dram_as; > + > + QCryptoHash *hash_ctx; > }; > > > diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c > index b6f43f65b29a..bc1d66ad8064 100644 > --- a/hw/misc/aspeed_hace.c > +++ b/hw/misc/aspeed_hace.c > @@ -1,6 +1,7 @@ > /* > * ASPEED Hash and Crypto Engine > * > + * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates > * Copyright (C) 2021 IBM Corp. > * > * Joel Stanley <joel@jms.id.au> > @@ -151,49 +152,28 @@ static int reconstruct_iov(AspeedHACEState *s, struct iovec *iov, int id, > return iov_count; > } > > -/** > - * Generate iov for accumulative mode. > - * > - * @param s aspeed hace state object > - * @param iov iov of the current request > - * @param id index of the current iov > - * @param req_len length of the current request > - * > - * @return count of iov > - */ > -static int gen_acc_mode_iov(AspeedHACEState *s, struct iovec *iov, int id, > - hwaddr *req_len) > -{ > - uint32_t pad_offset; > - uint32_t total_msg_len; > - s->total_req_len += *req_len; > - > - if (has_padding(s, &iov[id], *req_len, &total_msg_len, &pad_offset)) { > - if (s->iov_count) { > - return reconstruct_iov(s, iov, id, &pad_offset); > - } > - > - *req_len -= s->total_req_len - total_msg_len; > - s->total_req_len = 0; > - iov[id].iov_len = *req_len; > - } else { > - s->iov_cache[s->iov_count].iov_base = iov->iov_base; > - s->iov_cache[s->iov_count].iov_len = *req_len; > - ++s->iov_count; > - } > - > - return id + 1; > -} > - > 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; > + Error *local_err = NULL; > + > + if (acc_mode && s->hash_ctx == NULL) { > + s->hash_ctx = qcrypto_hash_new(algo, &local_err); > + if (s->hash_ctx == NULL) { > + qemu_log_mask(LOG_GUEST_ERROR, "qcrypto hash failed : %s", > + error_get_pretty(local_err)); > + error_free(local_err); > + return; > + } > + } > > if (sg_mode) { > uint32_t len = 0; > @@ -226,8 +206,16 @@ static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode, > } > iov[i].iov_base = haddr; > if (acc_mode) { > - niov = gen_acc_mode_iov(s, iov, i, &plen); > - > + s->total_req_len += plen; > + > + if (has_padding(s, &iov[i], plen, &total_msg_len, > + &pad_offset)) { > + /* Padding being present indicates the final request */ > + sg_acc_mode_final_request = true; > + iov[i].iov_len = pad_offset; > + } else { > + iov[i].iov_len = plen; > + } > } else { > iov[i].iov_len = plen; > } > @@ -252,21 +240,42 @@ static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode, > * required to check whether cache is empty. If no, we should > * combine cached iov and the current iov. > */ > - uint32_t total_msg_len; > - uint32_t pad_offset; > s->total_req_len += len; > if (has_padding(s, iov, len, &total_msg_len, &pad_offset)) { > - niov = reconstruct_iov(s, iov, 0, &pad_offset); > + i = reconstruct_iov(s, iov, 0, &pad_offset); > } > } > } > > - if (niov) { > - i = niov; > - } > + if (acc_mode) { > + if (qcrypto_hash_updatev(s->hash_ctx, iov, i, &local_err) < 0) { > + qemu_log_mask(LOG_GUEST_ERROR, "qcrypto hash update failed : %s", > + error_get_pretty(local_err)); > + error_free(local_err); > + return; > + } > + > + if (sg_acc_mode_final_request) { > + if (qcrypto_hash_finalize_bytes(s->hash_ctx, &digest_buf, > + &digest_len, &local_err)) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "qcrypto hash finalize failed : %s", > + error_get_pretty(local_err)); > + error_free(local_err); > + local_err = NULL; > + } > > - if (qcrypto_hash_bytesv(algo, iov, i, &digest_buf, &digest_len, NULL) < 0) { > - qemu_log_mask(LOG_GUEST_ERROR, "%s: qcrypto failed\n", __func__); > + 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, &local_err) < 0) { > + qemu_log_mask(LOG_GUEST_ERROR, "qcrypto hash bytesv failed : %s", > + error_get_pretty(local_err)); > + error_free(local_err); > return; > } > > @@ -397,6 +406,11 @@ static void aspeed_hace_reset(DeviceState *dev) > { > struct AspeedHACEState *s = ASPEED_HACE(dev); > > + if (s->hash_ctx != NULL) { > + qcrypto_hash_free(s->hash_ctx); > + s->hash_ctx = NULL; > + } > + > memset(s->regs, 0, sizeof(s->regs)); > s->iov_count = 0; > s->total_req_len = 0;
On Sat, 2024-10-12 at 08:20 +0200, Cédric Le Goater wrote: > + Aspeed reviewers. Sorry about that. All good. Seems sensible in concept and from a cursory glance, so if you want to tack it on: Acked-by: Andrew Jeffery <andrew@codeconstruct.com.au>
On 10/15/24 02:52, Andrew Jeffery wrote: > On Sat, 2024-10-12 at 08:20 +0200, Cédric Le Goater wrote: >> + Aspeed reviewers. Sorry about that. > > All good. Seems sensible in concept and from a cursory glance, so if > you want to tack it on: > > Acked-by: Andrew Jeffery <andrew@codeconstruct.com.au> Thanks Andrew, Now I wonder, if we can close these : Possibly related to: https://gitlab.com/qemu-project/qemu/-/issues/1121 Buglink: https://github.com/openbmc/qemu/issues/36 C.
Hi Cedric, > Subject: Re: [PATCH v6] hw/misc/aspeed_hace: Fix SG Accumulative hashing > > + Aspeed reviewers. Sorry about that. > > C. > > > On 10/11/24 07:38, 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 > > - Reworked qcrypto_hash*() error reports in > > do_hash_operation() ] > > Signed-off-by: Cédric Le Goater <clg@redhat.com> > > --- > > > > Changes in v6: > > - Reworked qcrypto_hash*() error reports in do_hash_operation() > > > > include/hw/misc/aspeed_hace.h | 4 ++ > > hw/misc/aspeed_hace.c | 104 > +++++++++++++++++++--------------- > > 2 files changed, 63 insertions(+), 45 deletions(-) > > > > diff --git a/include/hw/misc/aspeed_hace.h > > b/include/hw/misc/aspeed_hace.h index ecb1b67de816..4af99191955a > > 100644 > > --- a/include/hw/misc/aspeed_hace.h > > +++ b/include/hw/misc/aspeed_hace.h > > @@ -1,6 +1,7 @@ > > /* > > * ASPEED Hash and Crypto Engine > > * > > + * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates > > * Copyright (C) 2021 IBM Corp. > > * > > * SPDX-License-Identifier: GPL-2.0-or-later @@ -10,6 +11,7 @@ > > #define ASPEED_HACE_H > > > > #include "hw/sysbus.h" > > +#include "crypto/hash.h" > > > > #define TYPE_ASPEED_HACE "aspeed.hace" > > #define TYPE_ASPEED_AST2400_HACE TYPE_ASPEED_HACE "-ast2400" > > @@ -35,6 +37,8 @@ struct AspeedHACEState { > > > > MemoryRegion *dram_mr; > > AddressSpace dram_as; > > + > > + QCryptoHash *hash_ctx; > > }; > > > > > > diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c index > > b6f43f65b29a..bc1d66ad8064 100644 > > --- a/hw/misc/aspeed_hace.c > > +++ b/hw/misc/aspeed_hace.c > > @@ -1,6 +1,7 @@ > > /* > > * ASPEED Hash and Crypto Engine > > * > > + * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates > > * Copyright (C) 2021 IBM Corp. > > * > > * Joel Stanley <joel@jms.id.au> > > @@ -151,49 +152,28 @@ static int reconstruct_iov(AspeedHACEState *s, > struct iovec *iov, int id, > > return iov_count; > > } > > > > -/** > > - * Generate iov for accumulative mode. > > - * > > - * @param s aspeed hace state object > > - * @param iov iov of the current request > > - * @param id index of the current iov > > - * @param req_len length of the current request > > - * > > - * @return count of iov > > - */ > > -static int gen_acc_mode_iov(AspeedHACEState *s, struct iovec *iov, int id, > > - hwaddr *req_len) > > -{ > > - uint32_t pad_offset; > > - uint32_t total_msg_len; > > - s->total_req_len += *req_len; > > - > > - if (has_padding(s, &iov[id], *req_len, &total_msg_len, &pad_offset)) { > > - if (s->iov_count) { > > - return reconstruct_iov(s, iov, id, &pad_offset); > > - } > > - > > - *req_len -= s->total_req_len - total_msg_len; > > - s->total_req_len = 0; > > - iov[id].iov_len = *req_len; > > - } else { > > - s->iov_cache[s->iov_count].iov_base = iov->iov_base; > > - s->iov_cache[s->iov_count].iov_len = *req_len; > > - ++s->iov_count; > > - } > > - > > - return id + 1; > > -} > > - > > 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; > > + Error *local_err = NULL; > > + > > + if (acc_mode && s->hash_ctx == NULL) { > > + s->hash_ctx = qcrypto_hash_new(algo, &local_err); > > + if (s->hash_ctx == NULL) { > > + qemu_log_mask(LOG_GUEST_ERROR, "qcrypto hash failed : > %s", > > + error_get_pretty(local_err)); > > + error_free(local_err); > > + return; > > + } > > + } > > > > if (sg_mode) { > > uint32_t len = 0; > > @@ -226,8 +206,16 @@ static void do_hash_operation(AspeedHACEState *s, > int algo, bool sg_mode, > > } > > iov[i].iov_base = haddr; > > if (acc_mode) { > > - niov = gen_acc_mode_iov(s, iov, i, &plen); > > - > > + s->total_req_len += plen; > > + > > + if (has_padding(s, &iov[i], plen, &total_msg_len, > > + &pad_offset)) { > > + /* Padding being present indicates the final > request */ > > + sg_acc_mode_final_request = true; > > + iov[i].iov_len = pad_offset; > > + } else { > > + iov[i].iov_len = plen; > > + } > > } else { > > iov[i].iov_len = plen; > > } > > @@ -252,21 +240,42 @@ static void do_hash_operation(AspeedHACEState > *s, int algo, bool sg_mode, > > * required to check whether cache is empty. If no, we > should > > * combine cached iov and the current iov. > > */ > > - uint32_t total_msg_len; > > - uint32_t pad_offset; > > s->total_req_len += len; > > if (has_padding(s, iov, len, &total_msg_len, &pad_offset)) { > > - niov = reconstruct_iov(s, iov, 0, &pad_offset); > > + i = reconstruct_iov(s, iov, 0, &pad_offset); > > } > > } > > } > > > > - if (niov) { > > - i = niov; > > - } > > + if (acc_mode) { > > + if (qcrypto_hash_updatev(s->hash_ctx, iov, i, &local_err) < 0) { > > + qemu_log_mask(LOG_GUEST_ERROR, "qcrypto hash update > failed : %s", > > + error_get_pretty(local_err)); > > + error_free(local_err); > > + return; > > + } > > + > > + if (sg_acc_mode_final_request) { > > + if (qcrypto_hash_finalize_bytes(s->hash_ctx, &digest_buf, > > + &digest_len, > &local_err)) { > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "qcrypto hash finalize failed : %s", > > + error_get_pretty(local_err)); > > + error_free(local_err); > > + local_err = NULL; > > + } > > > > - if (qcrypto_hash_bytesv(algo, iov, i, &digest_buf, &digest_len, NULL) < > 0) { > > - qemu_log_mask(LOG_GUEST_ERROR, "%s: qcrypto failed\n", > __func__); > > + 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, &local_err) < 0) { > > + qemu_log_mask(LOG_GUEST_ERROR, "qcrypto hash bytesv > failed : %s", > > + error_get_pretty(local_err)); > > + error_free(local_err); > > return; > > } > > > > @@ -397,6 +406,11 @@ static void aspeed_hace_reset(DeviceState *dev) > > { > > struct AspeedHACEState *s = ASPEED_HACE(dev); > > > > + if (s->hash_ctx != NULL) { > > + qcrypto_hash_free(s->hash_ctx); > > + s->hash_ctx = NULL; > > + } > > + > > memset(s->regs, 0, sizeof(s->regs)); > > s->iov_count = 0; > > s->total_req_len = 0; Test Steps: 1. Create a random 32MB file dd if=/dev/random of=32MB count=32 bs=1M 2. Get the hash value in my host machine $ sha256sum 32MB 1ddcccdba742d762e2b8da0bceaf4778727c5eba54a24d7ae0c573c65414f736 32MB $ sha384sum 32MB 825d9b24bb797695545b3cbd2f373b9738627c7a1878e620415570a57c7faed77916d47084c954254f101fc0f10c0591 32MB $ sha512sum 32MB b5ae725b2dc1e521f48eae37dd82c3d5fc94f7acb5fff3dabf1caa4bb4b5bcfb498e7cc1fbaa97dda2664bff99f9f8e778f823e95afaf76fbf0985181522e478 32MB 3. Test HACE model with u-boot hash command a. load test file to address 83000000 via tftp ast# tftp 83000000 jamin_lin/32MB b. get sha256 ast# hash sha256 83000000 2000000 sha256 for 83000000 ... 84ffffff ==> 1ddcccdba742d762e2b8da0bceaf4778727c5eba54a24d7ae0c573c65414f736 c. get sha384 ast# hash sha384 83000000 2000000 sha384 for 83000000 ... 84ffffff ==> 825d9b24bb797695545b3cbd2f373b9738627c7a1878e620415570a57c7faed77916d47084c954254f101fc0f10c0591 d. get sha512 ast# hash sha512 83000000 2000000 sha512 for 83000000 ... 84ffffff ==> b5ae725b2dc1e521f48eae37dd82c3d5fc94f7acb5fff3dabf1caa4bb4b5bcfb498e7cc1fbaa97dda2664bff99f9f8e778f823e95afaf76fbf0985181522e478 Reviewed-by: Jamin Lin <jamin_lin@aspeedtech.com> Thanks-Jamin
On Wed, 16 Oct 2024 at 01:23, Jamin Lin <jamin_lin@aspeedtech.com> wrote: > 3. Test HACE model with u-boot hash command > a. load test file to address 83000000 via tftp > ast# tftp 83000000 jamin_lin/32MB > b. get sha256 > ast# hash sha256 83000000 2000000 > sha256 for 83000000 ... 84ffffff ==> 1ddcccdba742d762e2b8da0bceaf4778727c5eba54a24d7ae0c573c65414f736 > c. get sha384 > ast# hash sha384 83000000 2000000 > sha384 for 83000000 ... 84ffffff ==> 825d9b24bb797695545b3cbd2f373b9738627c7a1878e620415570a57c7faed77916d47084c954254f101fc0f10c0591 > d. get sha512 > ast# hash sha512 83000000 2000000 > sha512 for 83000000 ... 84ffffff ==> b5ae725b2dc1e521f48eae37dd82c3d5fc94f7acb5fff3dabf1caa4bb4b5bcfb498e7cc1fbaa97dda2664bff99f9f8e778f823e95afaf76fbf0985181522e478 I attempted this same test and noticed that the 'hash' command was not using the hardware. You can see this by putting some printf or breakpoint in eg hw/misc/aspeed_hace.c do_hash_operation. There's some missing work on the u-boot side to move the "hash" command over to the hash uclass, so it can be used to test this code path (or add support for the old API to the hace driver). Separately, I attempted to test with u-boot by enabling hash verification of the FIT image, and it fails to calculate the correct SHA. I think to have any confidence that this model works, we need to add some testing to qemu. I did this for the initial version of the model in tests/qtest/aspeed_hace-test.c. The upstream u-boot situation is a mess, and cannot be used to exercise the qemu model at this stage. Cheers, Joel
On 10/22/24 13:54, Joel Stanley wrote: > On Wed, 16 Oct 2024 at 01:23, Jamin Lin <jamin_lin@aspeedtech.com> wrote: > >> 3. Test HACE model with u-boot hash command >> a. load test file to address 83000000 via tftp >> ast# tftp 83000000 jamin_lin/32MB >> b. get sha256 >> ast# hash sha256 83000000 2000000 >> sha256 for 83000000 ... 84ffffff ==> 1ddcccdba742d762e2b8da0bceaf4778727c5eba54a24d7ae0c573c65414f736 >> c. get sha384 >> ast# hash sha384 83000000 2000000 >> sha384 for 83000000 ... 84ffffff ==> 825d9b24bb797695545b3cbd2f373b9738627c7a1878e620415570a57c7faed77916d47084c954254f101fc0f10c0591 >> d. get sha512 >> ast# hash sha512 83000000 2000000 >> sha512 for 83000000 ... 84ffffff ==> b5ae725b2dc1e521f48eae37dd82c3d5fc94f7acb5fff3dabf1caa4bb4b5bcfb498e7cc1fbaa97dda2664bff99f9f8e778f823e95afaf76fbf0985181522e478 > > I attempted this same test and noticed that the 'hash' command was not > using the hardware. You can see this by putting some printf or > breakpoint in eg hw/misc/aspeed_hace.c do_hash_operation. There's some > missing work on the u-boot side to move the "hash" command over to the > hash uclass, so it can be used to test this code path (or add support > for the old API to the hace driver). > > Separately, I attempted to test with u-boot by enabling hash > verification of the FIT image, and it fails to calculate the correct > SHA. > > I think to have any confidence that this model works, we need to add > some testing to qemu. I did this for the initial version of the model > in tests/qtest/aspeed_hace-test.c. There are "accumulative mode" tests in QEMU, which were added by commit e0c371a0d23b ("tests/qtest: Add test for Aspeed HACE accumulative mode") They pass today with this patch. Are you suggesting we should add more? Thanks, C. > The upstream u-boot situation is a mess, and cannot be used to > exercise the qemu model at this stage. > > Cheers, > > Joel >
On Wed, 23 Oct 2024 at 02:35, Cédric Le Goater <clg@redhat.com> wrote: > > On 10/22/24 13:54, Joel Stanley wrote: > > On Wed, 16 Oct 2024 at 01:23, Jamin Lin <jamin_lin@aspeedtech.com> wrote: > > > >> 3. Test HACE model with u-boot hash command > >> a. load test file to address 83000000 via tftp > >> ast# tftp 83000000 jamin_lin/32MB > >> b. get sha256 > >> ast# hash sha256 83000000 2000000 > >> sha256 for 83000000 ... 84ffffff ==> 1ddcccdba742d762e2b8da0bceaf4778727c5eba54a24d7ae0c573c65414f736 > >> c. get sha384 > >> ast# hash sha384 83000000 2000000 > >> sha384 for 83000000 ... 84ffffff ==> 825d9b24bb797695545b3cbd2f373b9738627c7a1878e620415570a57c7faed77916d47084c954254f101fc0f10c0591 > >> d. get sha512 > >> ast# hash sha512 83000000 2000000 > >> sha512 for 83000000 ... 84ffffff ==> b5ae725b2dc1e521f48eae37dd82c3d5fc94f7acb5fff3dabf1caa4bb4b5bcfb498e7cc1fbaa97dda2664bff99f9f8e778f823e95afaf76fbf0985181522e478 > > > > I attempted this same test and noticed that the 'hash' command was not > > using the hardware. You can see this by putting some printf or > > breakpoint in eg hw/misc/aspeed_hace.c do_hash_operation. There's some > > missing work on the u-boot side to move the "hash" command over to the > > hash uclass, so it can be used to test this code path (or add support > > for the old API to the hace driver). > > > > Separately, I attempted to test with u-boot by enabling hash > > verification of the FIT image, and it fails to calculate the correct > > SHA. > > > > I think to have any confidence that this model works, we need to add > > some testing to qemu. I did this for the initial version of the model > > in tests/qtest/aspeed_hace-test.c. > > There are "accumulative mode" tests in QEMU, which were added by commit > e0c371a0d23b ("tests/qtest: Add test for Aspeed HACE accumulative mode") > They pass today with this patch. Are you suggesting we should add more? I was trying to find a test case that showed the behaviour was broken before (segfault?) and fixed after, but haven't had any luck so far. The tests pass before this patch, and they pass after it. By that logic there's no problem merging this one: Reviewed-by: Joel Stanley <joel@jms.id.au> Someone (aspeed?) should take a todo to resolve the HACE situation in u-boot. Cheers, Joel
On 10/23/24 08:52, Joel Stanley wrote: > On Wed, 23 Oct 2024 at 02:35, Cédric Le Goater <clg@redhat.com> wrote: >> >> On 10/22/24 13:54, Joel Stanley wrote: >>> On Wed, 16 Oct 2024 at 01:23, Jamin Lin <jamin_lin@aspeedtech.com> wrote: >>> >>>> 3. Test HACE model with u-boot hash command >>>> a. load test file to address 83000000 via tftp >>>> ast# tftp 83000000 jamin_lin/32MB >>>> b. get sha256 >>>> ast# hash sha256 83000000 2000000 >>>> sha256 for 83000000 ... 84ffffff ==> 1ddcccdba742d762e2b8da0bceaf4778727c5eba54a24d7ae0c573c65414f736 >>>> c. get sha384 >>>> ast# hash sha384 83000000 2000000 >>>> sha384 for 83000000 ... 84ffffff ==> 825d9b24bb797695545b3cbd2f373b9738627c7a1878e620415570a57c7faed77916d47084c954254f101fc0f10c0591 >>>> d. get sha512 >>>> ast# hash sha512 83000000 2000000 >>>> sha512 for 83000000 ... 84ffffff ==> b5ae725b2dc1e521f48eae37dd82c3d5fc94f7acb5fff3dabf1caa4bb4b5bcfb498e7cc1fbaa97dda2664bff99f9f8e778f823e95afaf76fbf0985181522e478 >>> >>> I attempted this same test and noticed that the 'hash' command was not >>> using the hardware. You can see this by putting some printf or >>> breakpoint in eg hw/misc/aspeed_hace.c do_hash_operation. There's some >>> missing work on the u-boot side to move the "hash" command over to the >>> hash uclass, so it can be used to test this code path (or add support >>> for the old API to the hace driver). >>> >>> Separately, I attempted to test with u-boot by enabling hash >>> verification of the FIT image, and it fails to calculate the correct >>> SHA. >>> >>> I think to have any confidence that this model works, we need to add >>> some testing to qemu. I did this for the initial version of the model >>> in tests/qtest/aspeed_hace-test.c. >> >> There are "accumulative mode" tests in QEMU, which were added by commit >> e0c371a0d23b ("tests/qtest: Add test for Aspeed HACE accumulative mode") >> They pass today with this patch. Are you suggesting we should add more? > > I was trying to find a test case that showed the behaviour was broken > before (segfault?) and fixed after, but haven't had any luck so far. > > The tests pass before this patch, and they pass after it. By that > logic there's no problem merging this one: > > Reviewed-by: Joel Stanley <joel@jms.id.au> Thanks, > > Someone (aspeed?) should take a todo to resolve the HACE situation in u-boot. I will build a br2 image with upstream u-boot. The ones we use for tests have an OpenBMC u-boot IIRC. Cheers, C.
Hi Joel and Cedric > Subject: Re: [PATCH v6] hw/misc/aspeed_hace: Fix SG Accumulative hashing > > On 10/23/24 08:52, Joel Stanley wrote: > > On Wed, 23 Oct 2024 at 02:35, Cédric Le Goater <clg@redhat.com> wrote: > >> > >> On 10/22/24 13:54, Joel Stanley wrote: > >>> On Wed, 16 Oct 2024 at 01:23, Jamin Lin <jamin_lin@aspeedtech.com> > wrote: > >>> > >>>> 3. Test HACE model with u-boot hash command a. load test file to > >>>> address 83000000 via tftp ast# tftp 83000000 jamin_lin/32MB b. get > >>>> sha256 ast# hash sha256 83000000 2000000 > >>>> sha256 for 83000000 ... 84ffffff ==> > >>>> > 1ddcccdba742d762e2b8da0bceaf4778727c5eba54a24d7ae0c573c65414f736 > >>>> c. get sha384 > >>>> ast# hash sha384 83000000 2000000 > >>>> sha384 for 83000000 ... 84ffffff ==> > >>>> > 825d9b24bb797695545b3cbd2f373b9738627c7a1878e620415570a57c7faed77 > 91 > >>>> 6d47084c954254f101fc0f10c0591 > >>>> d. get sha512 > >>>> ast# hash sha512 83000000 2000000 > >>>> sha512 for 83000000 ... 84ffffff ==> > >>>> > b5ae725b2dc1e521f48eae37dd82c3d5fc94f7acb5fff3dabf1caa4bb4b5bcfb498 > >>>> e7cc1fbaa97dda2664bff99f9f8e778f823e95afaf76fbf0985181522e478 > >>> > >>> I attempted this same test and noticed that the 'hash' command was > >>> not using the hardware. You can see this by putting some printf or > >>> breakpoint in eg hw/misc/aspeed_hace.c do_hash_operation. There's > >>> some missing work on the u-boot side to move the "hash" command over > >>> to the hash uclass, so it can be used to test this code path (or add > >>> support for the old API to the hace driver). > >>> > >>> Separately, I attempted to test with u-boot by enabling hash > >>> verification of the FIT image, and it fails to calculate the correct > >>> SHA. > >>> > >>> I think to have any confidence that this model works, we need to add > >>> some testing to qemu. I did this for the initial version of the > >>> model in tests/qtest/aspeed_hace-test.c. > >> > >> There are "accumulative mode" tests in QEMU, which were added by > >> commit e0c371a0d23b ("tests/qtest: Add test for Aspeed HACE > >> accumulative mode") They pass today with this patch. Are you suggesting > we should add more? > > > > I was trying to find a test case that showed the behaviour was broken > > before (segfault?) and fixed after, but haven't had any luck so far. > > > > The tests pass before this patch, and they pass after it. By that > > logic there's no problem merging this one: > > > > Reviewed-by: Joel Stanley <joel@jms.id.au> > > Thanks, > > > > > Someone (aspeed?) should take a todo to resolve the HACE situation in > u-boot. > > I will build a br2 image with upstream u-boot. The ones we use for tests have > an OpenBMC u-boot IIRC. > I used ASPEED FORKED OpenBMC pre-built image and hardware hash engine was enabled by default with u-boot hash command. https://github.com/AspeedTech-BMC/openbmc/releases/download/v09.03/ast2600-default-obmc.tar.gz https://github.com/AspeedTech-BMC/u-boot/blob/aspeed-master-v2019.04/arch/arm/mach-aspeed/ast2600/spl.c#L46 I added "printf" in "do_hash_operation" function and ensure this function was called in HACE model. ast# hash sha256 83000000 8000000 jamin do_hash_operation jamin do_hash_operation: qcrypto_hash_new jamin do_hash_operation: acc_mode sha256 for 83000000 ... 8affffff ==> 652dcd794aabcbde2fe4480398ad5da3917cfb90d26baa64910fc4bea4471646 Thanks-Jamin > Cheers, > > C.
On Wed, 23 Oct 2024 at 19:50, Jamin Lin <jamin_lin@aspeedtech.com> wrote: > > > Someone (aspeed?) should take a todo to resolve the HACE situation in > > u-boot. > > > > I will build a br2 image with upstream u-boot. The ones we use for tests have > > an OpenBMC u-boot IIRC. > > > > I used ASPEED FORKED OpenBMC pre-built image and hardware hash engine was enabled by default with u-boot hash command. > https://github.com/AspeedTech-BMC/openbmc/releases/download/v09.03/ast2600-default-obmc.tar.gz > https://github.com/AspeedTech-BMC/u-boot/blob/aspeed-master-v2019.04/arch/arm/mach-aspeed/ast2600/spl.c#L46 > I added "printf" in "do_hash_operation" function and ensure this function was called in HACE model. Thanks for clarifying Jamin. It would be great if these u-boot changes could be submitted to mainline u-boot. Cheers, Joel
diff --git a/include/hw/misc/aspeed_hace.h b/include/hw/misc/aspeed_hace.h index ecb1b67de816..4af99191955a 100644 --- a/include/hw/misc/aspeed_hace.h +++ b/include/hw/misc/aspeed_hace.h @@ -1,6 +1,7 @@ /* * ASPEED Hash and Crypto Engine * + * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates * Copyright (C) 2021 IBM Corp. * * SPDX-License-Identifier: GPL-2.0-or-later @@ -10,6 +11,7 @@ #define ASPEED_HACE_H #include "hw/sysbus.h" +#include "crypto/hash.h" #define TYPE_ASPEED_HACE "aspeed.hace" #define TYPE_ASPEED_AST2400_HACE TYPE_ASPEED_HACE "-ast2400" @@ -35,6 +37,8 @@ struct AspeedHACEState { MemoryRegion *dram_mr; AddressSpace dram_as; + + QCryptoHash *hash_ctx; }; diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c index b6f43f65b29a..bc1d66ad8064 100644 --- a/hw/misc/aspeed_hace.c +++ b/hw/misc/aspeed_hace.c @@ -1,6 +1,7 @@ /* * ASPEED Hash and Crypto Engine * + * Copyright (c) 2024 Seagate Technology LLC and/or its Affiliates * Copyright (C) 2021 IBM Corp. * * Joel Stanley <joel@jms.id.au> @@ -151,49 +152,28 @@ static int reconstruct_iov(AspeedHACEState *s, struct iovec *iov, int id, return iov_count; } -/** - * Generate iov for accumulative mode. - * - * @param s aspeed hace state object - * @param iov iov of the current request - * @param id index of the current iov - * @param req_len length of the current request - * - * @return count of iov - */ -static int gen_acc_mode_iov(AspeedHACEState *s, struct iovec *iov, int id, - hwaddr *req_len) -{ - uint32_t pad_offset; - uint32_t total_msg_len; - s->total_req_len += *req_len; - - if (has_padding(s, &iov[id], *req_len, &total_msg_len, &pad_offset)) { - if (s->iov_count) { - return reconstruct_iov(s, iov, id, &pad_offset); - } - - *req_len -= s->total_req_len - total_msg_len; - s->total_req_len = 0; - iov[id].iov_len = *req_len; - } else { - s->iov_cache[s->iov_count].iov_base = iov->iov_base; - s->iov_cache[s->iov_count].iov_len = *req_len; - ++s->iov_count; - } - - return id + 1; -} - 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; + Error *local_err = NULL; + + if (acc_mode && s->hash_ctx == NULL) { + s->hash_ctx = qcrypto_hash_new(algo, &local_err); + if (s->hash_ctx == NULL) { + qemu_log_mask(LOG_GUEST_ERROR, "qcrypto hash failed : %s", + error_get_pretty(local_err)); + error_free(local_err); + return; + } + } if (sg_mode) { uint32_t len = 0; @@ -226,8 +206,16 @@ static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode, } iov[i].iov_base = haddr; if (acc_mode) { - niov = gen_acc_mode_iov(s, iov, i, &plen); - + s->total_req_len += plen; + + if (has_padding(s, &iov[i], plen, &total_msg_len, + &pad_offset)) { + /* Padding being present indicates the final request */ + sg_acc_mode_final_request = true; + iov[i].iov_len = pad_offset; + } else { + iov[i].iov_len = plen; + } } else { iov[i].iov_len = plen; } @@ -252,21 +240,42 @@ static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode, * required to check whether cache is empty. If no, we should * combine cached iov and the current iov. */ - uint32_t total_msg_len; - uint32_t pad_offset; s->total_req_len += len; if (has_padding(s, iov, len, &total_msg_len, &pad_offset)) { - niov = reconstruct_iov(s, iov, 0, &pad_offset); + i = reconstruct_iov(s, iov, 0, &pad_offset); } } } - if (niov) { - i = niov; - } + if (acc_mode) { + if (qcrypto_hash_updatev(s->hash_ctx, iov, i, &local_err) < 0) { + qemu_log_mask(LOG_GUEST_ERROR, "qcrypto hash update failed : %s", + error_get_pretty(local_err)); + error_free(local_err); + return; + } + + if (sg_acc_mode_final_request) { + if (qcrypto_hash_finalize_bytes(s->hash_ctx, &digest_buf, + &digest_len, &local_err)) { + qemu_log_mask(LOG_GUEST_ERROR, + "qcrypto hash finalize failed : %s", + error_get_pretty(local_err)); + error_free(local_err); + local_err = NULL; + } - if (qcrypto_hash_bytesv(algo, iov, i, &digest_buf, &digest_len, NULL) < 0) { - qemu_log_mask(LOG_GUEST_ERROR, "%s: qcrypto failed\n", __func__); + 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, &local_err) < 0) { + qemu_log_mask(LOG_GUEST_ERROR, "qcrypto hash bytesv failed : %s", + error_get_pretty(local_err)); + error_free(local_err); return; } @@ -397,6 +406,11 @@ static void aspeed_hace_reset(DeviceState *dev) { struct AspeedHACEState *s = ASPEED_HACE(dev); + if (s->hash_ctx != NULL) { + qcrypto_hash_free(s->hash_ctx); + s->hash_ctx = NULL; + } + memset(s->regs, 0, sizeof(s->regs)); s->iov_count = 0; s->total_req_len = 0;