Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Out-of-Bounds Memory Read due to unvalidated `inline_oob_size_div4` from untrusted DMA memory. - [High] Use-After-Free during hardware channel teardown due to incorrect destruction order and premature freeing of buffers. - [High] Race condition / UAF in `mana_hwc_send_request` due to uncoordinated timeout handling. -- --- Patch [2]: [PATCH net v3 2/6] net: mana: fix HWC RQ/SQ buffer size swap --- commit 2b43add14b8efbca0def0e3777e0b101884e1da7 Author: Long Li <[email protected]> net: mana: fix HWC RQ/SQ buffer size swap This commit fixes an issue where the HWC RQ and SQ buffer sizes were swapped during initialization. It ensures the RQ is sized with max_resp_msg_size and the SQ with max_req_msg_size. The commit also reorders the assignment of these queue dimensions before creating the CQ, as the RX completion handler depends on them being set. > diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c > b/drivers/net/ethernet/microsoft/mana/hw_channel.c > index 409e20caeccdc..cbb56c7647876 100644 > --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c > +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c > @@ -263,7 +263,7 @@ static void mana_hwc_rx_event_handler(void *ctx, u32 > gdma_rxq_id, > > /* 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_req_msg_size; > + rx_req_idx = (sge->address - rq_base_addr) / hwc->max_resp_msg_size; [Severity: High] This is a pre-existing issue, but does this function have an out-of-bounds memory read when parsing the WQE? Looking at the code just before this modification in mana_hwc_rx_event_handler(): dma_oob = (struct gdma_wqe *)wqe; sge = (struct gdma_sge *)(wqe + 8 + dma_oob->inline_oob_size_div4 * 4); ... rq_base_addr = hwc_rxq->msg_buf->mem_info.dma_handle; rx_req_idx = (sge->address - rq_base_addr) / hwc->max_resp_msg_size; Because inline_oob_size_div4 is read directly from the untrusted DMA buffer without validation, could a compromised hypervisor provide a large value that causes the sge pointer to point past the end of the mapped buffer, resulting in a kernel panic when sge->address is dereferenced? > > if (rx_req_idx >= hwc_rxq->msg_buf->num_reqs) { > dev_err(hwc->dev, "HWC RX: wrong rx_req_idx=%llu, > num_reqs=%u\n", [ ... ] [Severity: High] This is a pre-existing issue, but is there a use-after-free risk during hardware channel teardown? In 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); The queues (txq and rxq) are destroyed and their memory is freed before the CQ's RCU synchronization barrier is reached. Inside mana_hwc_destroy_cq(): kfree(hwc_cq->comp_buf); if (hwc_cq->gdma_cq) mana_gd_destroy_queue(gc, hwc_cq->gdma_cq); The comp_buf array is freed immediately, but the synchronize_rcu() barrier sits inside mana_gd_destroy_queue(). Does this allow in-flight IRQ handlers to access freed memory (txq, rxq, and comp_buf) before the RCU barrier completes? [Severity: High] This is a pre-existing issue, but is there a race condition between the timeout path and the response handler that could cause stack corruption? In 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); return err; The timeout path exits and frees the message ID without synchronizing with the receiver thread or clearing ctx->output_buf. Meanwhile, in mana_hwc_handle_resp(): if (!test_bit(msg_id, hwc->inflight_msg_res.map)) { ... ctx = hwc->caller_ctx + msg_id; ... memcpy(ctx->output_buf, resp_msg, resp_len); The test_bit() check is done locklessly. If a timeout occurs right after test_bit() succeeds, mana_hwc_send_request() will return and pop the caller's stack frame. Will the response handler then memcpy() into a dangling stack pointer (ctx->output_buf), corrupting the stack? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
