[PATCH v3] RDMA/rxe: Sanitize receive WQE in local buffer

[PATCH v3] RDMA/rxe: Sanitize receive WQE in local buffer

[Nicolas Morey]

For both SRQ and non-SRQ receive paths, the WQE is copied from shared
user memory into a local buffer to provide a kernel-owned copy. However,
several issues remain:

1. Double-fetch TOCTOU race: Reading wqe->dma.num_sge directly from
   shared memory allows the compiler to re-fetch it between the bounds
   check and memcpy(). Furthermore, memcpy() copies num_sge from
   shared memory, leaving an unvalidated value in the local buffer causing:

      BUG: KASAN: slab-out-of-bounds in rxe_receiver+0x8109/0x9ec0 [rdma_rxe]
      Read of size 4 at addr ffff88812c4867f8 by task kworker/u9:6/361
      Workqueue: rxe_wq do_work [rdma_rxe]
      Call Trace:
       rxe_receiver+0x8109/0x9ec0 [rdma_rxe]
       do_work+0x149/0x610 [rdma_rxe]
       process_one_work+0x726/0x10a0

      The buggy address belongs to the object at ffff88812c486000
       which belongs to the cache kmalloc-part-13-2k of size 2048
      The buggy address is located 0 bytes to the right of
       allocated 2040-byte region [ffff88812c486000, ffff88812c4867f8)

2. Uninitialized DMA state fields: cur_sge, sge_offset, length,
   and resid are copied directly from userspace without validation or
   initialization. A malicious or malformed WQE can supply an arbitrary
   cur_sge or sge_offset, leading to out-of-bounds array indexing in
   copy_data().

Consolidate WQE validation into a helper recv_wqe_sanitize() that
- Uses READ_ONCE() on user_wqe->dma.num_sge and sizes the copy with
  struct_size().
- Overwrites kernel_wqe->dma.num_sge with the validated value.
- Resets cur_sge and sge_offset to 0.
- Calculates and verifies length and resid from the SGE table using
  check_add_overflow() and bounds-checks against RXE_PORT_MAX_MSG_SZ.

Fixes: 8700e3e7c485 ("Soft RoCE driver")
Signed-off-by: Nicolas Morey <nmorey@suse.com>
---
v2 -> v3:
 - Extended fix to sanitize cur_sge, sge_offset, length, and resid (Sashiko).
 - Added READ_ONCE, struct_size, and check_add_overflow helpers.
 - Consolidated RQ and SRQ sanitization into recv_wqe_sanitize().
 - Dropped Reviewed-by tag due to substantial changes.

 drivers/infiniband/sw/rxe/rxe_resp.c | 70 ++++++++++++++++++++--------
 1 file changed, 51 insertions(+), 19 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
index 02b16e2b49b8..d686bad3c03c 100644
--- a/drivers/infiniband/sw/rxe/rxe_resp.c
+++ b/drivers/infiniband/sw/rxe/rxe_resp.c
@@ -257,6 +257,47 @@ static enum resp_states check_op_valid(struct rxe_qp *qp,
 	return RESPST_CHK_RESOURCE;
 }
 
+static enum resp_states recv_wqe_sanitize(struct rxe_qp *qp,
+					struct rxe_recv_wqe *kernel_wqe,
+					struct rxe_recv_wqe *user_wqe,
+					int max_sge)
+{
+	unsigned int num_sge;
+	unsigned long length = 0;
+	size_t size;
+	int i;
+
+	/* don't trust user space data */
+	num_sge = READ_ONCE(user_wqe->dma.num_sge);
+	if (unlikely(num_sge > max_sge)) {
+		rxe_dbg_qp(qp, "bad num_sge > max_sge\n");
+		return RESPST_ERR_MALFORMED_WQE;
+	}
+
+	size = struct_size(user_wqe, dma.sge, num_sge);
+	memcpy(kernel_wqe, user_wqe, size);
+
+	for (i = 0; i < num_sge; i++) {
+		if (check_add_overflow(length, kernel_wqe->dma.sge[i].length, &length)) {
+			rxe_dbg_qp(qp, "message length overflow\n");
+			return RESPST_ERR_MALFORMED_WQE;
+		}
+	}
+
+	if (unlikely(length > RXE_PORT_MAX_MSG_SZ)) {
+		rxe_dbg_qp(qp, "message length too long\n");
+		return RESPST_ERR_MALFORMED_WQE;
+	}
+
+	kernel_wqe->dma.length = length;
+	kernel_wqe->dma.resid = length;
+	kernel_wqe->dma.num_sge = num_sge;
+	kernel_wqe->dma.cur_sge = 0;
+	kernel_wqe->dma.sge_offset = 0;
+
+	return RESPST_NONE;
+}
+
 static enum resp_states get_srq_wqe(struct rxe_qp *qp)
 {
 	struct rxe_srq *srq = qp->srq;
@@ -264,9 +305,8 @@ static enum resp_states get_srq_wqe(struct rxe_qp *qp)
 	struct rxe_recv_wqe *wqe;
 	struct ib_event ev;
 	unsigned int count;
-	unsigned int num_sge;
-	size_t size;
 	unsigned long flags;
+	int err;
 
 	if (srq->error)
 		return RESPST_ERR_RNR;
@@ -279,17 +319,13 @@ static enum resp_states get_srq_wqe(struct rxe_qp *qp)
 		return RESPST_ERR_RNR;
 	}
 
