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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] rdma/rxe: data corruption in RDMA READ retry due to private WQE DMA state
- [High] rdma/rxe: TOCTOU / lockless torn read of wqe->state during memcpy
- [High] RDMA/rxe: memcpy into WQE buffer triggers FORTIFY_SOURCE panic
- [High] RDMA/rxe: TOCTOU bypass in completer WQE copy via timeout retry path
--

commit 59f69c9fb12619fc21466b561c2742dd05e89831
Author: Tristan Madani <tristan@talencesecurity.com>
Date:   Wed Oct 7 22:32:22 2026 +0000

    RDMA/rxe: copy send WQE to kernel buffer in completer path
    
This patch aims to prevent TOCTOU races by copying send WQEs to a kernel
private buffer during completion processing. It uses an acquire/release
pattern to observe state transitions and writes back completion status
and rd_atomic state to the shared memory queue.

> diff --git a/drivers/infiniband/sw/rxe/rxe_comp.c b/drivers/infiniband/sw/rxe/rxe_comp.c
> index 1390e861bd1d7..ca31093fdf3aa 100644
> --- a/drivers/infiniband/sw/rxe/rxe_comp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_comp.c
[ ... ]
> +/* 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);
> +}

[Severity: Critical]
Does this omit writing back the advanced dma.resid and dma.sge_offset fields
to the shared WQE?

If a multi-packet RDMA READ response experiences a network drop and triggers
a requester retry, the requester will read the shared WQE where dma.resid
still equals dma.length. This causes it to re-request data from the original
starting iova.

>  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;
[ ... ]
> +	/* 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;
> +	}

[Severity: High]
If state is wqe_state_posted, this assigns the shared memory pointer directly
to wqe_p without making a local copy.

If a timeout triggers this path, rxe_completer() transitions to
COMPST_ERROR_RETRY where it re-reads wqe->state from this shared pointer.
Could a concurrent userspace modification change the state to bypass the
wqe_state_posted check, leading to COMPST_ERROR where it re-reads
wqe->status directly from shared memory?

If userspace races to set wqe->status to IB_WC_SUCCESS, could this trigger
the WARN_ON_ONCE(wqe->status == IB_WC_SUCCESS) in rxe_completer() and
subsequently process a corrupted WQE?

> +
> +	/* 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;
> +	}

[Severity: Critical]
Could this reuse logic cause memory corruption during an RDMA READ retry?

Because rxe_comp_writeback_wqe() does not sync the advanced DMA state to
shared memory, a retried request will re-request the beginning of the
data. However, the requester's retry rewinds the shared WQE state to
wqe_state_posted and back to wqe_state_pending.

This allows the state check here to succeed, meaning the completer reuses
its private WQE containing the advanced DMA pointer. Won't the completer
then write the re-transmitted beginning data into the middle of the local
buffer?

> +
> +	/* Copy shared WQE to kernel-private buffer. Copy the full
> +	 * element (max_sge SGEs) so that inline data, which shares
> +	 * the flex array with SGEs, is always captured.
> +	 */
> +	qp->comp.shared_wqe = wqe;
> +	memcpy(&qp->comp.comp_wqe.wqe, wqe,
> +	       sizeof(*wqe) + qp->sq.max_sge * sizeof(struct rxe_sge));

[Severity: High]
This problem wasn't introduced by this patch, but it appears here and was
also introduced in the requester path in the previous patch of this series.

Because the destination pointer specifically targets the wqe field
(&qp->comp.comp_wqe.wqe), the compiler evaluates the object size strictly
as sizeof(struct rxe_send_wqe).

When max_sge > 0, the size exceeds the bounds of the wqe field. When
CONFIG_FORTIFY_SOURCE=y is enabled, will this trigger a runtime
field-spanning write warning or panic? Would it be safer to copy into
the enclosing struct address, such as &qp->comp.comp_wqe?

[Severity: High]
Is there a Time-of-Check to Time-of-Use race when copying the state?

The memcpy is an unordered bulk copy that re-reads wqe->state from shared
memory. A concurrent requester could update the WQE fields in shared memory
during this copy. Because memcpy provides no memory ordering, it could copy
the new state but an old status.

> +	qp->comp.comp_wqe_valid = true;
> +	if (qp->comp.comp_wqe.wqe.dma.num_sge > qp->sq.max_sge)
> +		qp->comp.comp_wqe.wqe.dma.num_sge = qp->sq.max_sge;
> +
> +	wqe = &qp->comp.comp_wqe.wqe;
> +	*wqe_p = wqe;
>  
> +check_state:
>  	/* WQE does not require an ack */
> -	if (wqe->state == wqe_state_done)
> +	if (wqe->state == wqe_state_done)

[Severity: High]
Here, the code checks if wqe->state == wqe_state_done using the value
inside the locally copied WQE.

Since this evaluates the re-read state from the memcpy rather than the
state local variable acquired safely via smp_load_acquire() earlier, could
this defeat the memory barrier and cause the completer to process a done
WQE with stale fields?

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