Re: [PATCH net-next 4/7] net: mana: validate hardware-supplied values in the HWC RX path

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] A lack of synchronization between the request timeout path and the interrupt response handler leads to severe stack memory corruption and data leakage due to reused message IDs.
- [High] An unvalidated `inline_oob_size_div4` value read from shared DMA memory is used to calculate the `sge` pointer, leading to an out-of-bounds read and kernel panic.
--

--- Patch [4]: [PATCH net-next 4/7] net: mana: validate hardware-supplied values in the HWC RX path ---
commit 2b4c8935e9727b2b08f9773b13a4caaa94f11d99
Author: Long Li <longli@microsoft.com>

net: mana: validate hardware-supplied values in the HWC RX path

This commit adds validation for lengths and indices received from device DMA
in the hardware channel receive path. It verifies the SGE address matches
the posted slot, ensures the response covers the header, bounds checks the
message ID, and rejects oversized responses to prevent out of bounds access.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 2239fdeda57cb..68236727aee8b 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -83,6 +83,17 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
>  	struct hwc_caller_ctx *ctx;
>  	int err;
>  
> +	/* Validate msg_id is in range before using it to index bitmap
> +	 * and caller_ctx array.  Malicious firmware could send
> +	 * out-of-range msg_id causing out-of-bounds access.
> +	 */
> +	if (msg_id >= hwc->num_inflight_msg) {
> +		dev_err(hwc->dev, "hwc_rx: msg_id %u >= max %u\n",
> +			msg_id, hwc->num_inflight_msg);
> +		mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
> +		return;
> +	}
> +
>  	if (!test_bit(msg_id, hwc->inflight_msg_res.map)) {
>  		dev_err(hwc->dev, "hwc_rx: invalid msg_id = %u\n", msg_id);
>  		mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
> @@ -90,6 +101,18 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
>  	}
>  
>  	ctx = hwc->caller_ctx + msg_id;
> +
> +	/* Reject responses larger than the RX DMA buffer — the SGE
> +	 * limits what hardware can DMA, so an oversized resp_len
> +	 * indicates a firmware bug.  Fail rather than silently
> +	 * truncating.
> +	 */
> +	if (resp_len > rx_req->buf_len) {
> +		dev_err(hwc->dev, "HWC RX: resp_len %u > buf_len %u\n",
> +			resp_len, rx_req->buf_len);
> +		resp_len = 0;
> +	}
> +
>  	err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
>  	if (err)
>  		goto out;

[Severity: Critical]
This isn't a bug introduced by this patch, but is there a race condition
between the request timeout path and this interrupt response handler?

If wait_for_completion_timeout() expires in mana_hwc_send_request(), the
function clears the message ID bit and returns. This leaves ctx->output_buf
dangling, pointing to the caller's stack frame.

If the hardware response arrives immediately after, or if the message ID is
reused by a new thread, could mana_hwc_handle_resp() read the ID and
execute memcpy() into the dangling output_buf? Since no lock is held to
verify the sender thread is still waiting, it seems this could lead to stack
memory corruption and cross-request data leakage.

[ ... ]

> @@ -261,19 +284,45 @@ static void mana_hwc_rx_event_handler(void *ctx, u32 gdma_rxq_id,
>  
>  	sge = (struct gdma_sge *)(wqe + 8 + dma_oob->inline_oob_size_div4 * 4);

[Severity: High]
This is a pre-existing issue, but does this calculation lack bounds checking
for the sge pointer?

The inline_oob_size_div4 field is read directly from the untrusted DMA WQE,
multiplied by 4, and added to the WQE pointer. If the WQE is located at the
very end of the page-aligned DMA ring buffer (for example, at offset 4064
in a 4096-byte page), could this unchecked addition cause the resulting sge
pointer to exceed the queue buffer boundary?

This could lead to an out-of-bounds read and kernel panic when sge->address
is later dereferenced, which might be reachable by a malicious host in a
confidential VM environment.

> -	/* Select the RX work request for virtual address and for reposting. */
> +	/* Recover the originating RX slot from the SGE address.  Of the three
> +	 * terms here only sge->address lives in device-accessible RQ memory;
> +	 * rq_base_addr and max_resp_msg_size are driver-private constants.  An
> +	 * in-range but wrong/unaligned SGE (corrupted WQE, or a malicious host
> +	 * in a CVM) would otherwise truncate onto a neighbouring slot, letting
> +	 * us read a stale response that could complete the wrong, reused
> +	 * in-flight request.  Require the index to be in range AND the address
> +	 * to exactly match the value the driver posted for that slot.
> +	 */
>  	rq_base_addr = hwc_rxq->msg_buf->mem_info.dma_handle;
>  	rx_req_idx = (sge->address - rq_base_addr) / hwc->max_resp_msg_size;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715032942.3945317-1-longli@microsoft.com?part=4