Re: [PATCH] RDMA/rxe: validate num_sge/cur_sge before indexing wqe->dma.sge[]

[Ibrahim Hashimov]

Hi Zhu Yanjun, thanks for the review -- good catch, you're right.

> From this function,
> static int rxe_init_sq(struct rxe_qp *qp, struct ibv_qp_init_attr *init_attr)
> ...
>      qp->sq.max_sge = init_attr->cap.max_send_sge;
> ...
> qp->sq.max_sge is also from the user space application. It is possible
> that qp->sq.max_sge is 0.
> Then this makes rxe_requester will return error and exit.

Correct: with max_sge == 0 my "cur_sge >= qp->sq.max_sge" check rejects
a legitimate zero-SGE WQE (cur_sge == 0), so such a QP could never post.
The claim in my commit message that zero-payload WQEs "remain valid" was
simply wrong for the max_sge == 0 case -- thanks for catching it.

One subtlety while fixing it: gating the check on num_sge alone is not
enough. The requester's payload is wqe->dma.resid, which is independent
of num_sge, so a crafted WQE with num_sge == 0, a large resid and an
out-of-range cur_sge would still reach copy_data() and dereference
dma->sge[cur_sge]. What actually makes a zero-SGE WQE safe is that
copy_data() returns early on length == 0, before it ever touches
dma->sge[]. So in v2 I gate the cur_sge bound on wqe->dma.resid (i.e.
"is there payload that will make copy_data() index dma->sge[]") rather
than on num_sge:

	if (unlikely(wqe->dma.num_sge > qp->sq.max_sge ||
		     (wqe->dma.resid &&
		      wqe->dma.cur_sge >= qp->sq.max_sge))) {

This lets a zero-payload WQE through (including on a max_sge == 0 QP),
still rejects the oversized-num_sge and out-of-range-cur_sge cases, and
keeps multi-packet sends valid (cur_sge advances within [0, num_sge)
while resid > 0, and num_sge <= max_sge keeps that in the array).

Re-verified on the same v6.19 KASAN (CONFIG_KASAN_VMALLOC=y) stand: the
original reproducer -- which posts a user-QP send WQE with an
out-of-range cur_sge -- reaches the exploit path and the QP now
completes cleanly with no "vmalloc-out-of-bounds in copy_data" report,
whereas the pre-fix kernel trips it.

One thing worth flagging for completeness: this gate (like the sibling
validate_send_wr()/get_srq_wqe() checks) validates the WQE in place in
the mmap'd ring, so it narrows rather than fully closes the underlying
race -- a userspace thread can still mutate cur_sge/resid after the
check and before copy_data() re-reads them. The retransmit path
(advance_dma_data() via req_retry()) also indexes dma->sge[cur_sge]
ahead of this check. Both are pre-existing and out of scope for this
one-liner, but I'm happy to look at them separately if you'd like.

I'll send this as [PATCH v2].

Thanks,
Ibrahim