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

[sashiko-bot@kernel.org]

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