Re: [PATCH net v5 2/7] net: mana: fix HWC RQ/SQ buffer size swap

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Use-after-free on stack buffer due to missing synchronization between timeout handler and RX completion.
- [High] Teardown sequence in mana_hwc_destroy_channel() frees WQs before quiescing the CQ, causing a UAF if an in-flight EQ handler runs concurrently.
- [High] In a Confidential VM setup, a malicious host can modify the unencrypted DMA memory field `inline_oob_size_div4` in the receive queue ring buffer to cause the guest to read past the end of the ring buffer allocation, resulting in a kernel panic.
--

--- Patch [2]: [PATCH net v5 2/7] net: mana: fix HWC RQ/SQ buffer size swap ---
commit b0ce57145f46d4e8a027e81baea5f74199a24b8e
Author: Long Li <[email protected]>

net: mana: fix HWC RQ/SQ buffer size swap

The HWC RQ receives responses and the SQ sends requests, but
mana_hwc_init_queues() sized the RQ with max_req_msg_size and the SQ with
max_resp_msg_size -- backwards.  mana_hwc_rx_event_handler() also
recovered the RX slot index by dividing by max_req_msg_size instead of
the response size that strides the RQ buffer.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 409e20caeccdc..3f011ebbe7b3e 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 isn't a bug introduced by this patch, but does mana_hwc_destroy_channel()
in hw_channel.c free the WQs before quiescing the CQ?

In mana_hwc_destroy_channel():
	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 EQ interrupt fires concurrently before mana_hwc_destroy_cq() is called, 
can mana_gd_process_eq_events() look up the still-published CQ, invoking 
mana_hwc_rx_event_handler() which dereferences the freed hwc->rxq here?


[Severity: High]
This is a pre-existing issue, but can a malicious host cause an out-of-bounds
dereference when we read sge->address here?

Earlier in mana_hwc_rx_event_handler():
	sge = (struct gdma_sge *)(wqe + 8 + dma_oob->inline_oob_size_div4 * 4);

If this is a Confidential VM, dma_oob resides in unencrypted DMA memory
shared with the host. Can the host supply an arbitrarily large
inline_oob_size_div4, forcing the guest kernel to dereference unmapped memory
when reading sge->address?


[Severity: Critical]
This is a pre-existing issue, but is there a use-after-free on the stack
buffer if mana_hwc_send_request() times out?

If mana_hwc_send_request() times out, it clears the inflight bit and returns,
leaving ctx->output_buf pointing to an invalid stack frame. 

Concurrently, mana_hwc_rx_event_handler() calls mana_hwc_handle_resp() which
does a lockless test_bit():

	if (!test_bit(msg_id, hwc->inflight_msg_res.map))
		return;

	...
	memcpy(ctx->output_buf, resp_msg, resp_len);

If the timeout thread clears the bit and returns just after the test_bit()
check in mana_hwc_handle_resp(), will we copy the response data into a popped
stack frame?

>  
>  	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",

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2