Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
Pre-existing issues: - [High] Use-After-Free of WQs during HWC channel destruction because WQs are destroyed before the EQ and CQ. - [High] Immediate reuse of `msg_id` after a timeout leads to cross-request stack corruption. - [High] Pending `mana_serv_work` items on the global workqueue are not canceled during device removal, causing a Use-After-Free. - [High] Missing bounds check on untrusted `inline_oob_size_div4` allows out-of-bounds memory read. - [High] Missing length check on `tx_oob_data_size` allows spoofed completions via stale DMA buffer data. - [Medium] Destroying the CQ before the EQ triggers a `WARN_ON_ONCE(!cq)` splat if an EQ interrupt fires during the race window. -- --- Patch [3]: [PATCH net-next v2 3/7] net: mana: free HWC comp_buf after destroying the EQ --- commit 6326f43efebe30510f1377a79fcf8a92f5abb2b7 Author: Long Li <[email protected]> net: mana: free HWC comp_buf after destroying the EQ This commit changes mana_hwc_destroy_cq() to free hwc_cq->comp_buf after destroying the CQ and EQ. This prevents a use-after-free where a late EQ interrupt handler could touch comp_buf while the EQ was still registered. > diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c > b/drivers/net/ethernet/microsoft/mana/hw_channel.c > index 3f011ebbe7b3e..2239fdeda57cb 100644 > --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c > +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c [ ... ] > @@ -384,14 +384,20 @@ 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); > - > if (hwc_cq->gdma_cq) > mana_gd_destroy_queue(gc, hwc_cq->gdma_cq); [Severity: Medium] This isn't a bug introduced by this patch, but destroying the CQ before the EQ appears to leave a race window for EQ interrupts. If a hardware interrupt fires for the EQ, could mana_gd_process_eqe() read a completion event, look up the CQ ID in the table, find NULL, and trigger the WARN_ON_ONCE(!cq) splat? > > + /* comp_buf is reached only by mana_hwc_comp_event(), which the [ ... ] > if (hwc_cq->gdma_eq) > mana_gd_destroy_queue(gc, hwc_cq->gdma_eq); > > + kfree(hwc_cq->comp_buf); > kfree(hwc_cq); > } I also noticed some pre-existing issues in related code: > drivers/net/ethernet/microsoft/mana/hw_channel.c: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); [Severity: High] This is a pre-existing issue, but WQs are destroyed and freed here before the CQ/EQ. Since the EQ and CQ are not yet destroyed and their interrupts are still active, could an in-flight EQ interrupt process a late completion, call mana_hwc_rx_event_handler(), and dereference hwc->rxq which points to freed memory? > drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_send_request() > > if (!wait_for_completion_timeout(&ctx->comp_event, > (msecs_to_jiffies(hwc->hwc_timeout)))) > { [ ... ] > err = -ETIMEDOUT; > goto out; > } > out: > mana_hwc_put_msg_index(hwc, msg_id); [Severity: High] This is a pre-existing issue, but does the timeout path prematurely release the msg_id without cancelling the request in hardware? If a second thread allocates the same msg_id and the hardware sends a late response for the first request, could mana_hwc_handle_resp() copy the response data into the second thread's stack buffer? > drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_rx_event_handler() > > sge = (struct gdma_sge *)(wqe + 8 + dma_oob->inline_oob_size_div4 * 4); > > /* Select the RX work request for virtual address and for reposting. */ > rq_base_addr = hwc_rxq->msg_buf->mem_info.dma_handle; > rx_req_idx = (sge->address - rq_base_addr) / hwc->max_resp_msg_size; [Severity: High] This is a pre-existing issue, but is there a missing bounds check on the untrusted inline_oob_size_div4 before it is used to calculate the sge pointer? If a malicious hypervisor supplies an arbitrarily large value, could this lead to an out-of-bounds memory read when sge->address is dereferenced? > drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_rx_event_handler() > > resp = (struct gdma_resp_hdr *)rx_req->buf_va; > > /* Read msg_id once from DMA buffer to prevent TOCTOU: > * DMA memory is shared/unencrypted in CVMs - host can > * modify it between reads. > */ > msg_id = READ_ONCE(resp->response.hwc_msg_id); > if (msg_id >= hwc->num_inflight_msg) { [ ... ] > mana_hwc_handle_resp(hwc, rx_oob->tx_oob_data_size, rx_req, msg_id); [Severity: High] This is a pre-existing issue, but does this code read msg_id from the DMA buffer before verifying that the response size (rx_oob->tx_oob_data_size) is large enough to contain it? If the hardware sends a small completion, could the buffer retain stale data containing an old msg_id, causing mana_hwc_handle_resp() to route the response to an active, unrelated request? > drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_schedule_serv_work() > > INIT_WORK(&mns_wk->serv_work, mana_serv_func); > schedule_work(&mns_wk->serv_work); [Severity: High] This is a pre-existing issue, but the work item is queued on the global workqueue here. During device unbind, mana_gd_remove() frees the gc structure via vfree(gc) but does not track or cancel pending work items. When the delayed work item executes, could mana_serv_func() retrieve the stale gc pointer via pci_get_drvdata(pdev) and dereference it? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
