[PATCH v7 0/2] RDMA/rxe: fix TOCTOU races in send WQE processing

[PATCH v7 0/2] RDMA/rxe: fix TOCTOU races in send WQE processing

[Tristan Madani]

The SoftRoCE driver maps its send work queue into userspace for direct
posting. The kernel reads WQE fields directly from this shared mapping,
which allows a concurrent userspace thread to modify fields between
kernel reads -- classic TOCTOU.

This series copies each send WQE to a kernel-private buffer before
processing, in both the requester (patch 1) and completer (patch 2)
paths.

v7 addresses all findings from the v6 sashiko AI review and Zhu Yanjun
review:

  - FORTIFY_SOURCE: memcpy destination is now the enclosing struct
    (send_wqe/comp_wqe) rather than the inner wqe member, so
    __member_size() sees the full buffer including the trailing SGE
    array.

  - Copy size: uses max(max_sge * sizeof(ib_sge), max_inline) to
    match the actual queue element data area, ensuring all SGE entries
    are captured even when max_sge * sizeof(ib_sge) exceeds max_inline.

  - IB_WR_REG_MR: opcode bound raised from IB_WR_ATOMIC_WRITE to
    IB_WR_REG_MR so kernel ULP fast registration is not rejected.
    Invalid opcodes get mask=0 in the requester (normal error path)
    or COMPST_ERROR in the completer.

  - num_sge: the requester no longer silently clamps num_sge, letting
    the existing rxe_requester() validation detect and error the QP.
    The completer validates num_sge and cur_sge bounds, returning
    COMPST_ERROR on violation to prevent OOB SGE walks.

  - DMA progress writeback: both paths now write back dma.resid,
    dma.cur_sge, and dma.sge_offset so retries resume from the correct
    offset (fixing IB_WR_FLUSH and partial READ retry regressions).

  - State TOCTOU in completer: the smp_load_acquire()d state is
    restored after memcpy to prevent a concurrent write from overwriting
    it.

  - Posted WQE: get_wqe() sets *wqe_p = NULL for posted state,
    preventing the timeout_retry path from operating on an uncopied
    shared WQE.

  - Retry: comp_wqe_valid is invalidated when a retry is initiated,
    ensuring retried responses get a fresh copy with updated PSNs.

Tristan Madani (2):
  RDMA/rxe: copy send WQE to kernel buffer before processing
  RDMA/rxe: copy send WQE to kernel buffer in completer path

 drivers/infiniband/sw/rxe/rxe_comp.c  | 72 ++++++++++++++++++++++++++++++++++-
 drivers/infiniband/sw/rxe/rxe_qp.c    |  2 ++
 drivers/infiniband/sw/rxe/rxe_req.c   | 64 +++++++++++++++++++++++++++++---
 drivers/infiniband/sw/rxe/rxe_verbs.h | 12 ++++++
 4 files changed, 143 insertions(+), 7 deletions(-)

Tristan Madani (2):
  RDMA/rxe: copy send WQE to kernel buffer before processing
  RDMA/rxe: copy send WQE to kernel buffer in completer path

 drivers/infiniband/sw/rxe/rxe_comp.c  | 72 ++++++++++++++++++++++++++-
 drivers/infiniband/sw/rxe/rxe_qp.c    |  2 +
 drivers/infiniband/sw/rxe/rxe_req.c   | 64 ++++++++++++++++++++++--
 drivers/infiniband/sw/rxe/rxe_verbs.h | 12 +++++
 4 files changed, 143 insertions(+), 7 deletions(-)

-- 
2.47.3

[PATCH v7 1/2] RDMA/rxe: copy send WQE to kernel buffer before processing

[Tristan Madani]

The rxe send queue is mapped into userspace via mmap. The requester
processes Work Queue Entries (WQEs) directly from this shared buffer
without first copying them to kernel memory. Userspace can modify WQE
fields (num_sge, sge_offset, SGE entries) between kernel reads,
leading to inconsistent state in copy_data().

