Re: [PATCH v8 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 2 potential issue(s) to consider:
- [High] RDMA/rxe: stale comp_wqe cache across RNR retry in rxe_completer()
- [High] RDMA/rxe: unvalidated shared state and status in get_wqe() hit WARN_ON_ONCE
--

commit 249927ff74c8311cd6d2280c79ed9338f558aefd
Author: Tristan Madani <tristmd@gmail.com>

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

This commit copies send WQEs from the userspace-mapped shared queue into a
completer-private buffer in get_wqe() to prevent TOCTOU races, reuses the
local copy while processing multi-packet responses, validates the opcode and
SGE fields, and writes back DMA progress on completion and retry.

> diff --git a/drivers/infiniband/sw/rxe/rxe_comp.c b/drivers/infiniband/sw/rxe/rxe_comp.c
> index 1390e861bd1d7..b8bc710dc75e9 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)

[ ... ]

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

[Severity: High]
Can userspace trigger the WARN_ON_ONCE(wqe->status == IB_WC_SUCCESS) in
rxe_completer() or force get_wqe() to overwrite qp->comp.comp_wqe mid-read?

In get_wqe() in drivers/infiniband/sw/rxe/rxe_comp.c, state is loaded from the
userspace-mapped shared queue and wqe (including wqe->status) is copied into
qp->comp.comp_wqe. At check_state, get_wqe() then checks:

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

When COMPST_ERROR runs in rxe_completer():

		case COMPST_ERROR:
			WARN_ON_ONCE(wqe->status == IB_WC_SUCCESS);
			do_complete(qp, wqe);
			rxe_qp_error(qp);
			goto exit;

If userspace writes wqe->state = wqe_state_error with wqe->status =
IB_WC_SUCCESS in the mmap'd send queue ring (or overwrites shared->status = 0
after rxe_req_writeback_wqe() sets wqe_state_error), get_wqe() returns
COMPST_ERROR without ensuring wqe->status != IB_WC_SUCCESS. This hits
WARN_ON_ONCE(wqe->status == IB_WC_SUCCESS) in rxe_completer() and causes
do_complete() to see IB_WC_SUCCESS, skipping mandatory error CQE generation
for unsignaled WRs.

In addition, get_wqe() only reuses qp->comp.comp_wqe when
qp->comp.comp_wqe.wqe.state == state. If userspace toggles wqe->state in the
shared ring between response packets of a multi-packet RDMA READ (for example
between wqe_state_pending and wqe_state_processing), won't get_wqe() discard
the cached local copy and overwrite qp->comp.comp_wqe from untrusted shared
memory on each packet?

[ ... ]

> @@ -809,6 +878,8 @@ int rxe_completer(struct rxe_qp *qp)
>  				 * rnr timer has fired
>  				 */
>  				qp->req.wait_for_rnr_timer = 1;
> +				rxe_comp_writeback_wqe(qp);
> +				qp->comp.comp_wqe_valid = false;
>  				rxe_dbg_qp(qp, "set rnr nak timer\n");

[Severity: High]
Can qp->comp.comp_wqe become stale across an RNR retry if rxe_completer()
runs again before rnr_nak_timer fires?

In rxe_completer() in drivers/infiniband/sw/rxe/rxe_comp.c, COMPST_RNR_RETRY
sets qp->req.wait_for_rnr_timer = 1, writes back and invalidates
qp->comp.comp_wqe_valid, and arms qp->rnr_nak_timer, while qp->req.need_retry
remains 0 until rnr_nak_timer() fires.

However, the early exit check at the top of rxe_completer() only checks
qp->req.need_retry:

	if (qp->req.need_retry)
		goto exit;

If rxe_completer() runs again before rnr_nak_timer expires (for example on
the next iteration of rxe_sender(), or when send_task is scheduled by
rxe_post_send(), rxe_comp_queue_pkt(), or rxe_skb_tx_dtor()), it enters
COMPST_GET_WQE -> get_wqe(). Because req_retry() has not run yet, the shared
WQE still has state == wqe_state_pending (or wqe_state_processing), so
get_wqe() copies the pre-retry WQE (with its old first_psn and last_psn) back
into qp->comp.comp_wqe and sets qp->comp.comp_wqe_valid = true.

When rnr_nak_timer() later fires, rxe_requester() in
drivers/infiniband/sw/rxe/rxe_req.c runs:

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

This does not clear qp->comp.comp_wqe_valid, and rxe_requester() immediately
retransmits the WQE with updated PSN bounds and sets shared->state back to
wqe_state_pending. When the retried response arrives, get_wqe() sees
qp->comp.comp_wqe_valid == true, qp->comp.shared_wqe == wqe, and
qp->comp.comp_wqe.wqe.state == state (both wqe_state_pending), so it reuses
the stale pre-retry comp_wqe. For a partially completed multi-packet RDMA
READ where first_psn advanced on retry, won't check_ack() compare pkt->psn
against the stale wqe->first_psn and transition the QP to COMPST_ERROR?

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