Re: [PATCH v9 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 2 potential issue(s) to consider:
- [Medium] RDMA/rxe: uninitialized skb pad leak for short atomic write in req_next_wqe()
- [Low] RDMA/rxe: build failure in rxe_requester() due to undeclared comp_wqe_valid
--

commit af65ad7c40a27fdf3681a52375dd72c8fe8b0691
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 shared send queue into
a kernel-private buffer in req_next_wqe() before processing, validates the WR
opcode and atomic write residual length, and writes updated WQE fields back
to the shared queue.

> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
> index 4f353abd76f55..ff1ec143d41f1 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.
> +	 * Clamp oversized resid to prevent OOB read in finish_packet().
> +	 */
> +	if (unlikely((wqe->mask & WR_ATOMIC_WRITE_MASK) &&
> +		     wqe->dma.resid > sizeof(u64)))
> +		wqe->dma.resid = sizeof(u64);
>  	return wqe;
>  }

[Severity: Medium]
Can an undersized wqe->dma.resid on an IB_WR_ATOMIC_WRITE WQE leak
uninitialized skb padding bytes onto the wire?

In req_next_wqe(), wqe->dma.resid is only clamped when it exceeds
sizeof(u64). If a user RC QP sets wqe->dma.resid in the mmap'd send queue
to a smaller value that is not a multiple of 4 (for example, 1..3 or 5..7),
rxe_requester() sets payload = wqe->dma.resid and init_req_packet() allocates
an skb with pad = (-payload) & 0x3 trailing bytes.

In finish_packet(), bth_pad(pkt) trailing bytes are only zeroed under
RXE_WRITE_OR_SEND_MASK, while RXE_ATOMIC_WRITE_MASK only copies payload
bytes:

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

When rxe_xmit_packet() computes the ICRC and transmits the packet, those 1 to
3 uninitialized pad bytes from alloc_skb() are included in the packet. Should
req_next_wqe() check wqe->dma.resid != sizeof(u64) instead of only
wqe->dma.resid > sizeof(u64)?

[ ... ]
> @@ -702,6 +758,8 @@ 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;
> +		qp->comp.comp_wqe_valid = false;
>  	}
>  
>  	wqe = req_next_wqe(qp);

[Severity: Low]
Does building the tree at this commit fail because comp_wqe_valid is not yet
a member of struct rxe_comp_info?

In rxe_requester(), the retry block sets qp->comp.comp_wqe_valid = false,
but struct rxe_comp_info in drivers/infiniband/sw/rxe/rxe_verbs.h does not
define comp_wqe_valid until the next patch in the series ("RDMA/rxe: copy
send WQE to kernel buffer in completer path"):

struct rxe_comp_info {
	u32			psn;
	int			opcode;
	int			timeout;
	int			timeout_retry;
	int			started_retry;
	u32			retry_cnt;
	u32			rnr_retry;
};

Should this assignment (and the corresponding note in the commit message) be
moved to the second patch so that each commit in the series compiles cleanly
during bisection?

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