This is the send-path counterpart to the receive-path fixes:
  - commit 22b8fbded65b8 ("RDMA/rxe: Fix TOCTOU heap overflow in
    get_srq_wqe")
  - commit d6ab440240a04 ("RDMA/rxe: Copy WQE to local buffer in
    non-SRQ receive path")

Fix by copying the send WQE to a kernel-private buffer in req_next_wqe()
before processing. The copy uses the full queue element data size
(max of max_sge SGEs and max_inline bytes) so that both SGE entries
and inline data are always captured.

After copying, reject out-of-range opcodes by setting mask to 0 so
that next_opcode() transitions the QP to error state through the
normal error path. This covers all valid opcodes up to IB_WR_REG_MR
while preventing OOB access in the wr_opcode_mask() lookup table.

The local copy is reused across multi-packet sends to preserve DMA
progress state (cur_sge, sge_offset, resid). Field updates including
DMA progress are written back to the shared queue using WRITE_ONCE()
for individual fields and smp_store_release() for state transitions,
so the completer and userspace observe consistent values without the
tearing risk of bulk memcpy(). DMA progress writeback ensures retries
can resume from the correct offset.

The copy is invalidated when the WQE index advances (last packet sent,
error, local ops, UD oversized), when a retry resets WQE state, on QP
reset, or when the QP enters error state for flush.

Fixes: 8700e3e7c485 ("Soft RoCE driver")
Cc: stable@vger.kernel.org
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---

Changes since v6:
  - Fixed all sashiko AI review findings (2 Critical + 9 High).
  - FORTIFY_SOURCE: memcpy into enclosing struct, not inner member.
  - Copy size: max(max_sge * sizeof(ib_sge), max_inline).
  - Opcode bound: IB_WR_REG_MR (was IB_WR_ATOMIC_WRITE).
  - Requester: removed silent num_sge clamp, use mask=0 for invalid opcodes.
  - Completer: validate num_sge + cur_sge, preserve smp_load_acquire state.
  - Both paths: DMA progress writeback (resid, cur_sge, sge_offset).
  - Completer: *wqe_p = NULL for posted state, invalidate on retry.
  - Addressed Zhu Yanjun review feedback.

Changes since v5:
  - Added opcode bounds check and num_sge clamp after copy.

Changes since v4:
  - Resent from the correct sender address.

Changes since v3:
  - Copy full queue element (max_sge SGEs) instead of per-WQE size.
  - Invalidate on QP reset, ERR flush, RESET state check.

Changes since v2:
  - WRITE_ONCE/smp_store_release for field writeback.

Changes since v1:
  - Split into per-path patches. Cache across multi-packet ops.
 drivers/infiniband/sw/rxe/rxe_qp.c    |  1 +
 drivers/infiniband/sw/rxe/rxe_req.c   | 64 ++++++++++++++++++++++++---
 drivers/infiniband/sw/rxe/rxe_verbs.h |  6 +++
 3 files changed, 66 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c
index 311f285d78a6b..77606c4a039b1 100644
--- a/drivers/infiniband/sw/rxe/rxe_qp.c
+++ b/drivers/infiniband/sw/rxe/rxe_qp.c
@@ -581,6 +581,7 @@ static void rxe_qp_reset(struct rxe_qp *qp)
 	qp->req.need_retry = 0;
 	qp->req.wait_for_rnr_timer = 0;
 	qp->req.noack_pkts = 0;
+	qp->req.send_wqe_valid = false;
 	qp->resp.msn = 0;
 	qp->resp.opcode = -1;
 	qp->resp.drop_msg = 0;
diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
index 24f5c044363f7..f4a258a66485c 100644
--- a/drivers/infiniband/sw/rxe/rxe_req.c
+++ b/drivers/infiniband/sw/rxe/rxe_req.c
@@ -161,6 +161,26 @@ static void req_check_sq_drain_done(struct rxe_qp *qp)
 	spin_unlock_irqrestore(&qp->state_lock, flags);
 }
 
+static void rxe_req_writeback_wqe(struct rxe_qp *qp)
+{
+	struct rxe_send_wqe *shared = qp->req.shared_wqe;
+	struct rxe_send_wqe *local = &qp->req.send_wqe.wqe;
+
+	if (!qp->req.send_wqe_valid || !shared)
+		return;
+
+	WRITE_ONCE(shared->status, local->status);
+	WRITE_ONCE(shared->first_psn, local->first_psn);
+	WRITE_ONCE(shared->last_psn, local->last_psn);
+	WRITE_ONCE(shared->mask, local->mask);
+	WRITE_ONCE(shared->has_rd_atomic, local->has_rd_atomic);
+	WRITE_ONCE(shared->dma.resid, local->dma.resid);
+	WRITE_ONCE(shared->dma.cur_sge, local->dma.cur_sge);
+	WRITE_ONCE(shared->dma.sge_offset, local->dma.sge_offset);
+	/* State must be last so the completer sees prior updates */
+	smp_store_release(&shared->state, local->state);
+}
+
 static struct rxe_send_wqe *__req_next_wqe(struct rxe_qp *qp)
 {
 	struct rxe_queue *q = qp->sq.queue;
@@ -193,7 +213,28 @@ static struct rxe_send_wqe *req_next_wqe(struct rxe_qp *qp)
 	}
 	spin_unlock_irqrestore(&qp->state_lock, flags);
 
-	wqe->mask = wr_opcode_mask(wqe->wr.opcode, qp);
+	/* Reuse the existing kernel-private copy if still valid */
+	if (qp->req.send_wqe_valid && qp->req.shared_wqe == wqe)
+		return &qp->req.send_wqe.wqe;
+
+	/* Copy WQE from userspace-mapped shared queue to kernel-private
+	 * buffer. Copy size matches the queue element data area.
+	 */
+	memcpy(&qp->req.send_wqe, wqe, sizeof(*wqe) +
+	       max_t(int, qp->sq.max_sge * sizeof(struct ib_sge),
+		    qp->sq.max_inline));
+	qp->req.shared_wqe = wqe;
+	qp->req.send_wqe_valid = true;
+
+	wqe = &qp->req.send_wqe.wqe;
+
+	/* Prevent OOB in wr_opcode_mask() for corrupted opcodes.
+	 * Mask 0 causes next_opcode() to fail, moving QP to error state.
+	 */
+	if (unlikely(wqe->wr.opcode > IB_WR_REG_MR))
+		wqe->mask = 0;
+	else
+		wqe->mask = wr_opcode_mask(wqe->wr.opcode, qp);
 	return wqe;
 }
 
