[PATCH v2] RDMA/rxe: Use validated num_sge in local buffer

[PATCH v2] RDMA/rxe: Use validated num_sge in local buffer

[Nicolas Morey]

For both SRQ and non-SRQ receive paths, the WQE is copied into a local
buffer to provide a kernel-owned, validated copy. While calculating the
memcpy size from the validated num_sge prevents overflow during the
copy, memcpy() itself still copies num_sge from shared memory.

A concurrent userspace modification before or during memcpy() leaves
an unvalidated num_sge in the local buffer, leading to potential
out-of-bounds reads in rxe_resp_check_length() and copy_data() 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)

Explicitly assign the validated num_sge to the local buffer after the
copy to prevent this race.

Fixes: 22b8fbded65b ("RDMA/rxe: Fix TOCTOU heap overflow in get_srq_wqe")
Fixes: d6ab440240a0 ("RDMA/rxe: Copy WQE to local buffer in non-SRQ receive path")
Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>
Signed-off-by: Nicolas Morey <nmorey@suse.com>
---
v1 -> v2:
 - Added KASAN call trace to the commit log
 - Collected Reviewed-by tag.

 drivers/infiniband/sw/rxe/rxe_resp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
index 02b16e2b49b8..cd51042857d6 100644
--- a/drivers/infiniband/sw/rxe/rxe_resp.c
+++ b/drivers/infiniband/sw/rxe/rxe_resp.c
@@ -288,6 +288,7 @@ 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);
+	qp->resp.srq_wqe.wqe.dma.num_sge = num_sge;
 
 	qp->resp.wqe = &qp->resp.srq_wqe.wqe;
 	queue_advance_consumer(q, QUEUE_TYPE_FROM_CLIENT);
@@ -328,6 +329,7 @@ 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);
+	qp->resp.srq_wqe.wqe.dma.num_sge = num_sge;
 
 	qp->resp.wqe = &qp->resp.srq_wqe.wqe;
 	return RESPST_CHK_LENGTH;
-- 
2.54.0

Re: [PATCH v2] RDMA/rxe: Use validated num_sge in local buffer

[sashiko-bot]

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

Pre-existing issues:
- [Critical] The receive WQE's DMA state fields (specifically `cur_sge`, `sge_offset`, `length`, and `resid`) are copied directly from shared user memory without validation or initialization. This leads to out-of-bounds array indexing and heap read/write in `copy_data()`.
- [Critical] The send path reads WQEs directly from the user-space shared ring buffer without making a local copy. All WQE fields, including `cur_sge`, `num_sge`, `sge_offset`, and `resid`, are subject to Time-Of-Check to Time-Of-Use (TOCTOU) races.
- [High] The local variable `num_sge` is read from shared user-memory `wqe->dma.num_sge` without using `READ_ONCE()`, creating a compiler double-fetch vulnerability between the bounds check and the `memcpy` size calculation.
--

commit 81af792b3b2b8089fc7af5abfac657516fe3ed47
Author: Nicolas Morey <nmorey@suse.com>

RDMA/rxe: Use validated num_sge in local buffer

This commit addresses a time-of-check to time-of-use heap overflow
in the receive path of rdma/rxe. It explicitly assigns the validated
num_sge to the locally copied wqe to prevent an attacker from modifying it
during the copy.

> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
> index b1ea720aa34f9..ba3b2d23b36b7 100644
> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
> @@ -288,6 +288,7 @@ 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);
> +	qp->resp.srq_wqe.wqe.dma.num_sge = num_sge;

[Severity: High]
This isn't a bug introduced by this patch, but does the read of num_sge
above this memcpy() in get_srq_wqe() require a READ_ONCE()?

	num_sge = wqe->dma.num_sge;
	if (unlikely(num_sge > srq->rq.max_sge)) {
		...
	}
	size = sizeof(*wqe) + num_sge * sizeof(struct rxe_sge);

Since wqe->dma.num_sge is in shared user-memory, can the compiler reload it
after the bounds check?

If an attacker modifies it concurrently in the shared mmap'd ring buffer,
could this result in an enormously large size calculation, causing memcpy()
to write far past the fixed-size local srq_wqe buffer into adjacent rxe_qp
fields?

[Severity: Critical]
This is a pre-existing issue, but are the other dma state fields like
cur_sge validated after being copied from shared memory here?

If an attacker crafts a wqe with an extremely large cur_sge, when
copy_data() in drivers/infiniband/sw/rxe/rxe_mr.c uses it as an array index:

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

could this result in an out-of-bounds kernel memory read past the local
srq_wqe structure and potential memory corruption?

>  
>  	qp->resp.wqe = &qp->resp.srq_wqe.wqe;
>  	queue_advance_consumer(q, QUEUE_TYPE_FROM_CLIENT);
> @@ -328,6 +329,7 @@ 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);
> +	qp->resp.srq_wqe.wqe.dma.num_sge = num_sge;
>  
>  	qp->resp.wqe = &qp->resp.srq_wqe.wqe;
>  	return RESPST_CHK_LENGTH;

[Severity: Critical]
This is also a pre-existing issue, but does the send path have a similar
time-of-check to time-of-use vulnerability that should be addressed?

In drivers/infiniband/sw/rxe/rxe_req.c:rxe_requester(), num_sge and cur_sge
are checked directly against the shared ring buffer without making a local
copy:

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

Can an attacker thread modify cur_sge to a massive value immediately after
this check?

Later in rxe_req.c:finish_packet(), the shared memory wqe pointer is passed
directly to copy_data():

	err = copy_data(qp->pd, 0, &wqe->dma,
			payload_addr(pkt), payload,
			RXE_FROM_MR_OBJ);

which recalculates the array index from the unvalidated shared memory:

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

Could this cause an out-of-bounds array access beyond the vmalloc allocation
of the ring buffer?

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