Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
Pre-existing issues: - [Critical] A timed-out HWC request prematurely releases its msg_id slot without hardware cancellation, allowing a delayed hardware response to corrupt the memory of a subsequent request. - [High] mana_hwc_send_request() does not reinitialize the completion event before waiting, making it vulnerable to duplicate hardware responses. - [High] The asynchronous work item `mana_serv_func` is scheduled on the global system workqueue but never canceled during device removal, leading to a race condition and UAF on `gc`. - [High] Untrusted inline_oob_size_div4 from DMA memory causes an out-of-bounds read when parsing the WQE. - [High] The HWC RX event handler drops RX WQEs and permanently drains the receive queue if the device returns a bogus msg_id or rx_req_idx. -- --- Patch [2]: [PATCH net v7 2/7] net: mana: fix HWC RQ/SQ buffer size swap --- commit 55639f5c7ee6bd080a63d387fa6ec19ca3c639a4 Author: Long Li <[email protected]> net: mana: fix HWC RQ/SQ buffer size swap This patch corrects the buffer sizes used for the hardware channel (HWC) queues. It sizes the receive queue (RQ) with max_resp_msg_size and the send queue (SQ) with max_req_msg_size, fixing a latent bug where these sizes were incorrectly swapped during queue initialization. > diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c > b/drivers/net/ethernet/microsoft/mana/hw_channel.c > index b5ed2dbce6ceb..ccef9bf9c6bfd 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; > > 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 isn't a bug introduced by this patch, but does this code properly bound-check inline_oob_size_div4? In mana_hwc_rx_event_handler(): wqe = mana_gd_get_wqe_ptr(rq, rx_oob->wqe_offset / GDMA_WQE_BU_SIZE); dma_oob = (struct gdma_wqe *)wqe; sge = (struct gdma_sge *)(wqe + 8 + dma_oob->inline_oob_size_div4 * 4); If the host returns a maliciously large inline_oob_size_div4 in the DMA buffer, could this cause an out-of-bounds memory read when dereferencing sge->address on the line modified above? [Severity: High] This is also a pre-existing issue, but does this error path permanently leak the RX WQE? By returning early on an invalid rx_req_idx (or msg_id further down), this skips reposting the RX WQE back to the hardware via mana_hwc_post_rx_wqe(). Could a malicious host repeatedly send invalid responses to completely drain the receive queue and halt control message processing? > @@ -737,14 +737,14 @@ static int mana_hwc_init_queues(struct > hw_channel_context *hwc, u16 q_depth, > goto out; > } > > - err = mana_hwc_create_wq(hwc, GDMA_RQ, q_depth, max_req_msg_size, > + err = mana_hwc_create_wq(hwc, GDMA_RQ, q_depth, max_resp_msg_size, > hwc->cq, &hwc->rxq); > if (err) { > dev_err(hwc->dev, "Failed to create HWC RQ: %d\n", err); > goto out; > } > > - err = mana_hwc_create_wq(hwc, GDMA_SQ, q_depth, max_resp_msg_size, > + err = mana_hwc_create_wq(hwc, GDMA_SQ, q_depth, max_req_msg_size, > hwc->cq, &hwc->txq); > if (err) { > dev_err(hwc->dev, "Failed to create HWC SQ: %d\n", err); > @@ -753,6 +753,7 @@ static int mana_hwc_init_queues(struct hw_channel_context > *hwc, u16 q_depth, > > hwc->num_inflight_msg = q_depth; > hwc->max_req_msg_size = max_req_msg_size; > + hwc->max_resp_msg_size = max_resp_msg_size; > > return 0; > out: [ ... ] [Severity: Critical] This isn't a bug introduced by this patch, but does the timeout handling in mana_hwc_send_request() allow a delayed hardware response to corrupt memory? drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_send_request() { if (!wait_for_completion_timeout(&ctx->comp_event, ...)) { ... err = -ETIMEDOUT; goto out; } ... out: mana_hwc_put_msg_index(hwc, msg_id); return err; } The timeout path jumps to out: and releases the msg_id back to the bitmap without waiting for or canceling the hardware operation. If a new request reuses this msg_id, and the hardware subsequently delivers a delayed response for the original request, could mana_hwc_handle_resp() blindly memcpy() the delayed response into the new request's output_buf? [Severity: High] This is a pre-existing issue, but is ctx->comp_event properly reinitialized when a msg_id is reused? drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_send_request() { ctx = hwc->caller_ctx + msg_id; ctx->output_buf = resp; ctx->output_buflen = resp_len; ... if (!wait_for_completion_timeout(&ctx->comp_event, ...)) { ... } Because reinit_completion() is never called before wait_for_completion_timeout(), a previous delayed response could cause mana_hwc_handle_resp() to call complete(). If that happens, could wait_for_completion_timeout() return immediately for a new request? This would cause the caller to process uninitialized data from ctx->output_buf. [Severity: High] This is a pre-existing issue, but can the global serv_work cause a use-after-free during device removal? 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); } The work item is scheduled on the global system workqueue. During device removal, mana_gd_remove() frees the gdma context (gc): drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_remove() { ... vfree(gc); } Since the driver does not wait for or cancel this asynchronous work item, could mana_serv_func() execute after mana_gd_remove() and dereference the freed gc pointer in mana_serv_reset()? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