@@ -582,9 +623,13 @@ static void update_state(struct rxe_qp *qp, struct rxe_pkt_info *pkt)
 {
 	qp->req.opcode = pkt->opcode;
 
-	if (pkt->mask & RXE_END_MASK)
+	rxe_req_writeback_wqe(qp);
+
+	if (pkt->mask & RXE_END_MASK) {
 		qp->req.wqe_index = queue_next_index(qp->sq.queue,
 						     qp->req.wqe_index);
+		qp->req.send_wqe_valid = false;
+	}
 
 	qp->need_req_skb = 0;
 
@@ -634,7 +679,9 @@ static int rxe_do_local_ops(struct rxe_qp *qp, struct rxe_send_wqe *wqe)
 
 	wqe->state = wqe_state_done;
 	wqe->status = IB_WC_SUCCESS;
+	rxe_req_writeback_wqe(qp);
 	qp->req.wqe_index = queue_next_index(qp->sq.queue, qp->req.wqe_index);
+	qp->req.send_wqe_valid = false;
 
 	return 0;
 }
@@ -666,6 +713,7 @@ int rxe_requester(struct rxe_qp *qp)
 		wqe = __req_next_wqe(qp);
 		spin_unlock_irqrestore(&qp->state_lock, flags);
 		if (wqe) {
+			qp->req.send_wqe_valid = false;
 			wqe->status = IB_WC_WR_FLUSH_ERR;
 			goto err;
 		} else {
@@ -681,6 +729,7 @@ int rxe_requester(struct rxe_qp *qp)
 		qp->req.wait_psn = 0;
 		qp->req.need_retry = 0;
 		qp->req.wait_for_rnr_timer = 0;
+		qp->req.send_wqe_valid = false;
 		spin_unlock_irqrestore(&qp->state_lock, flags);
 		goto exit;
 	}
@@ -695,6 +744,7 @@ int rxe_requester(struct rxe_qp *qp)
 	if (unlikely(qp->req.need_retry && !qp->req.wait_for_rnr_timer)) {
 		req_retry(qp);
 		qp->req.need_retry = 0;
+		qp->req.send_wqe_valid = false;
 	}
 
 	wqe = req_next_wqe(qp);
@@ -772,10 +822,12 @@ int rxe_requester(struct rxe_qp *qp)
 			wqe->last_psn = qp->req.psn;
 			qp->req.psn = (qp->req.psn + 1) & BTH_PSN_MASK;
 			qp->req.opcode = IB_OPCODE_UD_SEND_ONLY;
-			qp->req.wqe_index = queue_next_index(qp->sq.queue,
-						       qp->req.wqe_index);
 			wqe->state = wqe_state_done;
 			wqe->status = IB_WC_SUCCESS;
+			rxe_req_writeback_wqe(qp);
+			qp->req.wqe_index = queue_next_index(qp->sq.queue,
+						       qp->req.wqe_index);
+			qp->req.send_wqe_valid = false;
 			goto done;
 		}
 		payload = mtu;
