Re: [PATCH v6 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 5 potential issue(s) to consider:
- [Critical] RDMA/rxe: missing dma.cur_sge bounds check in get_wqe() allows OOB read
- [High] RDMA/rxe: wrong opcode bound in get_wqe() breaks IB_WR_REG_MR and wedges SQ
- [High] RDMA/rxe: stale comp_wqe and unshared DMA state in get_wqe() break READ retry
- [High] RDMA/rxe: TOCTOU races on wqe->state and shared wqe pointer in get_wqe()
- [High] RDMA/rxe: field-spanning memcpy() in get_wqe() triggers FORTIFY_SOURCE warn
--

commit 85eed619ea208c64c4708d3eee07f43c50d2a7e2
Author: Tristan Madani <tristmd@gmail.com>

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

This commit copies send WQEs from the shared queue to a completer-private
kernel buffer in get_wqe() and sanitizes fields before processing
completions. It also writes back completion status and rd_atomic state
before advancing the consumer index.

> diff --git a/drivers/infiniband/sw/rxe/rxe_comp.c b/drivers/infiniband/sw/rxe/rxe_comp.c
> index 1390e861bd1d7..6e72cfeb10ec8 100644
> --- a/drivers/infiniband/sw/rxe/rxe_comp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_comp.c
> @@ -137,22 +137,76 @@ 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);
> +}
> +
>  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 = 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;
> +	}

[Severity: High]
How does this reuse check interact with retries of partially completed
multi-packet RDMA READs?

During a multi-packet RDMA READ response, do_read() calls copy_data() on
&wqe->dma, which updates dma.resid, dma.cur_sge, and dma.sge_offset only in
qp->comp.comp_wqe.wqe. Because rxe_comp_writeback_wqe() only writes back
status and has_rd_atomic inside do_complete(), qp->comp.shared_wqe->dma in
qp->sq.queue is never updated as packets arrive.

If a timeout or NAK triggers a retry, req_retry() in
drivers/infiniband/sw/rxe/rxe_req.c reads wqe->dma.resid from the shared WQE
in qp->sq.queue:

	if (mask & WR_READ_MASK) {
		npsn = (wqe->dma.length - wqe->dma.resid) /
			qp->mtu;
		wqe->iova += npsn * qp->mtu;
	}

Since shared_wqe->dma.resid was never decremented, npsn is 0, wqe->iova is
not advanced, and rxe_requester() re-requests the entire read from offset 0
with new first_psn and last_psn values while setting shared_wqe->state back
to wqe_state_pending.

In addition, qp->comp.comp_wqe_valid is not invalidated on retry. When the
retried read response arrives, get_wqe() sees:

	qp->comp.comp_wqe_valid && qp->comp.shared_wqe == wqe &&
	qp->comp.comp_wqe.wqe.state == state

(both wqe_state_pending) and reuses the pre-retry qp->comp.comp_wqe.wqe with
the stale first_psn and last_psn. Doesn't check_ack() then fail the PSN
comparison against wqe->first_psn and return COMPST_ERROR, moving the QP to
IB_QPS_ERR?

> +
> +	/* Copy shared WQE to kernel-private buffer. Use max_inline
> +	 * as copy size since it covers both SGEs and inline data,
> +	 * which share the flex array.
> +	 */
> +	qp->comp.shared_wqe = wqe;
> +	memcpy(&qp->comp.comp_wqe.wqe, wqe,
> +	       sizeof(*wqe) + qp->sq.max_inline);
> +	qp->comp.comp_wqe_valid = true;

[Severity: High]
Are there remaining TOCTOU windows on wqe->state and the shared wqe pointer
in get_wqe()?

1. When state == wqe_state_posted above, get_wqe() sets *wqe_p = wqe
(pointing directly to the uncopied, unsanitized shared queue element) and
returns COMPST_EXIT when pkt is NULL. In rxe_completer():

	case COMPST_EXIT:
		if (qp->comp.timeout_retry && wqe) {
			state = COMPST_ERROR_RETRY;
			break;
		}
		...
	case COMPST_ERROR_RETRY:
		/* there is nothing to retry in this case */
		if (!wqe || (wqe->state == wqe_state_posted))
			goto exit;

