Re: [PATCH v8 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 1 potential issue(s) to consider:
- [Critical] RDMA/rxe: broken atomic write resid check in req_next_wqe() allows OOB read
--

commit c2bf5201e4676c1f9f9acad4282470fc916075a3
Author: Tristan Madani <tristmd@gmail.com>

RDMA/rxe: copy send WQE to kernel buffer before processing

This commit copies send WQEs from the userspace-mapped send queue into a
kernel-private buffer in req_next_wqe() before processing, writing updated
fields back to the shared queue via rxe_req_writeback_wqe().

> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
> index 4f353abd76f55..45e0e97d17926 100644
> --- a/drivers/infiniband/sw/rxe/rxe_req.c
> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
[ ... ]
> @@ -193,7 +213,35 @@ static struct rxe_send_wqe *req_next_wqe(struct rxe_qp *qp)
>  	}
>  	spin_unlock_irqrestore(&qp->state_lock, flags);
>  
> -	wqe->mask = wr_opcode_mask(wqe->wr.opcode, qp);
> +	/* 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 size matches the queue element data area.
> +	 */
> +	memcpy(&qp->req.send_wqe, wqe, sizeof(*wqe) +
> +	       max_t(int, qp->sq.max_sge * sizeof(struct ib_sge),
> +		    qp->sq.max_inline));
> +	qp->req.shared_wqe = wqe;
> +	qp->req.send_wqe_valid = true;
> +
> +	wqe = &qp->req.send_wqe.wqe;
> +
> +	/* Prevent OOB in wr_opcode_mask() for corrupted opcodes.
> +	 * Mask 0 causes next_opcode() to fail, moving QP to error state.
> +	 */
> +	if (unlikely(wqe->wr.opcode > IB_WR_REG_MR))
> +		wqe->mask = 0;
> +	else
> +		wqe->mask = wr_opcode_mask(wqe->wr.opcode, qp);
> +
> +	/* Atomic writes carry exactly sizeof(u64) payload.
> +	 * Reject oversized resid to prevent OOB read in finish_packet().
> +	 */
> +	if (unlikely((wqe->mask & RXE_ATOMIC_WRITE_MASK) &&
> +		     wqe->dma.resid > sizeof(u64)))
> +		wqe->mask = 0;
>  	return wqe;
>  }

[Severity: Critical]
Does this atomic write resid check in req_next_wqe() in
drivers/infiniband/sw/rxe/rxe_req.c actually reject an IB_WR_ATOMIC_WRITE WQE
posted with wqe->dma.resid > sizeof(u64), or can finish_packet() still read
out of bounds past qp->req.send_wqe?

First, wqe->mask is populated by wr_opcode_mask(), which returns flags from
enum rxe_wr_mask (bits 0..7, where WR_ATOMIC_WRITE_MASK is BIT(7)). Testing
wqe->mask against RXE_ATOMIC_WRITE_MASK from enum rxe_hdr_mask (BIT(27))
always evaluates to false.

Second, even if wqe->mask is set to 0, rxe_requester() and next_opcode() do
not reject a WQE based on wqe->mask == 0 when wqe->wr.opcode is
IB_WR_ATOMIC_WRITE. In rxe_requester():

	if (wqe->mask & WR_LOCAL_OP_MASK) {
		...
	}
	...
	opcode = next_opcode(qp, wqe, wqe->wr.opcode);
	if (unlikely(opcode < 0)) {
		wqe->status = IB_WC_LOC_QP_OP_ERR;
		goto err;
	}

	mask = rxe_opcode[opcode].mask;

next_opcode() -> next_opcode_rc() switches directly on wqe->wr.opcode without
checking wqe->mask:

	case IB_WR_ATOMIC_WRITE:
		return IB_OPCODE_RC_ATOMIC_WRITE;

rxe_requester() then sets mask and pkt.mask from rxe_opcode[opcode].mask
(which has RXE_ATOMIC_WRITE_MASK set), clamps payload to mtu (up to 4096
bytes), and calls finish_packet():

	if (pkt->mask & RXE_ATOMIC_WRITE_MASK) {
		memcpy(payload_addr(pkt), wqe->dma.atomic_wr, payload);
		wqe->dma.resid -= payload;
	}

Would this read up to 4096 bytes starting at
qp->req.send_wqe.wqe.dma.atomic_wr, reading past the 512-byte
qp->req.send_wqe.sge array into adjacent memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030165044.2799619-1-tristan@talencesecurity.com?part=1