[PATCH v3 0/2] RDMA/rxe: Fix TOCTOU races on mmap'd send queue

[PATCH v3 0/2] RDMA/rxe: Fix TOCTOU races on mmap'd send queue

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

The rxe driver maps the send queue into userspace via mmap. Both the
requester and completer process Work Queue Entries (WQEs) directly from
this shared buffer without first copying them to kernel memory. This
allows userspace to modify WQE fields concurrently, causing inconsistent
state in the kernel.

This series fixes both paths:
  - Patch 1/2: requester (rxe_req.c) - copy WQE before processing,
    with WRITE_ONCE per-field writeback
  - Patch 2/2: completer (rxe_comp.c) - same treatment, with reuse
    across multi-packet operations to preserve DMA progress

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")

Changes since v2:
  - Replaced bulk memcpy() writeback with targeted WRITE_ONCE() for
    individual fields and smp_store_release() for state transitions,
    avoiding the tearing risk of bulk memcpy on the shared queue
  - Added smp_load_acquire() in the completer to pair with the
    requester's smp_store_release() for state ordering

Changes since v1:
  - Same as v2 changes (v2 only covered patch 2/2)

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  | 58 +++++++++++++++++++++++++--
 drivers/infiniband/sw/rxe/rxe_req.c   | 61 +++++++++++++++++++++++++----
 drivers/infiniband/sw/rxe/rxe_verbs.h | 12 ++++++
 3 files changed, 119 insertions(+), 12 deletions(-)

-- 
2.39.5

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

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

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 local copy is reused across multi-packet sends
to preserve DMA progress state (cur_sge, sge_offset, resid). Field
updates 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().

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

Fixes: 8700e3e7c485 ("Soft RoCE driver")
Cc: stable@vger.kernel.org
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---
 drivers/infiniband/sw/rxe/rxe_req.c   | 59 +++++++++++++++++++++++++--
 drivers/infiniband/sw/rxe/rxe_verbs.h |  6 +++
 2 files changed, 61 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
index 24f5c044363f7..c72de69630e58 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);
 }
 
+/* Write back requester WQE fields to shared memory using targeted
+ * stores so the completer and userspace observe consistent state.
+ */
+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);
+	/* 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;
@@ -178,6 +198,8 @@ static struct rxe_send_wqe *req_next_wqe(struct rxe_qp *qp)
 {
 	struct rxe_send_wqe *wqe;
 	unsigned long flags;
+	unsigned int num_sge;
+	size_t copy_size;
 
 	req_check_sq_drain_done(qp);
 
@@ -193,6 +215,24 @@ static struct rxe_send_wqe *req_next_wqe(struct rxe_qp *qp)
 	}
 	spin_unlock_irqrestore(&qp->state_lock, flags);
 
+	/* 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 to prevent TOCTOU races on DMA state fields.
+	 */
+	num_sge = wqe->dma.num_sge;
+	if (unlikely(num_sge > qp->sq.max_sge)) {
+		rxe_dbg_qp(qp, "invalid num_sge in send WQE\n");
+		return NULL;
+	}
+	copy_size = sizeof(*wqe) + num_sge * sizeof(struct rxe_sge);
+	memcpy(&qp->req.send_wqe.wqe, wqe, copy_size);
+	qp->req.shared_wqe = wqe;
+	qp->req.send_wqe_valid = true;
+
+	wqe = &qp->req.send_wqe.wqe;
 	wqe->mask = wr_opcode_mask(wqe->wr.opcode, qp);
 	return wqe;
 }
@@ -582,9 +622,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 +678,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;
 }
@@ -695,6 +741,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 +819,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 +888,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 v3 1/2] RDMA/rxe: copy send WQE to kernel buffer before processing

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] rxe: Silent QP stall on invalid WQE num_sge
- [High] rxe: Stale WQE cache reuse across QP resets
- [High] rxe: Data clobbering of flush error state in rxe_requester
- [High] rxe: Out-of-bounds array read via unvalidated WQE opcode
- [High] rxe: TOCTOU info leak during WQE copy due to compiler reload
- [High] rxe: Uninitialized kernel memory transmission on inline sends
- [High] rxe: RDMA READ retries broken due to missing DMA state writeback
- [Medium] rxe: FORTIFY_SOURCE field-spanning write warning in req_next_wqe memcpy
--

commit 99abb995a563e5638c46b6895ade6985823dd689
Author: Tristan Madani <tristan@talencesecurity.com>

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

