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

在 2026/8/16 3:44, Tristan Madani 写道:
> From: Tristan Madani <[email protected]>
>
> 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). State
> changes are written back to the shared queue so the completer can observe
> them (e.g., wqe_state_pending for RC acknowledgment processing).
>
> The copy is invalidated when:
>   - The WQE index advances (last packet sent, error, local ops, UD oversized)
>   - A retry occurs (req_retry resets WQE state in shared memory)
>
> The num_sge field is validated against qp->sq.max_sge on copy-in to
> reject corrupted values early.
>
> Fixes: 8700e3e7c485 ("Soft RoCE driver")
> Cc: [email protected]
> Signed-off-by: Tristan Madani <[email protected]>
> ---
>   drivers/infiniband/sw/rxe/rxe_req.c   | 55 ++++++++++++++++++++++++++-
>   drivers/infiniband/sw/rxe/rxe_verbs.h |  6 +++
>   2 files changed, 60 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
> index 12d03f390b097..499f398b674c7 100644
> --- a/drivers/infiniband/sw/rxe/rxe_req.c
> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
> @@ -161,6 +161,20 @@ static void req_check_sq_drain_done(struct rxe_qp *qp)
>   	spin_unlock_irqrestore(&qp->state_lock, flags);
>   }
>   
> +/* Write local WQE copy back to shared queue so the completer
> + * can observe state transitions (e.g., wqe_state_pending).
> + */
> +static void rxe_req_writeback_wqe(struct rxe_qp *qp)
> +{
> +	if (qp->req.send_wqe_valid && qp->req.shared_wqe) {
> +		unsigned int num_sge = qp->req.send_wqe.wqe.dma.num_sge;
> +		size_t size = sizeof(struct rxe_send_wqe) +
> +			      num_sge * sizeof(struct rxe_sge);
> +
> +		memcpy(qp->req.shared_wqe, &qp->req.send_wqe.wqe, size);
> +	}
> +}
> +
rxe_req_writeback_wqe() uses memcpy() to write the local WQE state back
to qp->req.shared_wqe in user-mapped memory without synchronization.

This introduces two data race hazards:
1. Malicious or concurrent userspace writes during memcpy() can race
    with the kernel writeback, causing tearing or inconsistent shared state.
2. The kernel completer tasklet concurrently reads shared_wqe without
    holding a lock, risking torn reads of intermediate WQE states (e.g.
    wqe_state_pending) and triggering state machine corruption.

Writebacks to shared user-mapped memory must update state fields atomically
or execute under appropriate lock serialization rather than performing an

uncontrolled bulk memcpy().

Please fix the above problem with appropriate synchronize methods.

Thanks a lot.

Yanjun.Zhu

>   static struct rxe_send_wqe *__req_next_wqe(struct rxe_qp *qp)
>   {
>   	struct rxe_queue *q = qp->sq.queue;
> @@ -178,6 +192,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 +209,29 @@ static struct rxe_send_wqe *req_next_wqe(struct rxe_qp *qp)
>   	}
>   	spin_unlock_irqrestore(&qp->state_lock, flags);
>   
> +	/* If we already have a valid local copy of this WQE, use it.
> +	 * This preserves DMA progress state across multi-packet sends.
> +	 */
> +	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. Userspace can concurrently modify num_sge, SGE entries,
> +	 * or inline data offsets, leading to inconsistent state in
> +	 * copy_data(). This is the send-path variant of the receive-path
> +	 * fix in rxe_get_recv_wqe().
> +	 */
> +	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 +621,14 @@ static void update_state(struct rxe_qp *qp, struct rxe_pkt_info *pkt)
>   {
>   	qp->req.opcode = pkt->opcode;
>   
> -	if (pkt->mask & RXE_END_MASK)
> +	/* Write back local WQE state before possibly advancing index */
> +	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;
>   
> @@ -635,6 +679,7 @@ 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;
>   	qp->req.wqe_index = queue_next_index(qp->sq.queue, qp->req.wqe_index);
> +	qp->req.send_wqe_valid = false;
>   
>   	return 0;
>   }
> @@ -695,6 +740,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);
> @@ -761,6 +807,7 @@ int rxe_requester(struct rxe_qp *qp)
>   						       qp->req.wqe_index);
>   			wqe->state = wqe_state_done;
>   			wqe->status = IB_WC_SUCCESS;
> +			qp->req.send_wqe_valid = false;
>   			goto done;
>   		}
>   		payload = mtu;
> @@ -820,12 +867,18 @@ int rxe_requester(struct rxe_qp *qp)
>   	 * will continue looping and return to rxe_requester
>   	 */
>   done:
> +	/* Write back local WQE for paths that skip update_state()
> +	 * (local ops, UD oversized packets).
> +	 */
> +	rxe_req_writeback_wqe(qp);
>   	ret = 0;
>   	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.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 {

-- 
Best Regards,
Yanjun.Zhu