-	/* don't trust user space data */
-	num_sge = wqe->dma.num_sge;
-	if (unlikely(num_sge > srq->rq.max_sge)) {
+	err = recv_wqe_sanitize(qp, &qp->resp.srq_wqe.wqe, wqe, srq->rq.max_sge);
+	if (err) {
 		spin_unlock_irqrestore(&srq->rq.consumer_lock, flags);
-		rxe_dbg_qp(qp, "invalid num_sge in SRQ entry\n");
-		return RESPST_ERR_MALFORMED_WQE;
+		return err;
 	}
-	size = sizeof(*wqe) + num_sge * sizeof(struct rxe_sge);
-	memcpy(&qp->resp.srq_wqe, wqe, size);
-
 	qp->resp.wqe = &qp->resp.srq_wqe.wqe;
+
 	queue_advance_consumer(q, QUEUE_TYPE_FROM_CLIENT);
 	count = queue_count(q, QUEUE_TYPE_FROM_CLIENT);
 
@@ -314,22 +350,18 @@ static enum resp_states rxe_get_recv_wqe(struct rxe_qp *qp)
 {
 	struct rxe_queue *q = qp->rq.queue;
 	struct rxe_recv_wqe *wqe;
-	unsigned int num_sge;
-	size_t size;
+	int err;
 
 	wqe = queue_head(q, QUEUE_TYPE_FROM_CLIENT);
 	if (!wqe)
 		return RESPST_ERR_RNR;
 
-	num_sge = wqe->dma.num_sge;
-	if (unlikely(num_sge > qp->rq.max_sge)) {
-		rxe_dbg_qp(qp, "invalid num_sge in recv WQE\n");
-		return RESPST_ERR_MALFORMED_WQE;
-	}
-	size = sizeof(*wqe) + num_sge * sizeof(struct rxe_sge);
-	memcpy(&qp->resp.srq_wqe, wqe, size);
+	err = recv_wqe_sanitize(qp, &qp->resp.srq_wqe.wqe, wqe, qp->rq.max_sge);
+	if (err)
+		return err;
 
 	qp->resp.wqe = &qp->resp.srq_wqe.wqe;
+
 	return RESPST_CHK_LENGTH;
 }
 
-- 
2.54.0

Re: [PATCH v3] RDMA/rxe: Sanitize receive WQE in local buffer

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Copying into the inner `wqe` field triggers a FORTIFY_SOURCE field-spanning write WARN_ONCE or panic.

Pre-existing issues:
- [Critical] Double-fetch TOCTOU vulnerability in the send path (`rxe_req.c`) leading to out-of-bounds memory access.
--

commit 5c427e8b4bbe51e8f334533fce0d0db77db27960
Author: Nicolas Morey <nmorey@suse.com>

RDMA/rxe: Sanitize receive WQE in local buffer

This patch sanitizes receive work queue elements by copying them to a local
buffer and validating their fields. This addresses TOCTOU races and
out-of-bounds accesses during the receive path.

> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
[ ... ]
> @@ -257,6 +257,47 @@ static enum resp_states check_op_valid(struct rxe_qp *qp,
>  	return RESPST_CHK_RESOURCE;
>  }
>  
> +static enum resp_states recv_wqe_sanitize(struct rxe_qp *qp,
> +					struct rxe_recv_wqe *kernel_wqe,
> +					struct rxe_recv_wqe *user_wqe,
> +					int max_sge)
> +{
[ ... ]
> +	size = struct_size(user_wqe, dma.sge, num_sge);
> +	memcpy(kernel_wqe, user_wqe, size);

[Severity: High]
Will this memcpy trigger a FORTIFY_SOURCE warning or kernel panic due to a
field-spanning write?

When recv_wqe_sanitize is called from get_srq_wqe, it is passed a pointer to
the inner wqe sub-object:

> +	err = recv_wqe_sanitize(qp, &qp->resp.srq_wqe.wqe, wqe, srq->rq.max_sge);

Because the destination pointer points specifically to the nested wqe struct
within the larger srq_wqe struct, the compiler evaluates the size of this
object to exactly 40 bytes. When memcpy attempts to write up to
40 + 16 * max_sge bytes, it violates this strict bounds check, resulting in a
denial of service on the receive path.

[ ... ]

[Severity: Critical]
This is a pre-existing issue, but I noticed the symmetric send path appears
to suffer from a very similar TOCTOU vulnerability that might lead to an
out-of-bounds memory access.

In rxe_req.c, rxe_requester performs a bounds check on cur_sge while the
data is still in shared memory:

        if (unlikely(wqe->dma.num_sge > qp->sq.max_sge ||
                     (wqe->dma.resid &&
                      wqe->dma.cur_sge >= qp->sq.max_sge))) {

However, a malicious user could potentially overwrite cur_sge immediately
after this check. Later, copy_data in rxe_mr.c re-fetches cur_sge directly
from the shared memory and uses it as an array index:

        struct rxe_sge          *sge    = &dma->sge[dma->cur_sge];

Could this bypass the validation and read memory outside the wqe if the
value is mutated between the check and the use?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910083712.1595862-1-nmorey@suse.com?part=1