This commit introduces a local kernel-private copy of the requester Work Queue
Entries (WQEs) to prevent TOCTOU vulnerabilities during send queue processing.
It copies the WQE fields into a local buffer before validation and uses
targeted writes to update the shared completion state. A validity flag tracks
when the cached WQE is active for reuse across multi-packet sends.

> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
> index 53f7c1e7fe55b..025a41145c408 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);
>  }
>  
> +/* Write back requester WQE fields to shared memory using targeted
> + * stores so the completer and userspace observe consistent state.
> + */
> +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);
> +	/* State must be last so the completer sees prior updates */
> +	smp_store_release(&shared->state, local->state);
> +}

[Severity: High]
Does omitting DMA state updates here break RDMA READ retries?

To protect against TOCTOU attacks, this writeback deliberately omits
DMA state updates like resid and cur_sge.

However, the untouched req_retry() function reads wqe->dma.resid directly
from the shared WQE to calculate the retry PSN offset:

    npsn = (wqe->dma.length - wqe->dma.resid) / qp->mtu;

Because the shared resid is never updated by the processing path anymore,
req_retry() incorrectly computes that no progress was made. This sends
retries from the beginning of the buffer and breaks RDMA READ response state.

[ ... ]

> @@ -193,6 +215,24 @@ static struct rxe_send_wqe *req_next_wqe(struct rxe_qp *qp)
>  	}
>  	spin_unlock_irqrestore(&qp->state_lock, flags);
>  
> +	/* 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;

[Severity: High]
Can this erroneously reuse a stale cached WQE across QP resets?

When a QP transitions to IB_QPS_RESET, rxe_qp_reset() resets the queue
indices to 0 but does not clear qp->req.send_wqe_valid.

If the QP is later transitioned back to RTS and userspace posts a new WQE at
index 0, req_next_wqe() evaluates this condition. Since the address matches
the pre-reset WQE at index 0, it incorrectly reuses the stale cached WQE
instead of copying and processing the newly posted WQE.

> +
> +	/* Copy WQE from userspace-mapped shared queue to kernel-private
> +	 * buffer to prevent TOCTOU races on DMA state fields.
> +	 */
> +	num_sge = wqe->dma.num_sge;
> +	if (unlikely(num_sge > qp->sq.max_sge)) {
> +		rxe_dbg_qp(qp, "invalid num_sge in send WQE\n");
> +		return NULL;
> +	}

[Severity: High]
Does returning NULL here stall the queue?

If this validation fails, it returns NULL. The caller rxe_requester() handles
a NULL return by exiting the task loop and returning -EAGAIN. Because the WQE
is not explicitly failed with an error status, the queue consumer index is
never advanced and the QP is not transitioned to IB_QPS_ERR. The QP will
silently hang forever, repeatedly trying to process this malformed WQE.

> +	copy_size = sizeof(*wqe) + num_sge * sizeof(struct rxe_sge);
> +	memcpy(&qp->req.send_wqe.wqe, wqe, copy_size);

[Severity: Medium]
Will this memcpy trigger a FORTIFY_SOURCE warning or panic?

The destination pointer &qp->req.send_wqe.wqe is strongly typed to the wqe
member of the wrapper struct, so the compiler evaluates __builtin_object_size
as exactly sizeof(struct rxe_send_wqe). When num_sge > 0, copy_size exceeds
this struct size, which can trigger a __fortify_memcpy_chk warning for a
field-spanning write into the adjacent sge array.

[Severity: High]
Is there a TOCTOU info leak here due to a compiler reload during memcpy?

The variable num_sge is read from shared memory, validated against
qp->sq.max_sge, and used to compute copy_size.

If userspace maliciously inflates wqe->dma.num_sge in shared memory after
the local variable is read but before memcpy executes, the copied struct
header will contain the inflated num_sge value.

Because memcpy only copies bytes up to the original copy_size, the trailing
SGE array elements in the local buffer remain uninitialized. The secondary
validation in rxe_requester() reads the modified num_sge from the local copy,
validates it against max_sge, and processes the uninitialized SGE elements,
potentially leaking kernel memory to the network.

[Severity: High]
Will this transmit uninitialized kernel memory on inline sends?

For inline sends, the payload resides in the inline_data flex array following
the WQE header, often with num_sge == 0.

If num_sge is 0, the copy_size calculation results in strictly copying the WQE
header, entirely omitting the inline data payload. When the requester later
processes the packet, it reads the payload from the local copy's inline_data
union array, transmitting uninitialized kernel memory instead of the user's
data.

> +	qp->req.shared_wqe = wqe;
> +	qp->req.send_wqe_valid = true;
> +
> +	wqe = &qp->req.send_wqe.wqe;
>  	wqe->mask = wr_opcode_mask(wqe->wr.opcode, qp);

