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

[sashiko-bot@kernel.org]

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