Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Use-after-free of TX and RX work queues during HWC channel destruction due to incorrect teardown ordering. - [High] Data corruption and premature completion due to msg_id reuse after timeout in mana_hwc_send_request(). - [High] Use-after-free of gdma_context (gc) due to un-canceled service work during device removal. -- --- Patch [3]: [PATCH net-next 3/7] net: mana: free HWC comp_buf after destroying the EQ --- commit 0772412f368270fa1d8812b8355a2560f063690d Author: Long Li <[email protected]> Subject: net: mana: free HWC comp_buf after destroying the EQ This patch resolves a use-after-free issue by deferring the release of the hardware channel completion buffer until after the event queue has been destroyed. This ensures no late interrupts can access the freed memory. > diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c > b/drivers/net/ethernet/microsoft/mana/hw_channel.c > --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c > +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c [ ... ] > static void mana_hwc_destroy_cq(struct gdma_context *gc, struct hwc_cq > *hwc_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 while looking at teardown ordering, could there be a similar use-after-free with the TX and RX work queues? In mana_hwc_destroy_channel(), the work queues are freed before the completion queue: 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); } If an interrupt fires before the CQ and EQ are destroyed, will the completion handler still attempt to dereference the already freed work queues? mana_hwc_rx_event_handler() { struct hw_channel_context *hwc = ctx; struct hwc_wq *hwc_rxq = hwc->rxq; ... rq = hwc_rxq->gdma_wq; ... } [Severity: High] This is a pre-existing issue, but does mana_hwc_send_request() have a race condition if a request times out? If a timeout occurs, mana_hwc_send_request() releases the msg_id while the hardware might still be processing it: mana_hwc_send_request() { ... err = -ETIMEDOUT; goto out; ... out: mana_hwc_put_msg_index(hwc, msg_id); } Could another caller subsequently allocate the same msg_id and assign its own stack buffer? If the hardware then completes the original delayed request, would the handler overwrite the new caller's buffer with the old response? mana_hwc_handle_resp() { ctx = hwc->caller_ctx + msg_id; ... memcpy(ctx->output_buf, resp_msg, resp_len); ... complete(&ctx->comp_event); } [Severity: High] This is also a pre-existing issue, but could there be a use-after-free during device removal if service work is pending? In mana_gd_remove(), the gdma_context is freed without canceling pending work on the system workqueue: mana_gd_remove() { ... mana_gd_cleanup_device(pdev); xa_destroy(&gc->irq_contexts); pci_iounmap(pdev, gc->bar0_va); vfree(gc); } If the delayed work handler executes concurrently with or after device removal, will it dereference the freed pointer? mana_serv_reset() { struct gdma_context *gc = pci_get_drvdata(pdev); struct hw_channel_context *hwc; ... hwc = gc->hwc.driver_data; } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
