Re: [PATCH v3 2/2] RDMA/rxe: copy send WQE to kernel buffer in completer path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] rdma/rxe: TOCTOU mitigation bypass allows overwriting kernel DMA progress
- [High] rdma/rxe: out-of-bounds read in get_wqe() due to incorrect num_sge bounding
- [High] rdma/rxe: stale WQE reuse due to uncleared comp_wqe_valid on QP reset
- [High] rdma/rxe: TOCTOU double-fetch of num_sge due to missing READ_ONCE()
- [High] rdma/rxe: torn read of WQE state and status during memcpy causes successful completion of failed WQE
--

commit 925c6c168a2ebbc13c604cb14fd0fda0918233a7
Author: Tristan Madani <tristan@talencesecurity.com>

    RDMA/rxe: copy send WQE to kernel buffer in completer path

The commit modifies the rxe driver's completer path to copy Work Queue
Entries (WQEs) to a kernel-private buffer to prevent TOCTOU modifications
from userspace. The copied WQE is reused across multi-packet operations
when the state is unchanged to preserve DMA progress. Field updates are
written back to the shared queue to ensure consistent completion values.

> diff --git a/drivers/infiniband/sw/rxe/rxe_comp.c b/drivers/infiniband/sw/rxe/rxe_comp.c
> index 1390e861bd1d7..f57c4396c28cb 100644
> --- a/drivers/infiniband/sw/rxe/rxe_comp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_comp.c
[ ... ]
> @@ -142,17 +154,52 @@ static inline enum comp_state get_wqe(struct rxe_qp *qp,
>  				      struct rxe_send_wqe **wqe_p)
>  {
>  	struct rxe_send_wqe *wqe;
> +	u32 state;
> +	unsigned int num_sge;
>  
>  	/* we come here whether or not we found a response packet to see if
>  	 * there are any posted WQEs
>  	 */
>  	wqe = queue_head(qp->sq.queue, QUEUE_TYPE_FROM_CLIENT);
> -	*wqe_p = wqe;
>  
>  	/* no WQE or requester has not started it yet */
> -	if (!wqe || wqe->state == wqe_state_posted)
> +	if (!wqe) {
> +		*wqe_p = NULL;
>  		return pkt ? COMPST_DONE : COMPST_EXIT;
> +	}
>  
> +	/* Pairs with smp_store_release() in rxe_req_writeback_wqe() */
> +	state = smp_load_acquire(&wqe->state);
> +	if (state == wqe_state_posted) {
> +		*wqe_p = wqe;
> +		return pkt ? COMPST_DONE : COMPST_EXIT;
> +	}
> +
> +	/* Reuse existing local copy if still processing same WQE
> +	 * with unchanged state (preserves DMA progress for multi-packet ops)
> +	 */
> +	if (qp->comp.comp_wqe_valid && qp->comp.shared_wqe == wqe &&
> +	    qp->comp.comp_wqe.wqe.state == state) {

[Severity: Critical]
If a malicious userspace application alters wqe->state in the shared ring
buffer mid-operation to a different but non-terminating value, will this
equality check fail?

If it fails, the completer will fall through and recopy the entire WQE from
shared memory into the kernel buffer qp->comp.comp_wqe.

Will this recopy overwrite the kernel's valid DMA progress (resid,
sge_offset, cur_sge) with the newly crafted dma state and lengths from
userspace, defeating the protection?

[Severity: High]
When a Queue Pair (QP) is transitioned to the RESET state via ib_modify_qp(),
rxe_qp_reset() is called. Does rxe_qp_reset() clear the newly introduced
qp->comp.comp_wqe_valid and qp->req.send_wqe_valid flags?

If these flags are not cleared, and userspace posts a new WQE after restart,
it is placed at queue index 0. The memory address wqe would evaluate to the
exact same pointer as the pre-reset operation.

Would this cause the completer to unexpectedly reuse the stale kernel-private
WQE, ignoring the newly posted operation's parameters?

> +		wqe = &qp->comp.comp_wqe.wqe;
> +		*wqe_p = wqe;
> +		goto check_state;
> +	}
> +
> +	/* Copy shared WQE to kernel-private buffer */
> +	num_sge = wqe->dma.num_sge;
> +	if (unlikely(num_sge > RXE_MAX_SGE))
> +		num_sge = RXE_MAX_SGE;
> +
> +	qp->comp.shared_wqe = wqe;
> +	memcpy(&qp->comp.comp_wqe.wqe, wqe,
> +	       sizeof(*wqe) + num_sge * sizeof(struct rxe_sge));

[Severity: High]
Since wqe->dma.num_sge is read from an mmap'd shared memory queue into num_sge
without using READ_ONCE(), is the C compiler permitted to optimize the code
by reloading the value from memory during the memcpy size calculation?

If an attacker in userspace concurrently modifies wqe->dma.num_sge to a large
value after the bounds check, could the re-fetched value bypass the
RXE_MAX_SGE limit and cause memcpy to write out of bounds into the fixed-size
qp->comp.comp_wqe kernel buffer?

[Severity: High]
The code caps num_sge against the global maximum RXE_MAX_SGE (32) instead
of the queue's specific maximum qp->sq.max_sge.

If max_sge was configured to a small value (e.g., 1), the allocated queue
elements are much smaller. If a malicious element is near the end of the
vmalloc_user ring buffer, could the memcpy read past the allocated queue
memory?

Could this hit an unmapped guard page and cause a fatal kernel page fault,
or leak adjacent queue elements?

[Severity: High]
Because memcpy uses unordered loads to copy the WQE from shared memory, could it
experience a torn read where it captures a new wqe->state (wqe_state_error)
but an old wqe->status (IB_WC_SUCCESS)?

> +	qp->comp.comp_wqe_valid = true;
> +	qp->comp.comp_wqe.wqe.dma.num_sge = num_sge;
> +
> +	wqe = &qp->comp.comp_wqe.wqe;
> +	*wqe_p = wqe;
> +
> +check_state:
>  	/* WQE does not require an ack */
>  	if (wqe->state == wqe_state_done)
>  		return COMPST_COMP_WQE;

If get_wqe() later checks the local copy's state directly:

check_state:
	/* WQE caused an error */
	if (wqe->state == wqe_state_error)
		return COMPST_ERROR;

Will the state machine transition to COMPST_ERROR but process the completion
as IB_WC_SUCCESS because the torn read missed the status update?

Does the local variable wqe need to be updated with the cleanly loaded state
variable from earlier, or should the status and state be read more carefully?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007212737.1989004-1-tristmd@gmail.com?part=2