Re: [PATCH 12/15] yunsilicon: add RDMA QP verbs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 14 potential issue(s) to consider:
- [Critical] rdma: integer overflow on queue sizes leads to OOB DMA
- [High] rdma: uninitialized stack memory leaked to userspace in create_user_qp
- [High] rdma: sleeping allocation with spinlocks held in xsc_ib_err_state_qp
- [High] rdma: UAF and HW resource leak on allocation failure in destroy_qp_common
- [High] rdma: UAF and HW resource leak on qp_node allocation failure in create_qp_common
- [High] rdma: race condition adding QP node to SRQ list in create_qp_common
- [High] rdma: double byteswap and endianness corruption of qp_access_flags
- [High] infiniband/xsc: hardware DMA into stale memory due to uninitialized SGEs in xsc_ib_post_recv
- [High] xsc: ABI breakage and OOB read in create_qp due to size_t and blind copy
- [High] yunsilicon: dead code and missing error return in xsc_check_max_qp_depth
- [Medium] rdma: memory leak of mailbox in on error paths in create_qp_common
- [Medium] rdma: uninitialized stack memory copied to hardware context in xsc_set_path
- [Medium] rdma: undefined behavior due to negative shift in create_kernel_qp
- [Medium] rdma: passing struct size to sqd_event parameter in destroy_qp_common
--

commit 624855b048b853b9911aa55007116f0a1e95d930
Author: Xin Tian <tianx@yunsilicon.com>
Subject: yunsilicon: add RDMA QP verbs

This commit adds the queue pair control path: create, destroy, modify and
query, for both kernel and user queue pairs, including the address path setup
for RoCE.

