Re: [PATCH net v7 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 3 potential issue(s) to consider:

New issues:
- [Medium] Unprotected concurrent modification of `hwc->hwc_timeout` across hardirq and process contexts.

Pre-existing issues:
- [Critical] A race condition exists between `mana_hwc_send_request` timeouts and delayed hardware completions, leading to Use-After-Free and stack corruption of reused message slots.
- [High] Short or malformed hardware responses abort unrelated in-flight requests due to reading stale `msg_id` data from the DMA buffer.
--

--- Patch [4]: [PATCH net v7 4/7] net: mana: validate hardware-supplied values in the HWC RX path ---
commit 77109d87fe60b75679f5bdbcf384cc1fe90785bf
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 HWC RX path. It ensures OOB sizes, SGE addresses, response lengths,
and message IDs are within expected bounds before processing to prevent
out-of-bounds access or processing wrong requests.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 7e01596df11b6..2691d60945912 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -83,13 +83,29 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
>  	struct hwc_caller_ctx *ctx;
>  	int err;
>  
[ ... ]
>  	ctx = hwc->caller_ctx + msg_id;
> +
> +	/* An oversized resp_len cannot fit the RX buffer: reject it. */
> +	if (resp_len > rx_req->buf_len) {
> +		dev_err_ratelimited(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);

[Severity: Critical]
This isn't a bug introduced by this patch, but does a race condition exist
between mana_hwc_send_request timeouts and delayed hardware completions?

If mana_hwc_send_request times out, it releases the msg_id back to the pool
without synchronizing with the asynchronous response handler, abandoning the
request and destroying its stack frame.

If a delayed hardware completion triggers mana_hwc_handle_resp() later, could
this lead to a use-after-free and stack memory corruption when it copies data
into the newly reused ctx->output_buf?

[ ... ]
> @@ -237,18 +253,37 @@ static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
>  	}
>  }
>  
> +/* Drop an RX WQE with an untrusted SGE rather than repost it, which could
> + * double-post a buffer the device still owns.  This lowers the RQ depth;
> + * once it is exhausted the channel can no longer receive, so log it and
> + * shorten the timeout to fail callers fast.
> + */
> +static void mana_hwc_rx_leak_wqe(struct hw_channel_context *hwc)
> +{
> +	if (++hwc->rx_leaked_wqe == hwc->rxq->queue_depth) {
> +		dev_err(hwc->dev,
> +			"HWC RX: RQ exhausted after %u leaked WQEs; channel unusable\n",
> +			hwc->rx_leaked_wqe);
> +		if (hwc->hwc_timeout > 1)
> +			hwc->hwc_timeout = 1;

[Severity: Medium]
Is this modification of hwc->hwc_timeout safe from concurrent access?

This function introduces an unprotected modification to the shared timeout
state. Since it modifies hwc->hwc_timeout without locks or WRITE_ONCE() from
hardirq context, could this cause unpredictable timeout evaluations if it
races with process context readers or writers?

> +	}
> +}
> +
[ ... ]
> @@ -259,28 +294,58 @@ static void mana_hwc_rx_event_handler(void *ctx, u32 gdma_rxq_id,
[ ... ]
> -	/* Read msg_id once from DMA buffer to prevent TOCTOU:
> -	 * DMA memory is shared/unencrypted in CVMs - host can
> -	 * modify it between reads.
> +	/* Read msg_id once: it is host-writable DMA memory.  A short response
> +	 * is left for mana_hwc_handle_resp() to reject, so it cannot stall
> +	 * the channel.
>  	 */
>  	msg_id = READ_ONCE(resp->response.hwc_msg_id);

[Severity: High]
This is a pre-existing issue, but can reading msg_id from DMA memory here
cause unrelated in-flight requests to be aborted?

If a short or malformed hardware response is received, the code reads a
potentially stale msg_id directly from the DMA buffer and delegates rejection
to mana_hwc_handle_resp().

Could a malicious host or buggy hardware force the completion and abortion of
an unrelated in-flight request using this unverified msg_id?

>  	if (msg_id >= hwc->num_inflight_msg) {
> -		dev_err(hwc->dev, "HWC RX: wrong msg_id=%u\n", msg_id);
> +		dev_err_ratelimited(hwc->dev, "HWC RX: wrong msg_id=%u\n", msg_id);
> +		mana_hwc_post_rx_wqe(hwc_rxq, rx_req);
>  		return;
>  	}

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