@@ -839,8 +891,10 @@ int rxe_requester(struct rxe_qp *qp)
 	goto out;
 err:
 	/* update wqe_index for each wqe completion */
-	qp->req.wqe_index = queue_next_index(qp->sq.queue, qp->req.wqe_index);
 	wqe->state = wqe_state_error;
+	rxe_req_writeback_wqe(qp);
+	qp->req.wqe_index = queue_next_index(qp->sq.queue, qp->req.wqe_index);
+	qp->req.send_wqe_valid = false;
 	rxe_qp_error(qp);
 exit:
 	ret = -EAGAIN;
diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.h b/drivers/infiniband/sw/rxe/rxe_verbs.h
index 0f5ffd94643f9..a22dfc6e5ae3c 100644
--- a/drivers/infiniband/sw/rxe/rxe_verbs.h
+++ b/drivers/infiniband/sw/rxe/rxe_verbs.h
@@ -114,6 +114,12 @@ struct rxe_req_info {
 	int			wait_for_rnr_timer;
 	int			noack_pkts;
 	int			again;
+	struct rxe_send_wqe	*shared_wqe;
+	bool			send_wqe_valid;
+	struct {
+		struct rxe_send_wqe	wqe;
+		struct ib_sge		sge[RXE_MAX_SGE];
+	} send_wqe;
 };
 
 struct rxe_comp_info {
-- 
2.47.3

Re: [PATCH v7 1/2] RDMA/rxe: copy send WQE to kernel buffer before processing

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] RDMA/rxe: out-of-bounds read from qp->req.send_wqe in finish_packet()
--

commit 7ddcf04d6a32c8caa9ab909cc0cf868ac66e4fb8
Author: Tristan Madani <tristmd@gmail.com>

RDMA/rxe: copy send WQE to kernel buffer before processing

This commit copies send WQEs from the userspace-mapped send queue into a
kernel-private buffer in req_next_wqe() before processing, writing updated
status, state, and DMA progress fields back to the shared queue.

> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
> index 4f353abd76f55..11c9c3259220b 100644
> --- a/drivers/infiniband/sw/rxe/rxe_req.c
> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
[ ... ]
> @@ -193,7 +213,28 @@ static struct rxe_send_wqe *req_next_wqe(struct rxe_qp *qp)
>  	}
>  	spin_unlock_irqrestore(&qp->state_lock, flags);
>  
> -	wqe->mask = wr_opcode_mask(wqe->wr.opcode, qp);
> +	/* Reuse the existing kernel-private copy if still valid */
> +	if (qp->req.send_wqe_valid && qp->req.shared_wqe == wqe)
> +		return &qp->req.send_wqe.wqe;
> +
> +	/* Copy WQE from userspace-mapped shared queue to kernel-private
> +	 * buffer. Copy size matches the queue element data area.
> +	 */
> +	memcpy(&qp->req.send_wqe, wqe, sizeof(*wqe) +
> +	       max_t(int, qp->sq.max_sge * sizeof(struct ib_sge),
> +		    qp->sq.max_inline));
> +	qp->req.shared_wqe = wqe;
> +	qp->req.send_wqe_valid = true;
> +
> +	wqe = &qp->req.send_wqe.wqe;

[Severity: Critical]
As reported by Sashiko on v6 [1], can redirecting wqe to qp->req.send_wqe.wqe
cause an out-of-bounds read from qp->req.send_wqe in finish_packet() for an
IB_WR_ATOMIC_WRITE work request?

In rxe_verbs.h, qp->req.send_wqe reserves RXE_MAX_SGE entries (512 bytes)
after wqe:

	struct {
		struct rxe_send_wqe	wqe;
		struct ib_sge		sge[RXE_MAX_SGE];
	} send_wqe;

When a user QP posts a WQE in the mmap'd send queue with wqe->wr.opcode set to
IB_WR_ATOMIC_WRITE, wqe->dma.num_sge = 0, wqe->dma.cur_sge = 0, and
wqe->dma.resid = 4096, the num_sge and cur_sge check in rxe_requester() passes
when qp->sq.max_sge >= 1.

rxe_requester() then computes payload from wqe->dma.resid and only caps it at
mtu (up to 4096 bytes):

	mtu = get_mtu(qp);
	payload = (mask & (RXE_WRITE_OR_SEND_MASK | RXE_ATOMIC_WRITE_MASK)) ?
			wqe->dma.resid : 0;
	if (payload > mtu) {
		...
		payload = mtu;
	}

