| Message ID | 20240807195122.2827364-16-alejandro.zeise@seagate.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/misc/aspeed_hace: Fix SG Accumulative Hash Calculations | expand |
On 8/7/24 21:51, 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 > Buglink: https://github.com/openbmc/qemu/issues/36 > > Signed-off-by: Alejandro Zeise <alejandro.zeise@seagate.com> > --- > hw/misc/aspeed_hace.c | 94 +++++++++++++++++++---------------- > include/hw/misc/aspeed_hace.h | 4 ++ > 2 files changed, 54 insertions(+), 44 deletions(-) > > diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c > index c06c04ddc6..4247403d45 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,50 +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; > > + if (acc_mode && s->hash_ctx == NULL) { > + s->hash_ctx = qcrypto_hash_new(algo, NULL); I wonder if we shouldn't use an 'Error *' argument instead and output the literal error with error_get_pretty() in qemu_log_mask() ? > + if (s->hash_ctx == NULL) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: qcrypto failed to create hash context\n", > + __func__); > + return; > + } > + } > + > if (sg_mode) { > uint32_t len = 0; > > @@ -226,8 +205,15 @@ 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,20 +238,35 @@ 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, NULL) < 0) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: qcrypto hash update failed\n", __func__); > + return; This will drop the ending sequence: address_space_unmap() and setting of the IRQ status register. I guess adding an 'exit' goto label is preferable. > + } > + > + 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__); Same issue here with the ending sequence. This should be fixed in a separate patch. Thanks, C. > return; > } > @@ -397,6 +398,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; > diff --git a/include/hw/misc/aspeed_hace.h b/include/hw/misc/aspeed_hace.h > index ecb1b67de8..4af9919195 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 c06c04ddc6..4247403d45 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,50 +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; + if (acc_mode && s->hash_ctx == NULL) { + s->hash_ctx = qcrypto_hash_new(algo, NULL); + if (s->hash_ctx == NULL) { + qemu_log_mask(LOG_GUEST_ERROR, + "%s: qcrypto failed to create hash context\n", + __func__); + return; + } + } + if (sg_mode) { uint32_t len = 0; @@ -226,8 +205,15 @@ 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,20 +238,35 @@ 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, 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; } @@ -397,6 +398,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; diff --git a/include/hw/misc/aspeed_hace.h b/include/hw/misc/aspeed_hace.h index ecb1b67de8..4af9919195 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; };
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> --- hw/misc/aspeed_hace.c | 94 +++++++++++++++++++---------------- include/hw/misc/aspeed_hace.h | 4 ++ 2 files changed, 54 insertions(+), 44 deletions(-)