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

[Zhu Yanjun <yanjun.zhu@linux.dev>]

在 2026/10/30 12:32, Tristan Madani 写道:
> 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 copy uses the full queue element data size
> (max of max_sge SGEs and max_inline bytes) so that both SGE entries
> and inline data are always captured.
> 
> After copying, reject out-of-range opcodes by setting mask to 0 so that
> next_opcode() transitions the QP to error state. Clamp atomic write
> residual to exactly sizeof(u64) to prevent finish_packet() from reading
> past the WQE buffer or leaking uninitialized pad bytes. The WR-level
> mask (WR_ATOMIC_WRITE_MASK) is checked since wr_opcode_mask() populates
> wqe->mask from enum rxe_wr_mask.
> 
> The local copy is reused across multi-packet sends to preserve DMA
> progress state (cur_sge, sge_offset, resid). Field updates including
> DMA progress 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(). DMA progress writeback ensures retries
> can resume from the correct offset.
> 
> The copy is invalidated when the WQE index advances (last packet sent,
> error, local ops, UD oversized), when a retry resets WQE state, on QP
> reset, or when the QP enters error state for flush.
> 
> Fixes: 8700e3e7c485 ("Soft RoCE driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Tristan Madani <tristan@talencesecurity.com>

Thanks a lot for your efforts.
I am fine with this.

Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>

Zhu Yanjun

> ---
> 
> Changes since v9:
>    - Atomic write resid: check != sizeof(u64) instead of just > to also
>      catch undersized resid that would leak uninitialized skb pad bytes
>      (sashiko finding).
>    - Bisection fix: move qp->comp.comp_wqe_valid invalidation from
>      this patch to patch 2/2, since comp_wqe_valid is defined there
>      (sashiko finding).
> 
> Changes since v8:
>    - Fix atomic write resid check: use WR_ATOMIC_WRITE_MASK (WR-level
>      mask) instead of RXE_ATOMIC_WRITE_MASK (packet-level mask) so the
>      condition actually fires for IB_WR_ATOMIC_WRITE WQEs (sashiko
>      finding).
>    - Fix atomic write rejection: clamp dma.resid to sizeof(u64) instead
>      of setting mask to 0, since next_opcode() ignores wqe->mask and
>      uses wqe->wr.opcode directly (sashiko finding).
> 
> Changes since v7:
>    - Reject atomic write WQEs with oversized dma.resid to prevent
>      OOB read in finish_packet() (sashiko finding).
> 
> Changes since v6:
>    - Fixed all sashiko review findings (2 Critical + 9 High).
>    - FORTIFY_SOURCE: memcpy into enclosing struct, not inner member.
>    - Copy size: max(max_sge * sizeof(ib_sge), max_inline).
>    - Opcode bound: IB_WR_REG_MR (was IB_WR_ATOMIC_WRITE).
>    - Removed silent num_sge clamp, use mask=0 for invalid opcodes.
>    - DMA progress writeback (resid, cur_sge, sge_offset).
>    - Addressed Zhu Yanjun review feedback.
> 
> Changes since v5:
>    - Added opcode bounds check and num_sge clamp after copy.
> 
> Changes since v4:
>    - Resent from the correct sender address.
> 
> Changes since v3:
>    - Copy full queue element (max_sge SGEs) instead of per-WQE size.
>    - Invalidate on QP reset, ERR flush, RESET state check.
> 
> Changes since v2:
>    - WRITE_ONCE/smp_store_release for field writeback.
> 
> Changes since v1:
>    - Split into per-path patches. Cache across multi-packet ops.
> 
>   drivers/infiniband/sw/rxe/rxe_qp.c    |  1 +
>   drivers/infiniband/sw/rxe/rxe_req.c   | 71 +++++++++++++++++++++++++--
>   drivers/infiniband/sw/rxe/rxe_verbs.h |  6 +++
>   3 files changed, 73 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c
> index 311f285d78a6b..77606c4a039b1 100644
> --- a/drivers/infiniband/sw/rxe/rxe_qp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_qp.c
> @@ -581,6 +581,7 @@ static void rxe_qp_reset(struct rxe_qp *qp)
>   	qp->req.need_retry = 0;
>   	qp->req.wait_for_rnr_timer = 0;
>   	qp->req.noack_pkts = 0;
> +	qp->req.send_wqe_valid = false;
>   	qp->resp.msn = 0;
>   	qp->resp.opcode = -1;
>   	qp->resp.drop_msg = 0;
> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
> index 24f5c044363f7..dd8ad19ed1d86 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);
>   }
>   
> +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);
> +	WRITE_ONCE(shared->dma.resid, local->dma.resid);
> +	WRITE_ONCE(shared->dma.cur_sge, local->dma.cur_sge);
> +	WRITE_ONCE(shared->dma.sge_offset, local->dma.sge_offset);
> +	/* 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;
> @@ -193,7 +213,35 @@ static struct rxe_send_wqe *req_next_wqe(struct rxe_qp *qp)
>   	}
>   	spin_unlock_irqrestore(&qp->state_lock, flags);
>   
> -	wqe->mask = wr_opcode_mask(wqe->wr.opcode, qp);
> +	/* 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. Copy size matches the queue element data area.
> +	 */
> +	memcpy(&qp->req.send_wqe, wqe, sizeof(*wqe) +
> +	       max_t(int, qp->sq.max_sge * sizeof(struct ib_sge),
> +		    qp->sq.max_inline));
> +	qp->req.shared_wqe = wqe;
> +	qp->req.send_wqe_valid = true;
> +
> +	wqe = &qp->req.send_wqe.wqe;
> +
> +	/* Prevent OOB in wr_opcode_mask() for corrupted opcodes.
> +	 * Mask 0 causes next_opcode() to fail, moving QP to error state.
> +	 */
> +	if (unlikely(wqe->wr.opcode > IB_WR_REG_MR))
> +		wqe->mask = 0;
> +	else
> +		wqe->mask = wr_opcode_mask(wqe->wr.opcode, qp);
> +
> +	/* Atomic writes carry exactly sizeof(u64) payload.
> +	 * Clamp oversized resid to prevent OOB read in finish_packet().
> +	 */
> +	if (unlikely((wqe->mask & WR_ATOMIC_WRITE_MASK) &&
> +		     wqe->dma.resid != sizeof(u64)))
> +		wqe->dma.resid = sizeof(u64);
>   	return wqe;
>   }
>   
> @@ -582,9 +630,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 +686,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;
>   }
> @@ -666,6 +720,7 @@ int rxe_requester(struct rxe_qp *qp)
>   		wqe = __req_next_wqe(qp);
>   		spin_unlock_irqrestore(&qp->state_lock, flags);
>   		if (wqe) {
> +			qp->req.send_wqe_valid = false;
>   			wqe->status = IB_WC_WR_FLUSH_ERR;
>   			goto err;
>   		} else {
> @@ -681,6 +736,7 @@ int rxe_requester(struct rxe_qp *qp)
>   		qp->req.wait_psn = 0;
>   		qp->req.need_retry = 0;
>   		qp->req.wait_for_rnr_timer = 0;
> +		qp->req.send_wqe_valid = false;
>   		spin_unlock_irqrestore(&qp->state_lock, flags);
>   		goto exit;
>   	}
> @@ -695,6 +751,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 +829,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 +898,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 {