When rxe_requester() calls finish_packet(), the RXE_ATOMIC_WRITE_MASK branch
copies payload bytes directly from wqe->dma.atomic_wr without checking that
payload is sizeof(u64) (8 bytes) or fits within the copied buffer:

	if (pkt->mask & RXE_ATOMIC_WRITE_MASK) {
		memcpy(payload_addr(pkt), wqe->dma.atomic_wr, payload);
		wqe->dma.resid -= payload;
	}

If payload exceeds the copied tail size, memcpy() reads uninitialized bytes
from qp->req.send_wqe.sge, and if payload exceeds 512 bytes, it reads past the
end of qp->req.send_wqe into the rest of struct rxe_qp and adjacent slab
memory before transmitting the packet. Should finish_packet() or
rxe_requester() validate that payload == 8 for RXE_ATOMIC_WRITE_MASK and fits
within the copied WQE data area?

[1] https://lore.kernel.org/r/20261026001405.1698455-2-tristan@talencesecurity.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030151558.1458444-1-tristan@talencesecurity.com?part=1

[PATCH v7 2/2] RDMA/rxe: copy send WQE to kernel buffer in completer path

[Tristan Madani]

The completer reads send WQEs from the same userspace-mapped shared
queue as the requester. Even after the requester copies the WQE to a
kernel-private buffer (previous patch), the completer still reads
directly from shared memory, leaving it exposed to the same TOCTOU
races.

Fix by copying the WQE in get_wqe() to a completer-private buffer
using the full queue element data size. The local copy is reused
when the same WQE is being processed with unchanged state, which
preserves DMA progress across multi-packet RDMA READ responses.
A fresh copy is taken when the state changes or a new WQE appears.

After copying, reject invalid opcodes (above IB_WR_REG_MR), out-of-
bounds num_sge, and invalid cur_sge by transitioning the QP to error
state through COMPST_ERROR. This prevents OOB SGE walks in copy_data()
from corrupted DMA indices.

The state read from the shared queue is preserved via smp_load_acquire()
and restored after the memcpy to prevent a concurrent userspace write
from overwriting it with a stale value.

DMA progress (resid, cur_sge, sge_offset) is written back alongside
completion status and rd_atomic state, ensuring retries can resume
from the correct offset rather than restarting from zero.

The cache is invalidated on QP reset, when a completion advances to
the next WQE, and on retry to ensure the retried response gets a
fresh copy with the updated PSNs from the requester.

Fixes: 8700e3e7c485 ("Soft RoCE driver")
Cc: stable@vger.kernel.org
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---

Changes since v6:
  - Fixed all sashiko AI review findings (2 Critical + 9 High).
  - FORTIFY_SOURCE: memcpy into enclosing struct, not inner member.
  - Copy size: max(max_sge * sizeof(ib_sge), max_inline).
  - Opcode bound: IB_WR_REG_MR (was IB_WR_ATOMIC_WRITE).
  - Requester: removed silent num_sge clamp, use mask=0 for invalid opcodes.
  - Completer: validate num_sge + cur_sge, preserve smp_load_acquire state.
  - Both paths: DMA progress writeback (resid, cur_sge, sge_offset).
  - Completer: *wqe_p = NULL for posted state, invalidate on retry.
  - Addressed Zhu Yanjun review feedback.

Changes since v5:
  - Added opcode bounds check and num_sge clamp after copy.

Changes since v4:
  - Resent from the correct sender address.

Changes since v3:
  - Copy full queue element (max_sge SGEs) instead of per-WQE size.
  - Invalidate on QP reset, ERR flush, RESET state check.

Changes since v2:
  - WRITE_ONCE/smp_store_release for field writeback.

Changes since v1:
  - Split into per-path patches. Cache across multi-packet ops.
 drivers/infiniband/sw/rxe/rxe_comp.c  | 72 ++++++++++++++++++++++++++-
 drivers/infiniband/sw/rxe/rxe_qp.c    |  1 +
 drivers/infiniband/sw/rxe/rxe_verbs.h |  6 +++
 3 files changed, 77 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_comp.c b/drivers/infiniband/sw/rxe/rxe_comp.c
index 1390e861bd1d7..cd4085a1bf2f8 100644
--- a/drivers/infiniband/sw/rxe/rxe_comp.c
+++ b/drivers/infiniband/sw/rxe/rxe_comp.c
@@ -137,22 +137,87 @@ void rxe_comp_queue_pkt(struct rxe_qp *qp, struct sk_buff *skb)
 	rxe_sched_task(&qp->send_task);
 }
 