[Severity: High]
Can this result in an out-of-bounds array read?

At this point, wqe->wr.opcode has just been copied from userspace and is
entirely unvalidated. wr_opcode_mask() uses this opcode directly as an
array index into rxe_wr_opcode_info, which can lead to an out-of-bounds
kernel memory read and a system crash.

>  	return wqe;
>  }

[ ... ]

> @@ -839,8 +888,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);

[Severity: High]
Does this writeback clobber the flush error state with stale cached data?

If a QP enters IB_QPS_ERR while a WQE is locally cached, rxe_requester()
fetches the shared WQE directly via __req_next_wqe(). It assigns
wqe->status = IB_WC_WR_FLUSH_ERR and jumps to err:, setting
wqe->state = wqe_state_error directly on the shared WQE.

Then it calls rxe_req_writeback_wqe(qp). Since qp->req.send_wqe_valid is
still true, the writeback function writes the stale local->status and
local->state over the shared WQE, erasing the flush error state and
potentially stalling completions.

> +	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;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007212737.1989004-1-tristmd@gmail.com?part=1

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

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

The rxe completer processes send Work Queue Entries (WQEs) directly
from the mmap'd shared send queue without copying them to kernel memory.
Userspace can modify WQE fields (num_sge, opcode, PSN values, DMA state)
while the completer is processing them, leading to inconsistent
decisions in check_psn() and check_ack(), and potential out-of-bounds
access in copy_data() via a corrupted dma.num_sge.

This is the completer-path counterpart to the previous commit which
fixed the requester path.

Fix by copying the WQE to a kernel-private buffer in get_wqe() before
processing. The local copy is reused across multi-packet operations
(e.g. RDMA READ responses) when the WQE state is unchanged, preserving
DMA progress across completer invocations. The copy is refreshed when
the state changes (via smp_load_acquire pairing with the requester
smp_store_release) to ensure PSN and other fields are consistent.

Field updates (status, has_rd_atomic) are written back to the shared
queue using WRITE_ONCE() before advancing the consumer pointer, so
userspace observes consistent completion values.

Fixes: 8700e3e7c485 ("Soft RoCE driver")
Cc: stable@vger.kernel.org
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---
 drivers/infiniband/sw/rxe/rxe_comp.c  | 54 ++++++++++++++++++++++++++-
 drivers/infiniband/sw/rxe/rxe_verbs.h |  6 +++
 2 files changed, 58 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_comp.c b/drivers/infiniband/sw/rxe/rxe_comp.c
index 1390e861bd1d7..f57c4396c28cb 100644
--- a/drivers/infiniband/sw/rxe/rxe_comp.c
+++ b/drivers/infiniband/sw/rxe/rxe_comp.c
@@ -88,6 +88,18 @@ static inline unsigned long rnrnak_jiffies(u8 timeout)
 		usecs_to_jiffies(rnrnak_usec[timeout]), 1);
 }
 
+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);
+}
+
 static enum ib_wc_opcode wr_to_wc_opcode(enum ib_wr_opcode opcode)
 {
 	switch (opcode) {
@@ -142,17 +154,52 @@ static inline enum comp_state get_wqe(struct rxe_qp *qp,
 				      struct rxe_send_wqe **wqe_p)
 {
 	struct rxe_send_wqe *wqe;
+	u32 state;
+	unsigned int num_sge;
 
 	/* 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 = wqe;
+		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 */
+	num_sge = wqe->dma.num_sge;
+	if (unlikely(num_sge > RXE_MAX_SGE))
+		num_sge = RXE_MAX_SGE;
+
+	qp->comp.shared_wqe = wqe;
+	memcpy(&qp->comp.comp_wqe.wqe, wqe,
+	       sizeof(*wqe) + num_sge * sizeof(struct rxe_sge));
+	qp->comp.comp_wqe_valid = true;
+	qp->comp.comp_wqe.wqe.dma.num_sge = num_sge;
+
+	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 +501,9 @@ 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)
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 v3 2/2] RDMA/rxe: copy send WQE to kernel buffer in completer path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] rdma/rxe: TOCTOU mitigation bypass allows overwriting kernel DMA progress
- [High] rdma/rxe: out-of-bounds read in get_wqe() due to incorrect num_sge bounding
- [High] rdma/rxe: stale WQE reuse due to uncleared comp_wqe_valid on QP reset
- [High] rdma/rxe: TOCTOU double-fetch of num_sge due to missing READ_ONCE()
- [High] rdma/rxe: torn read of WQE state and status during memcpy causes successful completion of failed WQE
--

