[PATCH] RDMA/rxe: validate receive WQE cursor after snapshot

[PATCH] RDMA/rxe: validate receive WQE cursor after snapshot

[Sung Byeongchan]

Receive WQEs are held in queues which are writable by userspace.  The
responder bounds num_sge before copying a WQE to its private snapshot, but
does not validate cur_sge.  A non-empty SEND can therefore make copy_data()
select SGE metadata outside the populated receive SGE array.

Validate the immutable snapshot before installing it in the responder.
Require the copied num_sge to match the value used to size the copy and
require cur_sge to name a populated SGE whenever residual data remains.
Apply the same checks to per-QP and shared receive queues.

This was found by source review with AI assistance.  An unprivileged
RXE queue owner reproduced a kernel fault twice with num_sge set to 1,
resid set to 8, and cur_sge set to UINT_MAX.  The fixed kernel rejected
that WQE, while an ordinary eight-byte receive completed successfully.

Fixes: 8700e3e7c485 ("Soft RoCE driver")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Sung Byeongchan <tjdqudcks0424@naver.com>
---
 drivers/infiniband/sw/rxe/rxe_resp.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
index 02b16e2b49b8f..f059434eead13 100644
--- a/drivers/infiniband/sw/rxe/rxe_resp.c
+++ b/drivers/infiniband/sw/rxe/rxe_resp.c
@@ -288,8 +288,15 @@ static enum resp_states get_srq_wqe(struct rxe_qp *qp)
 	}
 	size = sizeof(*wqe) + num_sge * sizeof(struct rxe_sge);
 	memcpy(&qp->resp.srq_wqe, wqe, size);
+	wqe = &qp->resp.srq_wqe.wqe;
+	if (unlikely(wqe->dma.num_sge != num_sge ||
+		     (wqe->dma.resid && wqe->dma.cur_sge >= num_sge))) {
+		spin_unlock_irqrestore(&srq->rq.consumer_lock, flags);
+		rxe_dbg_qp(qp, "invalid num_sge/cur_sge in SRQ entry\n");
+		return RESPST_ERR_MALFORMED_WQE;
+	}
 
-	qp->resp.wqe = &qp->resp.srq_wqe.wqe;
+	qp->resp.wqe = wqe;
 	queue_advance_consumer(q, QUEUE_TYPE_FROM_CLIENT);
 	count = queue_count(q, QUEUE_TYPE_FROM_CLIENT);
 
@@ -328,8 +335,14 @@ static enum resp_states rxe_get_recv_wqe(struct rxe_qp *qp)
 	}
 	size = sizeof(*wqe) + num_sge * sizeof(struct rxe_sge);
 	memcpy(&qp->resp.srq_wqe, wqe, size);
+	wqe = &qp->resp.srq_wqe.wqe;
+	if (unlikely(wqe->dma.num_sge != num_sge ||
+		     (wqe->dma.resid && wqe->dma.cur_sge >= num_sge))) {
+		rxe_dbg_qp(qp, "invalid num_sge/cur_sge in recv WQE\n");
+		return RESPST_ERR_MALFORMED_WQE;
+	}
 
-	qp->resp.wqe = &qp->resp.srq_wqe.wqe;
+	qp->resp.wqe = wqe;
 	return RESPST_CHK_LENGTH;
 }
 
-- 
2.43.0

Re: [PATCH] RDMA/rxe: validate receive WQE cursor after snapshot

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] rdma/rxe: malformed WQE check wedges SRQ and drops error CQE
- [High] rxe: Missing READ_ONCE() on user-mapped num_sge allows double-fetch
--

commit 9a901f2aad88cef9dab9b3e49e87d7bb3dc7cd00
Author: Sung Byeongchan <tjdqudcks0424@naver.com>

RDMA/rxe: validate receive WQE cursor after snapshot

This commit adds validation for the receive WQE cursor before installing the
snapshot in the responder. It ensures that the copied num_sge matches the
size used for the copy and requires cur_sge to be within bounds. The checks
are applied to both per-QP and shared receive queues.

> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
> index 02b16e2b49b8f..f059434eead13 100644
> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
> @@ -288,8 +288,15 @@ static enum resp_states get_srq_wqe(struct rxe_qp *qp)
>  	}
>  	size = sizeof(*wqe) + num_sge * sizeof(struct rxe_sge);

[Severity: High]
Since num_sge is read from user-mapped memory in wqe->dma.num_sge earlier in
the function without READ_ONCE(), can the compiler re-fetch the value for this
size calculation or the snapshot validation check below?

If the compiler re-fetches the value after a malicious user modifies it, could
it lead to a heap buffer overflow in memcpy(), or bypass the
wqe->dma.num_sge != num_sge equality check?

>  	memcpy(&qp->resp.srq_wqe, wqe, size);
> +	wqe = &qp->resp.srq_wqe.wqe;
> +	if (unlikely(wqe->dma.num_sge != num_sge ||
> +		     (wqe->dma.resid && wqe->dma.cur_sge >= num_sge))) {
> +		spin_unlock_irqrestore(&srq->rq.consumer_lock, flags);
> +		rxe_dbg_qp(qp, "invalid num_sge/cur_sge in SRQ entry\n");
> +		return RESPST_ERR_MALFORMED_WQE;
> +	}

[Severity: High]
If we return early here, we skip calling queue_advance_consumer(). Will this
permanently wedge the shared receive queue because the malformed WQE is never
consumed, potentially causing a denial of service to all other connections
sharing this queue?

Additionally, because qp->resp.wqe is never set, when do_complete() is called
later:

drivers/infiniband/sw/rxe/rxe_resp.c:do_complete() {
    ...
    if (!wqe)
        goto finish;
    ...
}

Will this incorrectly skip generating the mandatory error CQE for the
malformed WQE, violating the IB specification (Section 10.8.3.1)?

>  
> -	qp->resp.wqe = &qp->resp.srq_wqe.wqe;
> +	qp->resp.wqe = wqe;
>  	queue_advance_consumer(q, QUEUE_TYPE_FROM_CLIENT);
>  	count = queue_count(q, QUEUE_TYPE_FROM_CLIENT);

[ ... ]

> @@ -328,8 +335,14 @@ static enum resp_states rxe_get_recv_wqe(struct rxe_qp *qp)
>  	}
>  	size = sizeof(*wqe) + num_sge * sizeof(struct rxe_sge);
>  	memcpy(&qp->resp.srq_wqe, wqe, size);
> +	wqe = &qp->resp.srq_wqe.wqe;
> +	if (unlikely(wqe->dma.num_sge != num_sge ||
> +		     (wqe->dma.resid && wqe->dma.cur_sge >= num_sge))) {
> +		rxe_dbg_qp(qp, "invalid num_sge/cur_sge in recv WQE\n");
> +		return RESPST_ERR_MALFORMED_WQE;
> +	}

[Severity: High]
Similarly to get_srq_wqe(), does returning here without setting qp->resp.wqe
cause do_complete() to skip generating the proper error CQE?

While the WQE is eventually consumed by flush_recv_queue() for normal QPs,
does skipping do_complete() here result in an incorrect generic flush error
CQE instead of the required local QP operation error CQE?

>  
> -	qp->resp.wqe = &qp->resp.srq_wqe.wqe;
> +	qp->resp.wqe = wqe;
>  	return RESPST_CHK_LENGTH;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007063029.26073-1-tjdqudcks0424@naver.com?part=1