Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Incomplete Fix / Use-After-Free of WQ structures due to incorrect channel teardown order. - [High] Stack Memory Corruption and Logic Error due to missing reinitialization after request timeout. - [High] Hardware DMA Use-After-Free when channel establishment times out. -- --- Patch [3]: [PATCH net v7 3/7] net: mana: free HWC comp_buf after destroying the EQ --- commit 6875abc2fe553564a7ca5aa77824fb5ca62feda6 Author: Long Li <[email protected]> net: mana: free HWC comp_buf after destroying the EQ mana_hwc_destroy_cq() freed comp_buf and the CQ before the EQ. While the EQ was still registered its handler could reach comp_buf and the CQ, so a late EQE could touch freed memory. This commit fixes the teardown order. > diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c > b/drivers/net/ethernet/microsoft/mana/hw_channel.c > index ccef9bf9c6bfd..7e01596df11b6 100644 > --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c > +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c > @@ -384,14 +384,17 @@ static void mana_hwc_comp_event(void *ctx, struct > gdma_queue *q_self) > > static void mana_hwc_destroy_cq(struct gdma_context *gc, struct hwc_cq > *hwc_cq) > { > - kfree(hwc_cq->comp_buf); > + /* Destroy the EQ first: it deregisters the IRQ and drains in-flight > + * handlers, so none can touch the CQ after it is freed. > + */ > + if (hwc_cq->gdma_eq) > + mana_gd_destroy_queue(gc, hwc_cq->gdma_eq); > > + /* Safe to free now that the EQ handler is fenced. */ > if (hwc_cq->gdma_cq) > mana_gd_destroy_queue(gc, hwc_cq->gdma_cq); > > - if (hwc_cq->gdma_eq) > - mana_gd_destroy_queue(gc, hwc_cq->gdma_eq); > - > + kfree(hwc_cq->comp_buf); > kfree(hwc_cq); > } [Severity: High] This isn't a bug introduced by this patch, but does the teardown order in mana_hwc_destroy_channel() leave a use-after-free for the WQ structures? In mana_hwc_destroy_channel(), the txq and rxq wrappers are destroyed before mana_hwc_destroy_cq() fences the EQ: mana_hwc_destroy_channel() { ... if (hwc->txq) mana_hwc_destroy_wq(hwc, hwc->txq); if (hwc->rxq) mana_hwc_destroy_wq(hwc, hwc->rxq); if (hwc->cq) mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq); ... } Since mana_hwc_destroy_wq() frees the wq structure, can an unfenced EQ handler dereference the freed hwc->rxq pointer in mana_hwc_rx_event_handler() while the EQ is still active? [Severity: High] This is a pre-existing issue, but does mana_hwc_send_request() have a stack corruption bug if a request times out? If wait_for_completion_timeout() times out, the function jumps to out and releases the msg_id: mana_hwc_send_request() { ... if (!wait_for_completion_timeout(&ctx->comp_event, ...)) { err = -ETIMEDOUT; goto out; } ... } A new request could then allocate the same msg_id without reinitializing ctx->comp_event. If a late response arrives for the original request, mana_hwc_handle_resp() will copy the stale payload into the new caller's stack buffer and complete the event: mana_hwc_handle_resp() { ... memcpy(ctx->output_buf, resp_msg, resp_len); out: ctx->error = err; ... complete(&ctx->comp_event); } Could this cause the new request to receive stale data and continue prematurely? [Severity: High] This is a pre-existing issue, but does a timeout during channel establishment result in a DMA use-after-free? In mana_hwc_establish_channel(), if the hardware takes too long to reply: mana_hwc_establish_channel() { ... if (!wait_for_completion_timeout(&hwc->hwc_init_eqe_comp, 60 * HZ)) return -ETIMEDOUT; ... } The error path calls mana_hwc_destroy_channel(), which checks max_num_cqs before tearing down the hardware channel: mana_hwc_destroy_channel() { ... if (gc->max_num_cqs > 0) mana_smc_teardown_hwc(&gc->shm_channel, false); ... } Since max_num_cqs remains 0 during an init timeout, mana_smc_teardown_hwc() is skipped. Are the DMA buffers then freed while the hardware channel is still active and capable of writing to them? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
