Hi Kane, > Subject: [PATCH v1 1/1] aspeed/hace: Fix mapped address may not be > unmapped issue > > In the do_hash_operation, the code may be returned earlier because > hash_prepare_sg_iov or hash_prepare_direct_iov may return a failure. > When this condition is happened, current code flow doesn't go through later > code segments. Finally, it causes the mapped address isn't unmapped properly. > > This change is intended to unmap the mapped address if there is any error is > occurred. Thus, it could prevent a reasoure leak situation. > > Signed-off-by: Kane-Chen-AS <[email protected]> > --- > hw/misc/aspeed_hace.c | 63 ++++++++++++++++++++++++++++++------------- > 1 file changed, 45 insertions(+), 18 deletions(-) > > diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c index > 0504d61cbf..a322905cb3 100644 > --- a/hw/misc/aspeed_hace.c > +++ b/hw/misc/aspeed_hace.c > @@ -218,8 +218,19 @@ static bool hash_accumulate_len(AspeedHACEState > *s, hwaddr plen) > return true; > } > > +static void hash_iov_unmap(AspeedHACEState *s, struct iovec *iov, > + hwaddr *mapped_lens, int iov_count) { > + for (; iov_count > 0; iov_count--) { > + address_space_unmap(&s->dram_as, iov[iov_count - 1].iov_base, > + mapped_lens[iov_count - 1], false, > + mapped_lens[iov_count - 1]); > + } > +} > + > static int hash_prepare_direct_iov(AspeedHACEState *s, struct iovec *iov, > - bool acc_mode, bool > *acc_final_request) > + bool acc_mode, bool > *acc_final_request, > + hwaddr *mapped_lens) > { > uint32_t total_msg_len; > uint32_t pad_offset; > @@ -243,9 +254,11 @@ static int hash_prepare_direct_iov(AspeedHACEState > *s, struct iovec *iov, > > iov[0].iov_base = haddr; > iov_idx = 1; > + mapped_lens[0] = plen; > > if (acc_mode) { > if (!hash_accumulate_len(s, plen)) { > + hash_iov_unmap(s, iov, mapped_lens, 1); > return -1; > } > > @@ -265,7 +278,8 @@ static int hash_prepare_direct_iov(AspeedHACEState > *s, struct iovec *iov, } > > static int hash_prepare_sg_iov(AspeedHACEState *s, struct iovec *iov, > - bool acc_mode, bool > *acc_final_request) > + bool acc_mode, bool > *acc_final_request, > + hwaddr *mapped_lens) > { > uint32_t total_msg_len; > uint32_t pad_offset; > @@ -275,6 +289,7 @@ static int hash_prepare_sg_iov(AspeedHACEState *s, > struct iovec *iov, > int iov_idx; > hwaddr plen; > void *haddr; > + int iov_mapped = 0; > > src = hash_get_source_addr(s); > for (iov_idx = 0; !(len & SG_LIST_LEN_LAST); iov_idx++) { @@ -282,7 > +297,7 @@ static int hash_prepare_sg_iov(AspeedHACEState *s, struct iovec > *iov, > qemu_log_mask(LOG_GUEST_ERROR, > "%s: Failed to set end of sg list marker\n", > __func__); > - return -1; > + goto fail; > } > > len = address_space_ldl_le(&s->dram_as, src, @@ -307,15 +322,17 > @@ static int hash_prepare_sg_iov(AspeedHACEState *s, struct iovec *iov, > "%s: Unable to map address, sg_addr=0x%x, " > "plen=0x%" HWADDR_PRIx "\n", > __func__, sg_addr, plen); > - return -1; > + goto fail; > } > > src += SG_LIST_ENTRY_SIZE; > > iov[iov_idx].iov_base = haddr; > + iov_mapped = iov_idx + 1; > + mapped_lens[iov_idx] = plen; > if (acc_mode) { > if (!hash_accumulate_len(s, plen)) { > - return -1; > + goto fail; > } > > if (has_padding(s, &iov[iov_idx], plen, &total_msg_len, @@ > -332,6 +349,10 @@ static int hash_prepare_sg_iov(AspeedHACEState *s, > struct iovec *iov, > } > > return iov_idx; > + > +fail: > + hash_iov_unmap(s, iov, mapped_lens, iov_mapped); > + return -1; > } > > static uint64_t hash_get_digest_addr(AspeedHACEState *s) @@ -350,6 > +371,7 @@ static uint64_t hash_get_digest_addr(AspeedHACEState *s) > static void hash_write_digest_and_unmap_iov(AspeedHACEState *s, > struct iovec *iov, > int iov_idx, > + hwaddr *mapped_lens, > uint8_t *digest_buf, > size_t digest_len) > { @@ -369,15 +391,12 @@ static void > hash_write_digest_and_unmap_iov(AspeedHACEState *s, > hace_hexdump("digest", (char *)digest_buf, digest_len); > } > > - for (; iov_idx > 0; iov_idx--) { > - address_space_unmap(&s->dram_as, iov[iov_idx - 1].iov_base, > - iov[iov_idx - 1].iov_len, false, > - iov[iov_idx - 1].iov_len); > - } > + hash_iov_unmap(s, iov, mapped_lens, iov_idx); > } > > static void hash_execute_non_acc_mode(AspeedHACEState *s, int algo, > - struct iovec *iov, int iov_idx) > + struct iovec *iov, int iov_idx, > + hwaddr *mapped_lens) > { > g_autofree uint8_t *digest_buf = NULL; > Error *local_err = NULL; > @@ -389,15 +408,17 @@ static void > hash_execute_non_acc_mode(AspeedHACEState *s, int algo, > "%s: qcrypto hash bytesv failed : %s", > __func__, error_get_pretty(local_err)); > error_free(local_err); > + hash_iov_unmap(s, iov, mapped_lens, iov_idx); > return; > } > > - hash_write_digest_and_unmap_iov(s, iov, iov_idx, digest_buf, > digest_len); > + hash_write_digest_and_unmap_iov(s, iov, iov_idx, mapped_lens, > + digest_buf, digest_len); > } > > static void hash_execute_acc_mode(AspeedHACEState *s, int algo, > struct iovec *iov, int iov_idx, > - bool final_request) > + bool final_request, hwaddr > + *mapped_lens) > { > g_autofree uint8_t *digest_buf = NULL; > Error *local_err = NULL; > @@ -411,6 +432,7 @@ static void hash_execute_acc_mode(AspeedHACEState > *s, int algo, > qemu_log_mask(LOG_GUEST_ERROR, "%s: qcrypto hash new > failed : %s", > __func__, error_get_pretty(local_err)); > error_free(local_err); > + hash_iov_unmap(s, iov, mapped_lens, iov_idx); > return; > } > } > @@ -419,6 +441,7 @@ static void hash_execute_acc_mode(AspeedHACEState > *s, int algo, > qemu_log_mask(LOG_GUEST_ERROR, "%s: qcrypto hash updatev > failed : %s", > __func__, error_get_pretty(local_err)); > error_free(local_err); > + hash_iov_unmap(s, iov, mapped_lens, iov_idx); > return; > } > > @@ -438,22 +461,25 @@ static void > hash_execute_acc_mode(AspeedHACEState *s, int algo, > s->total_req_len = 0; > } > > - hash_write_digest_and_unmap_iov(s, iov, iov_idx, digest_buf, > digest_len); > + hash_write_digest_and_unmap_iov(s, iov, iov_idx, mapped_lens, > + digest_buf, digest_len); > } > > static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode, > bool acc_mode) { > QEMU_UNINITIALIZED struct iovec iov[ASPEED_HACE_MAX_SG]; > + hwaddr mapped_lens[ASPEED_HACE_MAX_SG] = { 0 }; > bool acc_final_request = false; > int iov_idx = -1; > > /* Prepares the iov for hashing operations based on the selected mode > */ > if (sg_mode) { > - iov_idx = hash_prepare_sg_iov(s, iov, acc_mode, > &acc_final_request); > + iov_idx = hash_prepare_sg_iov(s, iov, acc_mode, > &acc_final_request, > + mapped_lens); > } else { > iov_idx = hash_prepare_direct_iov(s, iov, acc_mode, > - &acc_final_request); > + &acc_final_request, > + mapped_lens); > } > > if (iov_idx <= 0) { > @@ -468,9 +494,10 @@ static void do_hash_operation(AspeedHACEState *s, > int algo, bool sg_mode, > > /* Executes the hash operation */ > if (acc_mode) { > - hash_execute_acc_mode(s, algo, iov, iov_idx, acc_final_request); > + hash_execute_acc_mode(s, algo, iov, iov_idx, acc_final_request, > + mapped_lens); > } else { > - hash_execute_non_acc_mode(s, algo, iov, iov_idx); > + hash_execute_non_acc_mode(s, algo, iov, iov_idx, mapped_lens); > } > } > > -- > 2.43.0
Reviewed-by: Jamin Lin <[email protected]> Thanks, Jamin
