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

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

Pre-existing issues:
- [Critical] Race condition leading to stack memory corruption and Use-After-Free (ABA) on hardware command timeout.
--

--- Patch [4]: [PATCH net v4 4/7] net: mana: validate hardware-supplied values in the HWC RX path ---
commit 3dd532e8ebcefe4a1f19044be675c58a1504e9c6
Author: Long Li <[email protected]>

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

This commit adds validation for hardware-supplied lengths and indices in the
MANA hardware channel RX handler. It ensures the inline OOB size, SGE addresses,
and response lengths are checked before use. Out-of-bounds or untrusted RX WQEs
are leaked and, if exhausted, the channel goes into a terminal state.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 19896bb5ce1a4..5db8cfe2d8443 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -83,6 +83,19 @@ static void mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
>  	struct hwc_caller_ctx *ctx;
>  	int err;
>  
> +	/* Defence in depth: the sole caller, mana_hwc_rx_event_handler(),
> +	 * already rejects msg_id >= hwc->num_inflight_msg with the value it
> +	 * passes here by value, so this cannot be reached out of range.  Keep
> +	 * the guard at the indexing site so the bitmap and caller_ctx array
> +	 * are never indexed without a bound in view.
> +	 */
> +	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);

[Severity: Critical]
This is a pre-existing issue, but is there a race condition between this
test_bit() check and the command timeout path that could corrupt stack memory?

If a thread calling mana_hwc_send_request() times out, it takes this path:

	if (!wait_for_completion_timeout(&ctx->comp_event,
					 (hwc->hwc_timeout * HZ) / 1000)) {
		dev_err(hwc->dev, "HWC: Request timed out!\n");
		err = -ETIMEDOUT;
		goto out;
	}
    ...
    out:
	mana_hwc_put_msg_index(hwc, msg_id);
	return err;

The timeout path clears the message ID bit and returns without synchronizing
with the interrupt handler.

Concurrently, if a delayed hardware response arrives here in
mana_hwc_handle_resp(), it could pass the test_bit() check right before the
timeout path clears it.

The timeout thread then clears the bit and returns, popping its stack.
Then mana_hwc_handle_resp() proceeds and executes:

	if (!err && ctx->output_buf) {
		ctx->status_code = resp_msg->status;
		memcpy(ctx->output_buf, resp_msg, resp_len);

Since ctx->output_buf points to the now-freed stack frame, could this memcpy()
write hardware data into a freed stack frame or to the stack frame of a new
thread reusing the message ID?

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