commit 925c6c168a2ebbc13c604cb14fd0fda0918233a7
Author: Tristan Madani <tristan@talencesecurity.com>

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

The commit modifies the rxe driver's completer path to copy Work Queue
Entries (WQEs) to a kernel-private buffer to prevent TOCTOU modifications
from userspace. The copied WQE is reused across multi-packet operations
when the state is unchanged to preserve DMA progress. Field updates are
written back to the shared queue to ensure consistent completion values.

> diff --git a/drivers/infiniband/sw/rxe/rxe_comp.c b/drivers/infiniband/sw/rxe/rxe_comp.c
> index 1390e861bd1d7..f57c4396c28cb 100644
> --- a/drivers/infiniband/sw/rxe/rxe_comp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_comp.c
[ ... ]
> @@ -142,17 +154,52 @@ static inline enum comp_state get_wqe(struct rxe_qp *qp,
>  				      struct rxe_send_wqe **wqe_p)
>  {
>  	struct rxe_send_wqe *wqe;
> +	u32 state;
> +	unsigned int num_sge;
>  
>  	/* 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 = wqe;
> +		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) {

[Severity: Critical]
If a malicious userspace application alters wqe->state in the shared ring
buffer mid-operation to a different but non-terminating value, will this
equality check fail?

If it fails, the completer will fall through and recopy the entire WQE from
shared memory into the kernel buffer qp->comp.comp_wqe.

Will this recopy overwrite the kernel's valid DMA progress (resid,
sge_offset, cur_sge) with the newly crafted dma state and lengths from
userspace, defeating the protection?

[Severity: High]
When a Queue Pair (QP) is transitioned to the RESET state via ib_modify_qp(),
rxe_qp_reset() is called. Does rxe_qp_reset() clear the newly introduced
qp->comp.comp_wqe_valid and qp->req.send_wqe_valid flags?

If these flags are not cleared, and userspace posts a new WQE after restart,
it is placed at queue index 0. The memory address wqe would evaluate to the
exact same pointer as the pre-reset operation.

Would this cause the completer to unexpectedly reuse the stale kernel-private
WQE, ignoring the newly posted operation's parameters?

> +		wqe = &qp->comp.comp_wqe.wqe;
> +		*wqe_p = wqe;
> +		goto check_state;
> +	}
> +
> +	/* Copy shared WQE to kernel-private buffer */
> +	num_sge = wqe->dma.num_sge;
> +	if (unlikely(num_sge > RXE_MAX_SGE))
> +		num_sge = RXE_MAX_SGE;
> +
> +	qp->comp.shared_wqe = wqe;
> +	memcpy(&qp->comp.comp_wqe.wqe, wqe,
> +	       sizeof(*wqe) + num_sge * sizeof(struct rxe_sge));

[Severity: High]
Since wqe->dma.num_sge is read from an mmap'd shared memory queue into num_sge
without using READ_ONCE(), is the C compiler permitted to optimize the code
by reloading the value from memory during the memcpy size calculation?

If an attacker in userspace concurrently modifies wqe->dma.num_sge to a large
value after the bounds check, could the re-fetched value bypass the
RXE_MAX_SGE limit and cause memcpy to write out of bounds into the fixed-size
qp->comp.comp_wqe kernel buffer?

[Severity: High]
The code caps num_sge against the global maximum RXE_MAX_SGE (32) instead
of the queue's specific maximum qp->sq.max_sge.

If max_sge was configured to a small value (e.g., 1), the allocated queue
elements are much smaller. If a malicious element is near the end of the
vmalloc_user ring buffer, could the memcpy read past the allocated queue
memory?

Could this hit an unmapped guard page and cause a fatal kernel page fault,
or leak adjacent queue elements?

[Severity: High]
Because memcpy uses unordered loads to copy the WQE from shared memory, could it
experience a torn read where it captures a new wqe->state (wqe_state_error)
but an old wqe->status (IB_WC_SUCCESS)?

> +	qp->comp.comp_wqe_valid = true;
> +	qp->comp.comp_wqe.wqe.dma.num_sge = num_sge;
> +
> +	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;

If get_wqe() later checks the local copy's state directly:

check_state:
	/* WQE caused an error */
	if (wqe->state == wqe_state_error)
		return COMPST_ERROR;

Will the state machine transition to COMPST_ERROR but process the completion
as IB_WC_SUCCESS because the torn read missed the status update?

Does the local variable wqe need to be updated with the cleanly loaded state
variable from earlier, or should the status and state be read more carefully?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007212737.1989004-1-tristmd@gmail.com?part=2