Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [Critical] This problem wasn't introduced by this patch, but a race condition between the timeout path in `mana_hwc_send_request` and the hardware response handler leads to use-after-free stack corruption or tag reuse. -- --- Patch [4]: [PATCH net-next v2 4/7] net: mana: validate hardware-supplied values in the HWC RX path --- commit 20acf32024b20aecf06a023b18198388ad46e67f Author: Long Li <[email protected]> net: mana: validate hardware-supplied values in the HWC RX path This commit adds validation to the hardware-supplied values in the MANA HWC RX path. It prevents out-of-bounds access and stale response processing by checking the SGE pointer, address, and response length against posted buffers. > diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c > b/drivers/net/ethernet/microsoft/mana/hw_channel.c > index 2239fdeda57cb..515c45c4a890f 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); [Severity: Critical] This isn't a bug introduced by this patch, but is it possible for a delayed hardware response to corrupt the stack here? If a request times out in mana_hwc_send_request(), wait_for_completion_timeout() expires and the function jumps to the "out" label where it calls mana_hwc_put_msg_index(hwc, msg_id). This releases the msg_id for reuse without clearing ctx->output_buf (which points to a buffer on the caller's stack). If another thread subsequently allocates this same msg_id, it will set the in-flight bit. If the delayed hardware response for the original request arrives at this exact moment, mana_hwc_handle_resp() will pass the test_bit() check above. It will then proceed to copy the response data into ctx->output_buf: drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_handle_resp() { ... if (!err && ctx->output_buf) { ctx->status_code = resp_msg->status; memcpy(ctx->output_buf, resp_msg, resp_len); ctx->error = 0; } ... } If the new thread hasn't yet updated ctx->output_buf, wouldn't this memcpy() write DMA data directly into the timed-out caller's freed stack frame, causing a use-after-free stack corruption? Alternatively, if the new thread has already updated ctx->output_buf, could this prematurely complete the new sender's request with stale data from the delayed response? [ ... ] -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
