Re: [PATCH v5 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 4 potential issue(s) to consider:
- [Critical] RDMA/rxe: Out-of-bounds array access due to unvalidated dma.cur_sge in copied WQE
- [High] RDMA/rxe: Incomplete TOCTOU fix leaks shared WQE pointer when wqe_state_posted
- [High] RDMA/rxe: Broken RDMA READ retry logic due to omitted dma.resid writeback
- [High] rdma/rxe: FORTIFY_SOURCE field-spanning write warning in get_wqe()
--

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

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

This commit attempts to fix TOCTOU races by copying send WQEs from the
shared queue to a kernel-private buffer in the completer path. It
introduces local copies of the WQE to preserve DMA progress and updates
writeback logic before advancing the consumer index.

> diff --git a/drivers/infiniband/sw/rxe/rxe_comp.c b/drivers/infiniband/sw/rxe/rxe_comp.c
> index 1390e861bd1d7..5d8b692114a33 100644
> --- a/drivers/infiniband/sw/rxe/rxe_comp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_comp.c
[ ... ]
> +/* Write back completer WQE fields to shared memory */
> +static void rxe_comp_writeback_wqe(struct rxe_qp *qp)
> +{
> +	struct rxe_send_wqe *shared = qp->comp.shared_wqe;
> +	struct rxe_send_wqe *local = &qp->comp.comp_wqe.wqe;
> +
> +	if (!qp->comp.comp_wqe_valid || !shared)
> +		return;
> +
> +	WRITE_ONCE(shared->status, local->status);
> +	WRITE_ONCE(shared->has_rd_atomic, local->has_rd_atomic);

[Severity: High]
Does this code fail to write back dma.resid and dma.sge_offset?

If a timeout occurs, the requester's retry logic (req_retry()) reads the
unmodified dma.resid from the shared WQE (or its own unmodified private
copy) to calculate the retry offset.

This causes it to retry from the beginning (offset 0) but with an advanced
PSN, resulting in the completer writing redundant remote data into advanced
local offsets.

> +}
> +
>  static inline enum comp_state get_wqe(struct rxe_qp *qp,
>  				      struct rxe_pkt_info *pkt,
>  				      struct rxe_send_wqe **wqe_p)
>  {
>  	struct rxe_send_wqe *wqe;
> +	u32 state;
>  
>  	/* 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);
>  
>  	/* 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;

[Severity: High]
Does this code assign the userspace-mapped shared memory pointer to *wqe_p
instead of forcing a local copy or returning NULL?

If the completer drops into COMPST_ERROR_RETRY (e.g., following a timeout),
it evaluates wqe->state == wqe_state_posted using this shared pointer.

Userspace can concurrently modify wqe->state to bypass this check, causing
the completer to erroneously process un-started WQEs.

> +		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) {
> +		wqe = &qp->comp.comp_wqe.wqe;
> +		*wqe_p = wqe;
> +		goto check_state;
> +	}
> +
> +	/* Copy shared WQE to kernel-private buffer. Use max_inline
> +	 * as copy size since it covers both SGEs and inline data,
> +	 * which share the flex array.
> +	 */
> +	qp->comp.shared_wqe = wqe;
> +	memcpy(&qp->comp.comp_wqe.wqe, wqe,
> +	       sizeof(*wqe) + qp->sq.max_inline);

[Severity: High]
Can this memcpy trigger a FORTIFY_SOURCE field-spanning write warning?

This memcpy targets the wqe sub-object but copies a length that spans into
the adjacent sge field.

The compiler's bounds check restricts the write destination field to exactly
sizeof(*wqe), so this triggers a fortify warning (and panic if panic_on_warn
is enabled) when max_inline > 0.

> +	qp->comp.comp_wqe_valid = true;
> +	if (qp->comp.comp_wqe.wqe.dma.num_sge > qp->sq.max_sge)
> +		qp->comp.comp_wqe.wqe.dma.num_sge = qp->sq.max_sge;

[Severity: Critical]
Does this code neglect to validate cur_sge after the copy from userspace?

A malicious userspace application can set cur_sge to an arbitrarily large
value.

During RDMA read or Atomic response processing, copy_data() indexes the
dma->sge array using this unvalidated cur_sge, accessing memory outside
the comp_wqe buffer.

> +
> +	wqe = &qp->comp.comp_wqe.wqe;
> +	*wqe_p = wqe;
>  
> +check_state:

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