Re: [PATCH net-next v2 3/7] net: mana: free HWC comp_buf after destroying the EQ

[sashiko-bot@kernel.org]

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 <longli@microsoft.com>

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/20260721234339.1476932-1-longli@microsoft.com?part=3