diff --git a/drivers/infiniband/hw/xsc/qp.c b/drivers/infiniband/hw/xsc/qp.c
index 28c8a1246dfae..6a83adc9c833f 100644
--- a/drivers/infiniband/hw/xsc/qp.c
+++ b/drivers/infiniband/hw/xsc/qp.c
@@ -101,11 +101,11 @@ static int calc_sq_size(struct xsc_ib_dev *dev, struct ib_qp_init_attr *attr,
>  	qp->sq.wqe_cnt = roundup_pow_of_two(attr->cap.max_send_wr);
>  
>  	check_res = xsc_check_max_qp_depth(dev->xdev, &qp->sq.wqe_cnt, dev->xdev->caps.max_wqes);
>  	if (check_res) {
>  		ibdev_err(&dev->ib_dev, "max_send_wr:%d exceed max sq depth\n",
>  			  attr->cap.max_send_wr);
>  		return -EINVAL;
>  	}

[Severity: High]
Does this error path ever execute?

The helper xsc_check_max_qp_depth() unconditionally returns false while
silently clamping the queue depth instead of indicating an error. This hides
invalid user requests. 

Does clamping the queue depth break the power-of-two property if max_wqes is
not a power of two? This could break the wrapping logic in the post_send and
post_recv paths, potentially leading to memory corruption.

[ ... ]

>  	err = ib_copy_from_udata(&ucmd, udata, sizeof(ucmd));
>  	if (err) {
>  		ibdev_err(&dev->ib_dev, "failed to copy from udata, err=%d\n", err);
>  		return err;
>  	}

[Severity: High]
Is there a risk of an out-of-bounds memory read here?

This code reads sizeof(ucmd) bytes from userspace without verifying if
udata->inlen is large enough. If a 32-bit application or an older tool
passes a smaller struct, it might trigger an -EFAULT by crossing an unmapped
page boundary, or read uninitialized memory into fields like dmabuf_sz.

Also, does using size_t for dmabuf_sz in the xsc_ib_create_qp UAPI structure
break layout compatibility between 32-bit userspace and 64-bit kernels?

[ ... ]

>  	qp->sq.ds_cnt = ucmd.sq_wqe_count;
>  	qp->sq.wqe_cnt = ucmd.sq_wqe_count;
>  	qp->sq.wqe_shift = XSC_BASE_WQE_SHIFT;
>  	qp->rq.ds_cnt = ucmd.rq_wqe_count;
>  	qp->rq.wqe_cnt = ucmd.rq_wqe_count;
>  	qp->rq.wqe_shift = XSC_BASE_WQE_SHIFT;
>  
>  	qp->buf_size = (qp->sq.wqe_cnt << qp->sq.wqe_shift) + (qp->rq.wqe_cnt << qp->rq.wqe_shift);

[Severity: Critical]
Can this lead to an integer overflow and out-of-bounds DMA?

Unvalidated queue counts from userspace can overflow a 32-bit signed integer
when calculating qp->buf_size, resulting in a small allocation. However, the
hardware will receive the huge queue counts and expect a large ring buffer.
This could potentially allow malicious userspace to trigger out-of-bounds DMA
across host physical memory.

[ ... ]

>  	*inlen = sizeof(**in) + sizeof(*((*in)->req.pas)) * hw_npages;
>  	*in = xsc_vzalloc(*inlen);
>  	if (!*in) {
>  		err = -ENOMEM;
>  		goto err_umem;
>  	}
>  	xsc_ib_populate_pas(dev, qp->umem, page_shift, (*in)->req.pas, hw_npages, true);
>  	(*in)->req.pa_num = cpu_to_be16(hw_npages);
>  
>  	err = ib_copy_to_udata(udata, resp, sizeof(*resp));
>  	if (err) {

[Severity: High]
Does this leak uninitialized stack memory to userspace?

The resp structure is declared on the stack in create_qp_common() without
initialization, passed down to this function, and immediately copied to
userspace before any of its fields are populated.

[ ... ]

>  	sq_size = calc_sq_size(dev, init_attr, qp);
>  	if (sq_size < 0) {
>  		err = -EINVAL;
>  		ibdev_err(&dev->ib_dev, "err %d\n", err);
>  		return err;
>  	}
>  
>  	qp->rq.ds_cnt = qp->rq.wqe_cnt << (qp->rq.wqe_shift - XSC_BASE_WQE_SHIFT);

[Severity: Medium]
Could this evaluate to a negative shift?

If a QP is created without a receive queue, set_rq_size() sets wqe_shift to
0. Since XSC_BASE_WQE_SHIFT is non-zero, this evaluates to a bitwise left
shift by a negative amount, which is undefined behavior.

[ ... ]

>  err_wrid:
>  	kfree(qp->sq.prev_ce_wqe);
>  	kfree(qp->sq.need_flush);
>  	kfree(qp->sq.wr_opcode);
>  	kfree(qp->sq.wqe_head);
>  	kfree(qp->sq.w_list);
>  	kfree(qp->sq.wrid);
>  	kfree(qp->sq.wr_data);
>  	kfree(qp->rq.wrid);
>  
>  err_buf:
>  	xsc_buf_free(dev->xdev, &qp->buf);
>  	return err;

[Severity: Medium]
Does this error path leak the mailbox *in structure?

The memory is allocated with xsc_vzalloc() earlier in the function but does
not appear to be freed before returning here.

[ ... ]

>  	if (pd->uobject) {
>  		err = create_user_qp(dev, pd, qp, udata, &in, &resp, &inlen);
>  		if (err)
>  			ibdev_err(&dev->ib_dev, "failed to create user qp, err = %d\n", err);
>  	} else {
>  		err = create_kernel_qp(dev, init_attr, qp, &in, &inlen);
>  		if (err)
>  			ibdev_err(&dev->ib_dev, "failed to create kernel qp, err = %d\n", err);
>  		else
>  			qp->pa_lkey = to_xpd(pd)->pa_lkey;
>  	}
>  
>  	if (err)
>  		return err;

[Severity: Medium]
If create_user_qp() or create_kernel_qp() fails, does returning err here
directly bypass the err_create label where the mailbox structure is supposed
to be freed?

[ ... ]

>  	if (!pd->uobject && init_attr->srq) {
>  		qp_node = kvzalloc_obj(*qp_node, GFP_KERNEL);
>  		if (!qp_node)
>  			goto err_create;

[Severity: High]
If kvzalloc_obj() fails here, err is 0, so the function will jump to
err_create but return success to the caller.

Does this result in the hardware QP remaining active while the software
buffers are freed? The QP might be left dangling in the CQ lists, which can
lead to a use-after-free and a resource leak.

>  		qp_node->ibqp = &qp->ibqp;
>  		qp_node->qp_id = qp->xqp.qpn;
>  		ibdev_dbg(&dev->ib_dev, "qpn:%u add into srq_qp_list\n", qp_node->qp_id);
>  		list_add_tail(&qp_node->entry, &to_xsrq(init_attr->srq)->xsrq.srq_qp_list);

[Severity: High]
Is there a race condition here?

The QP node is being added to the SRQ tracking list without acquiring the
xsrq->lock. Concurrent QP creations or destructions on the same SRQ might
corrupt the linked list.

[ ... ]

>  static void destroy_qp_common(struct xsc_ib_dev *dev, struct xsc_ib_qp *qp)
>  {
>  	struct xsc_ib_cq *send_cq = NULL;
>  	struct xsc_ib_cq *recv_cq = NULL;
>  	struct xsc_modify_qp_mbox_in *in;
>  	int err;
>  
>  	in = kzalloc_obj(*in, GFP_KERNEL);
>  	if (!in)
>  		return;

[Severity: High]
Does an early return here abort the cleanup and cause a use-after-free?

If allocation fails, the QP destruction process ends prematurely, meaning the
hardware QP is not destroyed and it remains on the CQ lists. Since the caller
returns 0 unconditionally, the RDMA subsystem will free the struct ib_qp,
leaving dangling pointers behind.

>  
>  	if (qp->xqp.qp_type_internal == XSC_QUEUE_TYPE_RAW ||
>  	    qp->xqp.qp_type_internal == XSC_QUEUE_TYPE_RAW_TSO ||
>  	    qp->xqp.qp_type_internal == XSC_QUEUE_TYPE_RAW_TX ||
>  	    qp->state != IB_QPS_RESET)
>  		if (xsc_core_qp_modify(dev->xdev, to_xsc_state(qp->state),
>  				       XSC_QP_STATE_RST, in, sizeof(*in), &qp->xqp))

[Severity: Medium]
Is sizeof(*in) the correct argument for the sqd_event parameter in
xsc_core_qp_modify()?

Passing a structure size to a parameter intended as an event flag or boolean
might be a type mismatch that corrupts hardware behavior.

[ ... ]

>  static int __xsc_ib_modify_qp(struct ib_qp *ibqp,
>  			      const struct ib_qp_attr *attr, int attr_mask,
>  			      enum ib_qp_state cur_state, enum ib_qp_state new_state,
>  			      const struct xsc_ib_modify_qp *ucmd,
>  			      struct ib_udata *udata)
>  {
>  	struct xsc_ib_dev *dev = to_xdev(ibqp->device);
>  	struct xsc_ib_qp *qp = to_xqp(ibqp);
>  	struct xsc_ib_cq *send_cq, *recv_cq;
>  	struct xsc_qp_context *context;
>  	struct xsc_modify_qp_mbox_in *in;
>  	struct xsc_qp_path path;

[ ... ]

>  	if (attr_mask & IB_QP_AV) {
>  		err = xsc_set_path(dev, &attr->ah_attr, &path,
>  				   attr_mask & IB_QP_PORT ? attr->port_num : qp->port,
>  				   attr_mask, 0, attr, qp);
>  		if (err)
>  			goto out;
>  
>  		context->src_udp_port = path.sport;
>  		context->dscp = path.ecn_dscp;
>  		context->hop_limit = path.hop_limit;
>  		context->ip_type = (path.af_type == AF_INET ? 0 : 1);
>  		context->ip_type = cpu_to_be16(context->ip_type);
>  		memcpy(context->dip, path.dip, sizeof(context->dip));
>  		memcpy(context->sip, path.sip, sizeof(context->sip));

[Severity: Medium]
Could this copy uninitialized stack memory into the hardware context?

The path structure is declared on the stack and passed to xsc_set_path().
For IPv4, only 4 bytes of sip and dip are populated. Copying the full
IPv6-sized array from path to context might leak uninitialized stack data to
the device firmware.

[ ... ]

>  	if (udata && udata->inlen && attr_mask & IB_QP_STATE && new_state == IB_QPS_RTR) {
>  		if (!ucmd->profile &&
>  		    xdev->rdma_proto_mode == RDMA_PROTO_VEROCE) {
>  			context->swith_qp_type = 1;
>  			context->qp_access_flags |= QP_ACCESS_MODIFY_QP_TYPE;
>  			context->qp_access_flags = cpu_to_be32(context->qp_access_flags);

[Severity: High]
Does this corrupt the endianness of qp_access_flags?

It looks like context->qp_access_flags was already converted to big-endian
earlier via cpu_to_be32() in xsc_set_qp_access_flag(). Applying a bitwise OR
directly to the big-endian variable and then byteswapping it a second time
will corrupt the flag bits on little-endian architectures.

[ ... ]

>  	if (!err && (attr_mask & IB_QP_STATE) && !udata) {
>  		get_cqs(qp->xqp.qp_type, qp->send_cq, qp->recv_cq, &send_cq, &recv_cq);
>  		xsc_ib_lock_cqs(send_cq, recv_cq);
>  		err = xsc_ib_err_state_qp(dev, qp, to_xsc_state(cur_state),
>  					  to_xsc_state(new_state));
>  		xsc_ib_unlock_cqs(send_cq, recv_cq);

[Severity: High]
Is there a sleeping allocation happening while holding spinlocks?

xsc_ib_lock_cqs() acquires spinlocks and disables interrupts. Inside
xsc_ib_err_state_qp(), memory is allocated using kzalloc_obj(..., GFP_KERNEL),
which can sleep and trigger a panic in an atomic context.

[ ... ]

>  		for (i = 0; i < wr->num_sge; ++i) {
>  			if (unlikely(!wr->sg_list[i].length))
>  				continue;
>  			data_seg = get_seg_wqe(recv_head, i);
>  			data_seg->in_line = 0;
>  			WR_LE_64(data_seg->va, wr->sg_list[i].addr);
>  			WR_LE_32(data_seg->mkey, wr->sg_list[i].lkey);
>  			if (is_qp1(qp->xqp.qp_type))
>  				WR_LE_32(data_seg->seg_len, xdev->caps.rx_pkt_len_max);
>  			else
>  				WR_LE_32(data_seg->seg_len, wr->sg_list[i].length);
>  		}

[Severity: High]
Can the hardware scatter incoming network data into stale memory addresses?

When this loop skips 0-length SGEs or when wr->num_sge is less than the
maximum permitted, it leaves trailing data segments in the Receive WQE
uninitialized. Since the receive queue is a ring buffer, these slots might
contain stale SGE data from previous work requests.

Without a control segment specifying the valid SGE count, the hardware could
process these stale entries and write into memory that has already been freed
or reallocated.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012114352.2471317-1-tianx@yunsilicon.com?part=12