+/* Write back completer WQE fields to shared memory */
+static void rxe_comp_writeback_wqe(struct rxe_qp *qp)
+{
+	struct rxe_send_wqe *shared = qp->comp.shared_wqe;
+	struct rxe_send_wqe *local = &qp->comp.comp_wqe.wqe;
+
+	if (!qp->comp.comp_wqe_valid || !shared)
+		return;
+
+	WRITE_ONCE(shared->status, local->status);
+	WRITE_ONCE(shared->has_rd_atomic, local->has_rd_atomic);
+	WRITE_ONCE(shared->dma.resid, local->dma.resid);
+	WRITE_ONCE(shared->dma.cur_sge, local->dma.cur_sge);
+	WRITE_ONCE(shared->dma.sge_offset, local->dma.sge_offset);
+}
+
 static inline enum comp_state get_wqe(struct rxe_qp *qp,
 				      struct rxe_pkt_info *pkt,
 				      struct rxe_send_wqe **wqe_p)
 {
 	struct rxe_send_wqe *wqe;
+	u32 state;
 
 	/* we come here whether or not we found a response packet to see if
 	 * there are any posted WQEs
 	 */
 	wqe = queue_head(qp->sq.queue, QUEUE_TYPE_FROM_CLIENT);
-	*wqe_p = wqe;
 
 	/* no WQE or requester has not started it yet */
-	if (!wqe || wqe->state == wqe_state_posted)
+	if (!wqe) {
+		*wqe_p = NULL;
 		return pkt ? COMPST_DONE : COMPST_EXIT;
+	}
+
+	/* Pairs with smp_store_release() in rxe_req_writeback_wqe() */
+	state = smp_load_acquire(&wqe->state);
+	if (state == wqe_state_posted) {
+		*wqe_p = NULL;
+		return pkt ? COMPST_DONE : COMPST_EXIT;
+	}
+
+	/* Reuse existing local copy if still processing same WQE
+	 * with unchanged state (preserves DMA progress for multi-packet ops)
+	 */
+	if (qp->comp.comp_wqe_valid && qp->comp.shared_wqe == wqe &&
+	    qp->comp.comp_wqe.wqe.state == state) {
+		wqe = &qp->comp.comp_wqe.wqe;
+		*wqe_p = wqe;
+		goto check_state;
+	}
+
+	/* Copy shared WQE to kernel-private buffer.
+	 * Copy size matches the queue element data area.
+	 */
+	qp->comp.shared_wqe = wqe;
+	memcpy(&qp->comp.comp_wqe, wqe,
+	       sizeof(*wqe) + max_t(int,
+				qp->sq.max_sge * sizeof(struct ib_sge),
+				qp->sq.max_inline));
+	qp->comp.comp_wqe_valid = true;
+
+	/* Preserve the state we observed via smp_load_acquire()
+	 * since memcpy may have overwritten it with a stale read.
+	 */
+	qp->comp.comp_wqe.wqe.state = state;
+
+	/* Reject corrupted opcode or SGE indices, error the QP */
+	if (unlikely(qp->comp.comp_wqe.wqe.wr.opcode > IB_WR_REG_MR) ||
+	    qp->comp.comp_wqe.wqe.dma.num_sge > qp->sq.max_sge ||
+	    (qp->comp.comp_wqe.wqe.dma.resid &&
+	     qp->comp.comp_wqe.wqe.dma.cur_sge >= qp->sq.max_sge)) {
+		qp->comp.comp_wqe.wqe.status = IB_WC_LOC_QP_OP_ERR;
+		wqe = &qp->comp.comp_wqe.wqe;
+		*wqe_p = wqe;
+		return COMPST_ERROR;
+	}
+
+	wqe = &qp->comp.comp_wqe.wqe;
+	*wqe_p = wqe;
 
+check_state:
 	/* WQE does not require an ack */
 	if (wqe->state == wqe_state_done)
 		return COMPST_COMP_WQE;
@@ -454,6 +519,8 @@ static void do_complete(struct rxe_qp *qp, struct rxe_send_wqe *wqe)
 	if (post)
 		make_send_cqe(qp, wqe, &cqe);
 
+	rxe_comp_writeback_wqe(qp);
+	qp->comp.comp_wqe_valid = false;
 	queue_advance_consumer(qp->sq.queue, QUEUE_TYPE_FROM_CLIENT);
 
 	if (post)
@@ -788,6 +855,7 @@ int rxe_completer(struct rxe_qp *qp)
 							RXE_CNT_COMP_RETRY);
 					qp->req.need_retry = 1;
 					qp->comp.started_retry = 1;
