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

Nicolas Morey posted 1 patch 2 weeks, 1 day ago
drivers/infiniband/sw/rxe/rxe_resp.c | 70 ++++++++++++++++++++--------
1 file changed, 51 insertions(+), 19 deletions(-)
[PATCH v3] RDMA/rxe: Sanitize receive WQE in local buffer
Posted by Nicolas Morey 2 weeks, 1 day ago
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