> This is an AI-generated review of your patch. The human sending this email has
> considered the AI review valid, or at least plausible.
> ---
> net: mana: validate hardware-supplied values in the HWC RX path
>
> This adds validation of device-supplied values in the HWC RX completion
> path: the inline OOB size and the SGE address are snapshotted and checked
> before use, the recovered RX slot index is bounds checked and the SGE address
> must match the one the driver posted for that slot, and an oversized response
> length is rejected. Untrustworthy completions no longer repost their RX WQE;
> those leaks are counted and, once they reach the RQ depth, the command
> timeout is shortened.
>
> No Link: tags are present in the commit header.
>
> One question about the changelog before the diff:
>
> [Severity: Medium]
>
> > A short response is no longer rejected in the handler: it reaches
> > mana_hwc_handle_resp(), whose mana_hwc_verify_resp_msg() fails it with
> > -EPROTO and completes the waiting sender, so a single malformed
> > response cannot convert into a channel-wide timeout.
>
> Is this paragraph describing a change against the kernel tree, or against an
> earlier revision of the patch? In the parent commit,
> mana_hwc_rx_event_handler() has no response-length check between
>
> resp = (struct gdma_resp_hdr *)rx_req->buf_va;
>
> and the msg_id read, and this diff does not remove such a check, so there is
> no
> short-response rejection in the handler being relaxed here.
>
> Does the stated guarantee hold in the resulting code? hwc_msg_id sits at
> bytes 10-11 of struct gdma_msg_hdr:
>
> include/net/mana/gdma.h:
> struct gdma_msg_hdr {
> u32 hdr_type;
> u32 msg_type;
> u16 msg_version;
> u16 hwc_msg_id;
> u32 msg_size;
> };
>
> A response shorter than 12 bytes therefore does not contain the field the
> handler reads. If the bytes read there are out of range, the handler returns
> early and mana_hwc_verify_resp_msg() is never reached, so no sender is
> completed. Could the paragraph be reworded to describe what the code
> actually guarantees?
You're right -- a <12-byte response has no hwc_msg_id, so the handler returns
early at the msg_id bound check and never reaches mana_hwc_verify_resp_msg().
That paragraph is dropped in v7; the changelog no longer claims a short
response is handled downstream.
>
> > 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: High]
>
> This isn't a bug introduced by this patch, but while the msg_id guard is being
> added here, is the bare test_bit() on hwc->inflight_msg_res.map enough to
> own the slot? At this commit the bit is tested without inflight_msg_res.lock,
> and mana_hwc_send_request() releases the slot on its timeout path:
>
> out:
> mana_hwc_put_msg_index(hwc, msg_id);
> return err;
>
> so a late or replayed response can find the bit set again for a reused slot
> and
> then write through the previous caller's output_buf:
>
> ctx = hwc->caller_ctx + msg_id;
> ...
> memcpy(ctx->output_buf, resp_msg, resp_len);
>
> The new mana_hwc_rx_leak_wqe() latch below makes the abandon-and-reuse
> path much easier to reach. For completeness: this race is closed later in the
> same series by "net: mana: fix stale HWC response after command timeout",
> which adds per-slot lock, refcount and responded state and NULLs output_buf
> under the lock on timeout, so no change is needed here if the ordering of the
> series is kept.
Agreed, and thanks for confirming the fix. The stale-response patch adds a
per-slot lock, a refcount and a "responded" flag, and NULLs output_buf under
the lock on timeout, so the reuse race is closed in the final tree. No change
here.
>
> [ ... ]
>
> > @@ -237,18 +262,39 @@ static void mana_hwc_init_event_handler(void
> *ctx, struct gdma_queue *q_self,
> > }
> > }
> >
> > +/* An RX WQE whose SGE the handler cannot trust is deliberately not
> > + * reposted: reposting a slot we may have mis-identified could
> > +double-post
> > + * a buffer the device still owns. Each such leak permanently lowers
> > +the
> > + * RQ's posted depth, so once the whole depth is gone the channel can
> > +no
> > + * longer receive responses. Make that terminal state explicit --
> > +log it
> > + * once and shorten the command timeout so callers fail fast --
> > +rather than
> > + * letting every later command drain its full timeout against a dead RQ.
> > + */
> > +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);
> > + hwc->hwc_timeout = 1;
> > + }
> > +}
>
> [Severity: High]
>
> Is hwc->rxq->queue_depth ever greater than 1 here? The header defines:
>
> include/net/mana/hw_channel.h:
> #define HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH 1
>
> and that is the only depth used:
>
> mana_hwc_create_channel()
> mana_hwc_init_queues(hwc,
> HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH, ...)
> mana_hwc_create_wq(..., q_depth, ...)
> hwc_wq->queue_depth = q_depth;
>
> If so, ++hwc->rx_leaked_wqe == hwc->rxq->queue_depth is true on the first
> call and hwc->hwc_timeout = 1 is latched immediately.
>
> Does that mean one single unattributable completion, from any of the three
> new early returns, permanently reduces every later HWC command to
>
> wait_for_completion_timeout(&ctx->comp_event,
> msecs_to_jiffies(hwc->hwc_timeout))
>
> with hwc_timeout == 1, returning -ETIMEDOUT? The inputs that decide those
> returns (rx_oob->wqe_offset, the WQE flags word, sge->address) all come
> from the device-writable memory this patch treats as untrusted, and none of
> the three paths establishes that a posted WQE was actually consumed, so a
> single forged or stale completion would appear to disable vport configuration,
> queries, RDMA setup and teardown for the life of the device.
Disagree.
Depth is 1 today, so yes, one un-attributable completion latches it. That is
intended: the three paths leak the WQE rather than repost it, and at depth 1
the RQ then has no posted buffer, so it genuinely cannot receive another
response -- shortening the timeout just makes later commands fail fast against
a dead RQ instead of each draining the full timeout. The alternative, reposting
a WQE we could not attribute, is worse: it could double-post a buffer the
device still owns.
>
> [Severity: High]
>
> This isn't a bug introduced by this patch, but does the new interrupt-context
> path make the following teardown behaviour reachable from device input?
> Once hwc_timeout is 1, mana_gd_destroy_queue() still frees the memory
> whether or not the destroy-region command succeeded:
>
> drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_destroy_queue
> () {
> ...
> mana_gd_destroy_dma_region(gc, gmi->dma_region_handle);
> mana_gd_free_memory(gmi);
> kfree(queue);
> }
>
> mana_gd_destroy_dma_region() is itself an HWC command, so with the
> timeout latched every unbind frees DMA regions whose device-side mappings
> were never torn down. mana_hwc_send_request() already collapses
> hwc_timeout to
> 1 after a genuine timeout, and this error return has always been ignored, but
> previously that state required a real hardware stall and a failing command in
> the
> log. Should the leak accounting request a service reset instead of silently
> forcing the fail-fast timeout from an interrupt?
Disagree.
mana_gd_destroy_queue() freeing the region regardless of the command result is
pre-existing and independent of this patch, so I'd keep it out of this series.
Requesting a service reset from hardirq is a heavier, separate design choice;
the fail-fast timeout is the minimal response to a permanently-exhausted RQ.
There are other reset mechanism used by the driver.
>
> [Severity: Medium]
>
> Is hwc_timeout a safe place to record this terminal state? The field has
> other
> writers, and a host-supplied reconfig event overwrites it:
>
> drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_init_event_h
> andler() {
> ...
> case HWC_DATA_CFG_HWC_TIMEOUT:
> hwc->hwc_timeout = val;
> ...
> }
>
> Because the leak helper tests with == rather than >=, the shortening can never
> be applied a second time, so after such an event every later command drains
> its
> full timeout against a dead RQ again, which is the behaviour the comment
> above the helper says it avoids.
>
> In the other direction, the store is unconditional, unlike the existing guard
> in
> mana_hwc_send_request():
>
> if (hwc->hwc_timeout > 1)
> hwc->hwc_timeout = 1;
>
> so it can raise hwc_timeout from the 0 sentinel that mana_serv_reset()
> sets:
>
> /* HWC is not responding in this case, so don't wait */
> hwc->hwc_timeout = 0;
>
> which mana_need_log() also consumes:
>
> if (hwc && hwc->hwc_timeout == 0)
> return false;
>
> Would a separate sticky flag (and hwc->rx_leaked_wqe being reset when the
> channel is re-established) express this state better? As it stands
> rx_leaked_wqe
> is never reset and no recovery is requested.
Disagree.
The harmful direction -- resurrecting the serv_reset() 0 sentinel -- is guarded
in v7: the store is "if (hwc->hwc_timeout > 1) hwc->hwc_timeout = 1;", matching
mana_hwc_send_request(). The == vs >= and the reconfig overwrite are benign for
what is a terminal state, so I'd rather not add a separate sticky field for it.
>
> [Severity: Medium]
>
> Should this store be annotated? mana_hwc_rx_leak_wqe() runs in HWC
> interrupt context:
>
> mana_gd_intr() -> EQ handler -> mana_hwc_comp_event()
> -> mana_hwc_rx_event_handler() -> mana_hwc_rx_leak_wqe()
>
> while the same field is read and read-modify-written from process context with
> plain accesses in mana_hwc_send_request(), written by
> mana_gd_query_hwc_timeout() through a request/response round trip, read
> unlocked by mana_need_log() and used as a 0 sentinel by mana_serv_reset().
> There is no common lock and no READ_ONCE()/WRITE_ONCE() here, so the
> process-context update can be lost and the 0 sentinel can be resurrected to 1
> mid-reset. A structurally identical field in the same series,
> hwc_init_max_num_cqs, is annotated with WRITE_ONCE()/READ_ONCE() and
> a comment; was the omission here intentional?
Fix in v7.
>
> > +
> > static void mana_hwc_rx_event_handler(void *ctx, u32 gdma_rxq_id,
> > const struct hwc_rx_oob *rx_oob) {
>
> [ ... ]
>
> > @@ -259,28 +305,76 @@ static void mana_hwc_rx_event_handler(void *ctx,
> u32 gdma_rxq_id,
> > wqe = mana_gd_get_wqe_ptr(rq, rx_oob->wqe_offset /
> GDMA_WQE_BU_SIZE);
> > dma_oob = (struct gdma_wqe *)wqe;
> >
> > - sge = (struct gdma_sge *)(wqe + 8 + dma_oob->inline_oob_size_div4 *
> 4);
> > -
> > - /* Select the RX work request for virtual address and for reposting. */
> > + /* inline_oob_size_div4 lives in device-accessible RQ memory (shared
> > + * and host-writable in a confidential VM), so snapshot it once and
> > + * validate and use only the snapshot. It is a bit-field, which
> > + * READ_ONCE() cannot take the size of, so read the u32 flags word it
> > + * shares through the union and extract the field from the local copy.
> > + * The driver programs INLINE_OOB_SMALL_SIZE for every HWC RQ
> WQE via
> > + * mana_gd_post_work_request(), so the only valid value is
> > + * INLINE_OOB_SMALL_SIZE / 4, which puts the SGE at wqe + 16 inside
> > + * this WQE's own BU. Reject anything else -- the slot cannot be
> > + * trusted, so leak this RX WQE rather than repost the wrong one.
> > + */
> > + oob_snapshot.flags = READ_ONCE(dma_oob->flags);
> > + oob_div4 = oob_snapshot.inline_oob_size_div4;
> > + if (oob_div4 != INLINE_OOB_SMALL_SIZE / 4) {
> > + dev_err(hwc->dev, "HWC RX: unexpected
> inline_oob_size_div4=%u\n",
> > + oob_div4);
> > + mana_hwc_rx_leak_wqe(hwc);
> > + return;
> > + }
> > + sge = (struct gdma_sge *)(wqe + 8 + oob_div4 * 4);
> > +
> > + /* Recover the originating RX slot from the SGE address. Snapshot it
> > + * once, for the same shared-memory reason: of the three terms only
> > + * sge_addr comes from device memory; rq_base_addr and
> > + * max_resp_msg_size are driver-private. An in-range but wrong SGE
> > + * would otherwise truncate onto a neighbouring slot, letting us read
> > + * a stale response that could complete the wrong, reused in-flight
> > + * request. Require the index in range AND the address to exactly
> > + * match the value the driver posted for that slot.
> > + */
> > + sge_addr = READ_ONCE(sge->address);
> > rq_base_addr = hwc_rxq->msg_buf->mem_info.dma_handle;
> > - rx_req_idx = (sge->address - rq_base_addr) / hwc-
> >max_resp_msg_size;
> > -
> > - if (rx_req_idx >= hwc_rxq->msg_buf->num_reqs) {
> > - dev_err(hwc->dev, "HWC RX: wrong rx_req_idx=%llu,
> num_reqs=%u\n",
> > - rx_req_idx, hwc_rxq->msg_buf->num_reqs);
> > + rx_req_idx = (sge_addr - rq_base_addr) / hwc->max_resp_msg_size;
> > +
> > + if (rx_req_idx >= hwc_rxq->queue_depth) {
> > + /* Cannot identify the slot, so we cannot safely repost this
> > + * WQE; leak it. An out-of-range index means a corrupted SGE
> > + * from hardware or host tampering.
> > + */
> > + dev_err(hwc->dev, "HWC RX: SGE idx %llu out of range\n",
> > + rx_req_idx);
> > + mana_hwc_rx_leak_wqe(hwc);
> > return;
> > }
>
> [Severity: Low]
>
> Why was the bound changed from the array's own counter to a field of a
> different structure? The index is used immediately afterwards to index
> hwc_rxq->msg_buf->reqs[], which is declared as:
>
> include/net/mana/hw_channel.h:
> u32 num_reqs;
> struct hwc_work_request reqs[] __counted_by(num_reqs);
>
> so num_reqs is the annotated bound for that array, while queue_depth is a
> u16 in struct hwc_wq. The two are equal only by construction:
> mana_hwc_create_wq() sets hwc_wq->queue_depth = q_depth and
> separately calls mana_hwc_alloc_dma_buf(hwc, q_depth, ...), which allocates
> reqs[q_depth] and sets dma_buf->num_reqs = q_depth. Nothing ties them
> together, num_reqs becomes write-only in this file, and the change isn't
> mentioned in the changelog even though every other added validation is.
> Would keeping the check against msg_buf->num_reqs (and keeping num_reqs
> in the error message) be preferable?
Yes -- fixed in v7. The bound is back to hwc_rxq->msg_buf->num_reqs (the
__counted_by bound for reqs[]), and num_reqs is in the error message.
>
> >
> > rx_req = &hwc_rxq->msg_buf->reqs[rx_req_idx];
> > + if (sge_addr != (u64)rx_req->buf_sge_addr) {
> > + /* In-range index but the address does not match what the
> > + * driver posted for that slot; the same unrecoverable case,
> > + * so leak this WQE rather than repost the wrong one.
> > + */
> > + dev_err(hwc->dev, "HWC RX: invalid SGE address %llx
> (idx=%llu)\n",
> > + sge_addr, rx_req_idx);
> > + mana_hwc_rx_leak_wqe(hwc);
> > + return;
> > + }
> > +
>
> [Severity: Medium]
>
> This isn't a problem introduced by this patch, but does the address match
> establish the invariant the helper's comment relies on ("reposting a slot we
> may have mis-identified could double-post a buffer the device still owns")?
> The check only shows that the WQE named by the device-supplied completion
> offset still contains the OOB size and SGE address the driver once wrote
> there,
> and that offset is masked into the ring rather than
> rejected:
>
> drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_get_wqe_ptr()
> {
> u32 offset = (wqe_offset * GDMA_WQE_BU_SIZE) & (wq->queue_size -
> 1);
>
> WARN_ON_ONCE((offset + GDMA_WQE_BU_SIZE) > wq->queue_size);
> ...
> }
>
> RQ WQE memory keeps those driver-written values after the device consumes
> the WQE, and mana_hwc_comp_event() dispatches on completions[i].is_sq
> alone, without looking at rx_oob->vendor_err or eom, so a spurious, error or
> replayed completion naming a still-owned slot passes both new checks and
> gets reposted. The HWC RQ is created with spec.monitor_avl_buf = false, so
> mana_gd_post_work_request() does no free-space check and
> wq->head simply advances past the number of buffers the driver owns. Is
> some per-slot posted/consumed state needed for the address match to mean
> "this WQE was just consumed for this buffer"?
>
> > 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);
> > if (msg_id >= hwc->num_inflight_msg) {
> > dev_err(hwc->dev, "HWC RX: wrong msg_id=%u\n", msg_id);
> > + mana_hwc_post_rx_wqe(hwc_rxq, rx_req);
> > return;
> > }
>
> [Severity: Low]
>
> Should these messages be rate limited? All four rejection paths in this
> handler
> use plain dev_err() from hardirq context with device-supplied values
> ("unexpected inline_oob_size_div4=%u", "SGE idx %llu out of range", "invalid
> SGE address %llx (idx=%llu)", "wrong msg_id=%u"). This path previously
> returned without reposting, which bounded how often a host could drive it;
> with the added mana_hwc_post_rx_wqe() a host that keeps fabricating
> completions can repeat it indefinitely and flood the log, which is unpleasant
> with a serial console. dev_err_ratelimited(), or the counter scheme used for
> the leak accounting, would avoid that.
Good point -- fixed in v7. The reposting paths (wrong msg_id, and the
msg_id/resp_len rejections in mana_hwc_handle_resp()) now use
dev_err_ratelimited(), so a host can't flood the log. The three leak paths
don't repost and are bounded by the RQ depth, so they keep a single dev_err().
Thanks,
Long
