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 [off-list ref]
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.

[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?

[ ... ]

[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/20260813174243.3044348-1-longli@microsoft.com?part=2