+					qp->comp.comp_wqe_valid = false;
 					qp->req.again = 1;
 				}
 				goto done;
diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c
index 77606c4a039b1..450254a131017 100644
--- a/drivers/infiniband/sw/rxe/rxe_qp.c
+++ b/drivers/infiniband/sw/rxe/rxe_qp.c
@@ -582,6 +582,7 @@ static void rxe_qp_reset(struct rxe_qp *qp)
 	qp->req.wait_for_rnr_timer = 0;
 	qp->req.noack_pkts = 0;
 	qp->req.send_wqe_valid = false;
+	qp->comp.comp_wqe_valid = false;
 	qp->resp.msn = 0;
 	qp->resp.opcode = -1;
 	qp->resp.drop_msg = 0;
diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.h b/drivers/infiniband/sw/rxe/rxe_verbs.h
index a22dfc6e5ae3c..6205dbc29dff6 100644
--- a/drivers/infiniband/sw/rxe/rxe_verbs.h
+++ b/drivers/infiniband/sw/rxe/rxe_verbs.h
@@ -130,6 +130,12 @@ struct rxe_comp_info {
 	int			started_retry;
 	u32			retry_cnt;
 	u32			rnr_retry;
+	struct rxe_send_wqe	*shared_wqe;
+	bool			comp_wqe_valid;
+	struct {
+		struct rxe_send_wqe	wqe;
+		struct ib_sge		sge[RXE_MAX_SGE];
+	} comp_wqe;
 };
 
 /* responder states */
-- 
2.47.3

Re: [PATCH v7 2/2] RDMA/rxe: copy send WQE to kernel buffer in completer path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] RDMA/rxe: missing rxe_comp_writeback_wqe() before retry loses READ progress
- [High] RDMA/rxe: missing comp_wqe_valid invalidation in COMPST_RNR_RETRY
--

commit 7163a14999e322c25c58445d22fa6ea5241151a2
Author: Tristan Madani <tristmd@gmail.com>

RDMA/rxe: copy send WQE to kernel buffer in completer path

This commit copies the send WQE from userspace-mapped shared memory into a
completer-private kernel buffer in get_wqe() to prevent TOCTOU races, while
caching the copy across multi-packet responses and writing back modified WQE
fields on completion.

> diff --git a/drivers/infiniband/sw/rxe/rxe_comp.c b/drivers/infiniband/sw/rxe/rxe_comp.c
> index 1390e861bd1d7..cd4085a1bf2f8 100644
> --- a/drivers/infiniband/sw/rxe/rxe_comp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_comp.c
> @@ -137,22 +137,87 @@ void rxe_comp_queue_pkt(struct rxe_qp *qp, struct sk_buff *skb)
>  	rxe_sched_task(&qp->send_task);
>  }
>  
> +/* Write back completer WQE fields to shared memory */
> +static void rxe_comp_writeback_wqe(struct rxe_qp *qp)
> +{
> +	struct rxe_send_wqe *shared = qp->comp.shared_wqe;
> +	struct rxe_send_wqe *local = &qp->comp.comp_wqe.wqe;
> +
> +	if (!qp->comp.comp_wqe_valid || !shared)
> +		return;
> +
> +	WRITE_ONCE(shared->status, local->status);
> +	WRITE_ONCE(shared->has_rd_atomic, local->has_rd_atomic);
> +	WRITE_ONCE(shared->dma.resid, local->dma.resid);
> +	WRITE_ONCE(shared->dma.cur_sge, local->dma.cur_sge);
> +	WRITE_ONCE(shared->dma.sge_offset, local->dma.sge_offset);
> +}

[ ... ]

> @@ -454,6 +519,8 @@ static void do_complete(struct rxe_qp *qp, struct rxe_send_wqe *wqe)
>  	if (post)
>  		make_send_cqe(qp, wqe, &cqe);
>  
> +	rxe_comp_writeback_wqe(qp);
> +	qp->comp.comp_wqe_valid = false;
>  	queue_advance_consumer(qp->sq.queue, QUEUE_TYPE_FROM_CLIENT);
>  
>  	if (post)
> @@ -788,6 +855,7 @@ int rxe_completer(struct rxe_qp *qp)
>  							RXE_CNT_COMP_RETRY);
>  					qp->req.need_retry = 1;
>  					qp->comp.started_retry = 1;
> +					qp->comp.comp_wqe_valid = false;
>  					qp->req.again = 1;
>  				}
>  				goto done;