If wqe->state in shared memory changes after get_wqe() read it (for example,
via concurrent userspace modification of the mmap'd send queue) and
qp->comp.retry_cnt == 0, COMPST_ERROR_RETRY transitions to COMPST_ERROR and
calls do_complete(qp, wqe) directly on the uncopied shared WQE. Should
*wqe_p be set to NULL when state == wqe_state_posted?

2. When state != wqe_state_posted, memcpy() overwrites
qp->comp.comp_wqe.wqe.state with a second read from shared memory rather
than preserving the local state variable loaded via smp_load_acquire(). If
userspace mutates wqe->state in shared memory between smp_load_acquire() and
memcpy(), check_state can observe wqe_state_posted or wqe_state_error (with
wqe->status == IB_WC_SUCCESS, triggering WARN_ON_ONCE in COMPST_ERROR). Also,
across multi-packet responses, userspace can flip shared_wqe->state in the
mmap'd ring to make qp->comp.comp_wqe.wqe.state == state false and force
get_wqe() to overwrite comp_wqe mid-operation.

[Severity: High]
Also, in struct rxe_comp_info in drivers/infiniband/sw/rxe/rxe_verbs.h,
comp_wqe is defined as:

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

Because memcpy() here (and in req_next_wqe() in
drivers/infiniband/sw/rxe/rxe_req.c from the previous patch) passes the inner
member &qp->comp.comp_wqe.wqe instead of the outer struct &qp->comp.comp_wqe,
__member_size(&qp->comp.comp_wqe.wqe) is sizeof(struct rxe_send_wqe). Will
copying sizeof(*wqe) + qp->sq.max_inline bytes trigger a
CONFIG_FORTIFY_SOURCE field-spanning write warning whenever
qp->sq.max_inline > 0?

> +
> +	/* Sanitize attacker-controlled fields from the copy */
> +	if (unlikely(qp->comp.comp_wqe.wqe.wr.opcode > IB_WR_ATOMIC_WRITE)) {
> +		qp->comp.comp_wqe_valid = false;
> +		*wqe_p = NULL;
> +		return pkt ? COMPST_DONE : COMPST_EXIT;
> +	}

[Severity: High]
Won't checking wr.opcode > IB_WR_ATOMIC_WRITE reject kernel-issued
IB_WR_REG_MR work requests and wedge the send queue?

In include/rdma/ib_verbs.h, enum ib_wr_opcode defines:

	IB_WR_FLUSH = IB_UVERBS_WR_FLUSH,
	IB_WR_ATOMIC_WRITE = IB_UVERBS_WR_ATOMIC_WRITE,

	/* These are kernel only and can not be issued by userspace */
	IB_WR_REG_MR = 0x20,

IB_WR_ATOMIC_WRITE is 15, while IB_WR_REG_MR is 0x20 (32), which is why
wr_opcode_mask() in drivers/infiniband/sw/rxe/rxe_loc.h checks
opcode > IB_WR_REG_MR and wr_to_wc_opcode() in
drivers/infiniband/sw/rxe/rxe_comp.c handles IB_WR_REG_MR. Checking
wr.opcode > IB_WR_ATOMIC_WRITE here (as well as in req_next_wqe() in
drivers/infiniband/sw/rxe/rxe_req.c) rejects every IB_WR_REG_MR WQE posted by
kernel ULPs.

Furthermore, when this check triggers in get_wqe(), it sets *wqe_p = NULL and
returns COMPST_DONE or COMPST_EXIT without advancing the SQ consumer index or
transitioning to COMPST_ERROR. Because the WQE remains at the head of
qp->sq.queue forever (and COMPST_EXIT sees wqe == NULL, skipping
COMPST_ERROR_RETRY and calling reset_retry_timer(qp)), doesn't this
permanently wedge the send queue and re-arm retrans_timer in an infinite loop?

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

[Severity: Critical]
Does get_wqe() also need to validate dma.cur_sge from the copied WQE?

In a user QP, shared_wqe->dma.cur_sge resides in the userspace-mapped send
queue buffer and can be modified by userspace before get_wqe() copies the WQE.
While dma.num_sge is clamped to qp->sq.max_sge here, dma.cur_sge is left
unchecked. When an RDMA READ or ATOMIC response arrives, do_read() or
do_atomic() passes &wqe->dma to copy_data() in
drivers/infiniband/sw/rxe/rxe_mr.c:

	struct rxe_sge		*sge	= &dma->sge[dma->cur_sge];
	u32			offset	= dma->sge_offset;
	u32			resid	= dma->resid;
	...
	if (sge->length && (offset < sge->length)) {
		mr = lookup_mr(pd, access, sge->lkey, RXE_LOOKUP_LOCAL);

Can an out-of-bounds dma.cur_sge index past qp->comp.comp_wqe.sge[RXE_MAX_SGE]
and read arbitrary kernel memory in copy_data()?

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