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

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

Pre-existing issues:
- [Critical] Race condition between timeout path and RX interrupt handler leading to kernel stack corruption and cross-request data corruption.
- [High] Missing length check before reading DMA response payload causes stale message IDs to incorrectly complete unrelated requests.
--

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

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

This commit adds validation for lengths, indices, and sizes taken from device
DMA in the HWC RX path. It aims to protect against buggy firmware or
malicious hosts by snapshotting values and performing bounds checks. It also
drops the early return for short responses, deferring to mana_hwc_handle_resp()
to handle them.

> 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
> @@ -90,6 +103,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);

[Severity: Critical]
This is a pre-existing issue, but is there a race condition between the
timeout path and the RX interrupt handler leading to kernel stack
corruption?

If a slow hardware response causes wait_for_completion_timeout() to
expire in mana_hwc_send_request(), the timeout path frees the msg_id
and returns:

mana_hwc_send_request() {
        err = wait_for_completion_timeout(...);
        if (!err) {
                err = -ETIMEDOUT;
                goto out;
        }
        ...
out:
        mana_hwc_put_msg_index(hwc, msg_id);
        return err;
}

Returning here pops the stack frame pointed to by ctx->output_buf.

If the interrupt handler is concurrently running and passes the bounds
checks, it could then execute memcpy() directly into this deallocated
stack frame:

mana_hwc_handle_resp() {
        ...
        memcpy(ctx->output_buf, resp_msg, resp_len);
        ...
}

Can this cause memory corruption, especially if the timed-out msg_id
is quickly reallocated to a new request which then gets overwritten
with the stale response?

>  	if (err)
>  		goto out;

[ ... ]

> @@ -259,28 +305,76 @@ static void mana_hwc_rx_event_handler(void *ctx, u32 gdma_rxq_id,
>  	resp = (struct gdma_resp_hdr *)rx_req->buf_va;
>  
> -	/* 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 from the DMA buffer to prevent TOCTOU: DMA memory
> +	 * is shared/unencrypted in CVMs, so the host can modify it between
> +	 * reads.  A short response is not rejected here; it is handed to
> +	 * mana_hwc_handle_resp() below, whose mana_hwc_verify_resp_msg()
> +	 * fails it with -EPROTO and completes the waiting sender, so one
> +	 * malformed response cannot stall the whole channel.
>  	 */
>  	msg_id = READ_ONCE(resp->response.hwc_msg_id);

[Severity: High]
This isn't a bug introduced by this patch, but does reading the DMA
response payload without a length check cause stale message IDs to
incorrectly complete unrelated requests?

If the hardware or a malicious host provides an RX response with
tx_oob_data_size smaller than the offset required to contain the
hwc_msg_id field, it will not overwrite the hwc_msg_id field in the
persistent DMA ring buffer.

The driver then reads this stale msg_id in mana_hwc_rx_event_handler(),
which likely belongs to a previously completed request that has since
been re-allocated to a new, unrelated active request.

The driver subsequently fails this unrelated active request with -EPROTO
in mana_hwc_handle_resp():

mana_hwc_handle_resp() {
        ...
        err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
        if (err)
                goto out;
        ...
out:
        ctx->error = err;
        ...
        complete(&ctx->comp_event);
}

Could a malicious host forge short DMA responses to intentionally abort
unrelated active requests?

>  	if (msg_id >= hwc->num_inflight_msg) {
>  		dev_err(hwc->dev, "HWC RX: wrong msg_id=%u\n", msg_id);

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