[Severity: High]
As reported by Sashiko on v6 [1], does invalidating qp->comp.comp_wqe_valid
in COMPST_ERROR_RETRY without calling rxe_comp_writeback_wqe(qp) first lose
DMA progress for partially completed multi-packet RDMA read operations?

During a multi-packet RDMA read, rxe_requester() sends a request packet with
payload == 0 and leaves shared_wqe->dma.resid equal to wqe->dma.length in
qp->sq.queue. As non-final read response packets arrive in rxe_completer(),
do_read() calls copy_data(), advancing wqe->dma.resid, wqe->dma.cur_sge, and
wqe->dma.sge_offset in the local qp->comp.comp_wqe.wqe copy, and returns
COMPST_UPDATE_COMP:

drivers/infiniband/sw/rxe/rxe_comp.c:do_read() {
	ret = copy_data(qp->pd, IB_ACCESS_LOCAL_WRITE,
			&wqe->dma, payload_addr(pkt),
			payload_size(pkt), RXE_TO_MR_OBJ);
	if (ret) {
		wqe->status = IB_WC_LOC_PROT_ERR;
		return COMPST_ERROR;
	}

	if (wqe->dma.resid == 0 && (pkt->mask & RXE_END_MASK))
		return COMPST_COMP_ACK;

	return COMPST_UPDATE_COMP;
}

Because rxe_comp_writeback_wqe() is only called in do_complete() when the WQE
finishes and is popped from the send queue, clearing qp->comp.comp_wqe_valid
in COMPST_ERROR_RETRY discards the updated local DMA state before req_retry()
runs on the shared WQE:

drivers/infiniband/sw/rxe/rxe_req.c:req_retry() {
			if (mask & WR_READ_MASK) {
				npsn = (wqe->dma.length - wqe->dma.resid) /
					qp->mtu;
				wqe->iova += npsn * qp->mtu;
			}
}

With shared_wqe->dma.resid still equal to wqe->dma.length, npsn evaluates to
0 and the retried read restarts from offset 0 with cur_sge == 0 and
sge_offset == 0 instead of resuming from the partial offset. Should
rxe_comp_writeback_wqe(qp) be called before clearing comp_wqe_valid on retry
(or after copy_data() in do_read())?

[Severity: High]
As reported by Sashiko on v6 [2], what happens in rxe_completer() when an RNR
NAK triggers COMPST_RNR_RETRY right below this hunk?

drivers/infiniband/sw/rxe/rxe_comp.c:rxe_completer() {
		case COMPST_RNR_RETRY:
			/* we come here if we received an RNR NAK */
			if (qp->comp.rnr_retry > 0) {
				if (qp->comp.rnr_retry != 7)
					qp->comp.rnr_retry--;

				/* don't start a retry flow until the
				 * rnr timer has fired
				 */
				qp->req.wait_for_rnr_timer = 1;
				rxe_dbg_qp(qp, "set rnr nak timer\n");
				// TODO who protects from destroy_qp??
				mod_timer(&qp->rnr_nak_timer,
					  jiffies + rnrnak_jiffies(aeth_syn(pkt)
						& ~AETH_TYPE_MASK));
				goto exit;
			}
}

Unlike COMPST_ERROR_RETRY, COMPST_RNR_RETRY does not clear
qp->comp.comp_wqe_valid, and when rnr_nak_timer expires and rxe_requester()
calls req_retry(), only qp->req.send_wqe_valid is cleared. Once
rxe_requester() retransmits the WQE and sets shared_wqe->state back to
wqe_state_pending with updated first_psn and last_psn values, won't get_wqe()
see:

	if (qp->comp.comp_wqe_valid && qp->comp.shared_wqe == wqe &&
	    qp->comp.comp_wqe.wqe.state == state) {
		wqe = &qp->comp.comp_wqe.wqe;
		*wqe_p = wqe;
		goto check_state;
	}

and reuse the stale pre-retry qp->comp.comp_wqe copy, causing check_ack() to
compare the retried response against stale first_psn and last_psn values and
fail with COMPST_ERROR?

[1] https://lore.kernel.org/r/20261026001405.1698455-3-tristan@talencesecurity.com
[2] https://lore.kernel.org/r/20261026001405.1698455-3-tristan@talencesecurity.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030151558.1458444-1-tristan@talencesecurity.com?part=2