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

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