Re: [PATCH v4 1/2] RDMA/rxe: copy send WQE to kernel buffer before processing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] rdma: remote kernel heap information leak via unvalidated inline data length
- [High] mm: fortify_source field-spanning write warning in req_next_wqe memcpy
- [High] mm: truncated memcpy of WQE flex array corrupts inline data
- [Medium] locking: missing memory barriers when flushing WQEs leads to stale completion status
--

commit 05fdd210d6581da6d266954d4f637a96e6640c15
Author: Tristan Madani <tristan@talencesecurity.com>
Subject: RDMA/rxe: copy send WQE to kernel buffer before processing

This commit copies the requester WQE from a user-space mapped buffer to a
kernel-private buffer before processing to prevent Time-Of-Check to
Time-Of-Use (TOCTOU) modifications. It also introduces a local copy reuse
mechanism and targeted writebacks to the shared queue to prevent tearing.

> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
> index 53f7c1e7fe55b..df0fa455cd9c1 100644
> --- a/drivers/infiniband/sw/rxe/rxe_req.c
> +++ b/drivers/infiniband/sw/rxe/rxe_req.c

[ ... ]

> @@ -193,6 +213,20 @@ static struct rxe_send_wqe *req_next_wqe(struct rxe_qp *qp)
>  	}
>  	spin_unlock_irqrestore(&qp->state_lock, flags);
>  
> +	/* Reuse the existing kernel-private copy if still valid */
> +	if (qp->req.send_wqe_valid && qp->req.shared_wqe == wqe)
> +		return &qp->req.send_wqe.wqe;
> +
> +	/* Copy WQE from userspace-mapped shared queue to kernel-private
> +	 * buffer. Copy the full element (max_sge SGEs) so that inline
> +	 * data, which shares the flex array with SGEs, is always captured.
> +	 */
> +	memcpy(&qp->req.send_wqe.wqe, wqe,
> +	       sizeof(*wqe) + qp->sq.max_sge * sizeof(struct rxe_sge));

[Severity: High]
Does this memcpy trigger a fortify_source field-spanning write warning?

The destination pointer &qp->req.send_wqe.wqe explicitly points to the wqe
member of the anonymous send_wqe struct, which has a size of 64 bytes. If
max_sge > 0, the copy length exceeds 64 bytes.

When compiled with CONFIG_FORTIFY_SOURCE=y, __builtin_object_size(p, 1) bounds
the destination to the exact size of the wqe member because it is embedded
inside an outer struct before the sge array. This detects the out-of-bounds
write and triggers a deterministic run-time field-spanning write warning,
potentially crashing the kernel if panic_on_warn is enabled.

[Severity: High]
Can this size calculation truncate the inline data if max_inline_data is not
a multiple of 16?

During QP initialization, qp->sq.max_sge is computed via integer division
(wqe_size / sizeof(struct ib_sge)). If max_inline_data is configured to a
value that is not a multiple of 16 (e.g., 20 bytes), wqe_size evaluates to
20, but max_sge evaluates to 1.

The memcpy size would then evaluate to 64 + 1 * 16 = 80 bytes, whereas the
inline data spans 20 bytes up to offset 84. This truncates the copy, leaving
the last 4 bytes of inline data uncopied. When finish_packet() reads the
inline data from the kernel buffer, will it transmit uninitialized or stale
kernel memory instead of the user's provided payload?

> +	qp->req.shared_wqe = wqe;
> +	qp->req.send_wqe_valid = true;
> +
> +	wqe = &qp->req.send_wqe.wqe;
>  	wqe->mask = wr_opcode_mask(wqe->wr.opcode, qp);
>  	return wqe;
>  }

[ ... ]

> @@ -666,6 +706,7 @@ int rxe_requester(struct rxe_qp *qp)
>  		wqe = __req_next_wqe(qp);
>  		spin_unlock_irqrestore(&qp->state_lock, flags);
>  		if (wqe) {
> +			qp->req.send_wqe_valid = false;
>  			wqe->status = IB_WC_WR_FLUSH_ERR;
>  			goto err;

[ ... ]

> @@ -695,6 +737,7 @@ int rxe_requester(struct rxe_qp *qp)
>  	if (unlikely(qp->req.need_retry && !qp->req.wait_for_rnr_timer)) {
>  		req_retry(qp);
>  		qp->req.need_retry = 0;
> +		qp->req.send_wqe_valid = false;
>  	}
>  
>  	wqe = req_next_wqe(qp);

[Severity: Critical]
Does this introduce a remote kernel heap information leak for inline packets?

By moving the WQE buffer to the kernel heap, we now need to validate bounds
that were previously safe to read from shared memory. For inline data packets,
wqe->dma.resid (which stores the user-provided payload length) is only bounded
by qp->mtu (e.g. 4096). The validation checks in this function completely
ignore wqe->dma.resid and wqe->dma.sge_offset for inline sends.

Later in finish_packet(), the inline payload is copied into the network packet:

memcpy(payload_addr(pkt), &wqe->dma.inline_data[wqe->dma.sge_offset], payload);

Could a malicious user set sge_offset = 0 and length to 4096? The memcpy
would read past the end of the qp->req.send_wqe allocation and leak adjacent
kernel heap memory into the outgoing network packet.

>  	if (unlikely(!wqe))
>  		goto exit;

[ ... ]

> @@ -839,8 +884,10 @@ int rxe_requester(struct rxe_qp *qp)
>  	goto out;
>  err:
>  	/* update wqe_index for each wqe completion */
> -	qp->req.wqe_index = queue_next_index(qp->sq.queue, qp->req.wqe_index);
>  	wqe->state = wqe_state_error;
> +	rxe_req_writeback_wqe(qp);

[Severity: Medium]
Does this fast-flush path miss memory barriers when updating the shared WQE?

When handling IB_QPS_ERR, this function sets qp->req.send_wqe_valid = false,
assigns wqe->status = IB_WC_WR_FLUSH_ERR directly, and then jumps to err:,
where it sets wqe->state = wqe_state_error; and calls rxe_req_writeback_wqe(qp).

Because qp->req.send_wqe_valid is false, rxe_req_writeback_wqe() returns
immediately without issuing the intended smp_store_release() memory barrier.

Could the CPU or compiler reorder the writes to status and state, causing
the completer polling the shared queue to observe wqe_state_error but read a
stale or uninitialized status?

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