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 <[email protected]> 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/[email protected]?part=4
