Re: [PATCH 11/15] yunsilicon: add RDMA CQ/SRQ/AH

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 13 potential issue(s) to consider:
- [Critical] core: hardware DMA buffer overflow in QP1 (MAD) receive path
- [High] locking: AB-BA deadlock between cq->lock and srq->lock
- [High] mm: sleeping in atomic context due to GFP_KERNEL allocations under spinlocks
- [High] mm: missing kcalloc NULL check in xsc_cache_srq_wr causes immediate panic
- [High] core: dangling tail pointer leads to Use-After-Free list corruption in SRQ cache
- [High] locking: unprotected list modification on cross-CQ err_state_qp_list
- [High] core: SRQ destruction UAF race with global ID reclamation
- [High] core: skipping zero-length SGEs leaves uninitialized hardware WQE gaps
- [High] core: uninitialized cqe_sz variable breaks error CQ cleaning for userspace CQs
- [High] rdma/xsc: memory leak and subsequent NULL dereference in xsc_post_cache_wrs error path
- [Medium] net: out-of-bounds read when parsing MAD headers from truncated packets
- [Medium] core: max_cqes truncation introduces non-power-of-two mask indexing vulnerability
- [Low] core: xsc_ib_query_ah drops cached RoCE destination MAC address
--

commit 362ed58c2fd14cd6742383cf2e788372cccd3288
Author: Xin Tian <tianx@yunsilicon.com>

yunsilicon: add RDMA CQ/SRQ/AH

Add the completion queues, shared receive queues and address handles: the
create/destroy/query/modify verbs and the CQ poll and completion
processing.

The QP receive helper and error-state tracking are included here
because the CQ and SRQ paths reference them.

> diff --git a/drivers/infiniband/hw/xsc/ah.c b/drivers/infiniband/hw/xsc/ah.c
> --- /dev/null
> +++ b/drivers/infiniband/hw/xsc/ah.c
[ ... ]
> @@ -112,23 +112,33 @@ int xsc_ib_query_ah(struct ib_ah *ibah, struct rdma_ah_attr *ah_attr)
>  {
>  	struct xsc_ib_ah *ah = to_xah(ibah);
>  	u32 tmp;
>  
>  	memset(ah_attr, 0, sizeof(*ah_attr));
>  	ah_attr->type = ibah->type;
[ ... ]
>  	rdma_ah_set_static_rate(ah_attr, ah->av.stat_rate_sl >> 4);
>  	if (ah_attr->type == RDMA_AH_ATTR_TYPE_ROCE) {
>  		rdma_ah_set_sl(ah_attr, (ah->av.stat_rate_sl >> 1) & 0x7);
>  	} else {

[Severity: Low]
Does this code intentionally omit restoring the dmac field for RoCE
connections? During xsc_ib_create_ah(), the destination MAC address is
saved into ah->av.rmac, but the query path appears to leave it zeroed out
here, which might cause the driver to report a zeroed MAC address.

> diff --git a/drivers/infiniband/hw/xsc/cq.c b/drivers/infiniband/hw/xsc/cq.c
> --- /dev/null
> +++ b/drivers/infiniband/hw/xsc/cq.c
[ ... ]
> @@ -201,15 +201,17 @@ static void xsc_handle_rdma_mad_resp_recv(struct xsc_ib_cq *cq,
>  	if (eth_type == ETH_P_IP) {
>  		pading_sz = sizeof(*grh) - sizeof(*ip4h);
>  		memmove((u8 *)(grh + 1) - sizeof(*ip4h), ip4h, sizeof(*ip4h));
>  		memset(grh, 0, pading_sz);
>  	} else {
>  		memmove(grh, ip6h, sizeof(*ip6h));
>  	}
>  	memmove(grh + 1, mad, sizeof(*mad));

[Severity: Medium]
Is it possible for memmove to read out of bounds here if an attacker or
misconfigured node sends a truncated packet? There does not seem to be a
check against the overall packet length (wc->byte_len) before copying
sizeof(*mad) bytes.

[ ... ]
> @@ -375,14 +375,16 @@ int xsc_ib_create_cq(struct ib_cq *ibcq,
>  	int irqn;
>  	int err;
>  	unsigned int eqn;
>  
>  	entries = roundup_pow_of_two(entries);
>  
>  	ibdev_dbg(&dev->ib_dev, "entries:%d, vector:%d, max_cqes:%d\n", entries, vector,
>  		  dev->xdev->caps.max_cqes);
>  
>  	if (entries > dev->xdev->caps.max_cqes)
>  		entries = dev->xdev->caps.max_cqes;

[Severity: Medium]
Could capping entries at max_cqes break the power-of-two invariant
established just above it? If the hardware reports a max_cqes value that
is not a power of two, the polling paths that use bitwise AND masking to
calculate ring buffer indices might experience discontinuous indices and read
out of bounds.

>  	cq = to_xcq(ibcq);
>  	cq->ibcq.cqe = entries;
[ ... ]
>  	cq->create_flags = attr->flags;
>  	cq->invalid = 0;
>  
>  	if (udata) {
>  		err = create_cq_user(dev, udata, NULL, cq, entries,
>  				     &cqb, &cqe_size, &index, &inlen);
>  		if (err)
>  			goto err_create;
>  	} else {
>  		cqe_size = is_support_cqe64(dev->xdev) ?
>  			sizeof(struct xsc_cqe64) : sizeof(struct xsc_cqe);
>  		err = create_cq_kernel(dev, cq, entries, cqe_size, &cqb, &index, &inlen);
>  		if (err)
>  			goto err_create;
>  	}
>  
>  	cq->cqe_size = cqe_size;

[Severity: High]
Does the userspace CQ creation path leave cq->xcq.cqe_sz uninitialized?
create_cq_kernel() sets it internally, but create_cq_user() does not appear
to. If it remains 0, the flush-cleanup routine __xsc_ib_cq_clean() might
fail to step through the buffer array correctly when overwriting matching
QP elements.

> diff --git a/drivers/infiniband/hw/xsc/qp.c b/drivers/infiniband/hw/xsc/qp.c
> --- /dev/null
> +++ b/drivers/infiniband/hw/xsc/qp.c
[ ... ]
> @@ -100,18 +100,20 @@ int xsc_ib_err_state_qp(struct xsc_ib_dev *dev, struct xsc_ib_qp *qp,
>  	if (cur_state != XSC_QP_STATE_ERR && state == XSC_QP_STATE_ERR &&
>  	    !qp->has_trig_cq_comp_handle) {
>  		if (!qp->ibqp.srq && qp->recv_cq) {
>  			err_rq_node = kzalloc_obj(*err_rq_node, GFP_KERNEL);

[Severity: High]
Can this allocation trigger a scheduling while atomic panic?
xsc_ib_err_state_qp() is called from xsc_poll_one() during CQ polling,
which holds cq->lock via spin_lock_irqsave. Calling kzalloc_obj with
GFP_KERNEL under a spinlock might sleep.

>  			if (!err_rq_node)
>  				return -ENOMEM;
>  			err_rq_node->qp_id = qp->xqp.qpn;
>  			err_rq_node->is_sq = false;
>  			recv_xcq = to_xcq(qp->recv_cq);
>  			list_add_tail(&err_rq_node->entry, &recv_xcq->err_state_qp_list);

[Severity: High]
Does this introduce unprotected list modifications? Since this is called
synchronously while holding only the lock for the CQ currently being
polled (e.g., the send CQ), modifying the receive CQ's err_state_qp_list
appears to happen locklessly. This might corrupt the double-linked list
if a concurrent polling thread traverses it.

[ ... ]
> @@ -131,10 +131,11 @@ static int xsc_wq_overflow(struct xsc_ib_wq *wq, int nreq, struct xsc_ib_cq *cq)
>  {
>  	unsigned int cur;
>  
>  	cur = wq->head - wq->tail;
>  	if (likely(cur + nreq < wq->max_post))
>  		return 0;
>  
>  	spin_lock(&cq->lock);

[Severity: High]
Could this lead to a circular locking deadlock between cq->lock and
srq->lock?

Path A acquires cq->lock first, then srq->lock:
xsc_ib_poll_cq() -> acquires cq->lock
   xsc_poll_one()
      xsc_free_one_cache_srq_wr() -> acquires srq->lock

Path B acquires srq->lock first, then cq->lock:
xsc_ib_post_srq_recv() -> acquires srq->lock
   xsc_post_cache_wrs()
      xsc_ib_post_recv()
         xsc_wq_overflow() -> acquires cq->lock

This might trigger a hard kernel deadlock when receive operations coincide
with polling.

[ ... ]
> @@ -186,13 +186,17 @@ int xsc_ib_post_recv(struct ib_qp *ibqp, const struct ib_recv_wr *wr,
>  		recv_head = get_recv_wqe(qp, idx);
>  
>  		for (i = 0; i < wr->num_sge; ++i) {
>  			if (unlikely(!wr->sg_list[i].length))
>  				continue;

[Severity: High]
Does skipping zero-length SGEs here leave uninitialized gaps in the
hardware WQE? By using continue, the data_seg at index i is skipped. If
the hardware descriptor ring contains stale data from previous work
requests, the NIC might execute the stale SGE and cause spurious DMA
mapping faults.

>  			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);

[Severity: Critical]
Could instructing the hardware to use rx_pkt_len_max for QP1 packets
cause a DMA buffer overflow? MAD receive buffers allocated by the IB core
are typically only large enough for GRH + MAD (~300 bytes). If an incoming
padded or malformed packet arrives, writing up to the port MTU size might
corrupt remote kernel memory.

>  			else
>  				WR_LE_32(data_seg->seg_len, wr->sg_list[i].length);
>  		}

> diff --git a/drivers/infiniband/hw/xsc/srq.c b/drivers/infiniband/hw/xsc/srq.c
> --- /dev/null
> +++ b/drivers/infiniband/hw/xsc/srq.c
[ ... ]
> @@ -51,11 +51,13 @@ void xsc_free_one_cache_srq_wr(struct xsc_ib_srq *srq, struct xsc_ib_qp *qp, u64
>  		if (tmp_wr->wr_id == wr_id) {
>  			if (prev_wr)
>  				prev_wr->next = tmp_wr->next;
>  			else
>  				qp->srq_head_wr = tmp_wr->next;
>  
>  			kfree(tmp_wr->sg_list);
>  			kfree(tmp_wr);
>  			break;
>  		}

[Severity: High]
If the item being freed is the tail of the list (tmp_wr == qp->srq_tail_wr),
does qp->srq_tail_wr need to be updated? If it is left dangling, a
subsequent call to xsc_post_cache_wrs() with init=false might execute
qp->srq_tail_wr->next = wr_head, leading to a use-after-free.

[ ... ]
> @@ -279,15 +279,16 @@ int xsc_ib_destroy_srq(struct ib_srq *srq, struct ib_udata *udata)
>  {
>  	struct xsc_ib_srq *xsrq = to_xsrq(srq);
>  	struct xsc_ib_dev *dev = to_xdev(srq->device);
>  
>  	xsc_core_destroy_srq(dev->xdev, &xsrq->xsrq);
>  	if (xsrq->create_type == XSC_SRQ_USER) {
>  		destroy_srq_user(xsrq, NULL);
>  	} else {
>  		xsc_destroy_cache_wr(xsrq);
>  		destroy_srq_kernel(dev, xsrq);
>  	}

[Severity: High]
Is there a race condition between releasing the SRQ ID globally and
clearing its local data? xsc_core_destroy_srq() frees the ID for immediate
reuse. If a concurrent thread creates an SRQ and reuses srqn before
xsc_destroy_cache_wr() executes memset(&cache_wr[srqn], 0, ...), it might
wipe out the newly allocated cache list of the new SRQ.

[ ... ]
> @@ -290,14 +290,17 @@ static int xsc_cache_srq_wr(struct xsc_ib_srq *srq, const struct ib_recv_wr *wr,
>  {
>  	int i, n;
>  	int srqn = srq->xsrq.srqn;
>  	struct xsc_srq_cache_wr *cache_wr = srq->xsrq.cache_wr;
>  	struct ib_recv_wr *head_wr = NULL, *new_wr = NULL, *prev_wr = NULL;
>  
>  	for (n = 0; wr; wr = wr->next, n++) {
>  		new_wr = kcalloc(1, sizeof(struct ib_recv_wr), GFP_KERNEL);

[Severity: High]
Can these allocations trigger a scheduling while atomic panic?
xsc_ib_post_srq_recv() acquires srq->lock with spin_lock_irqsave before
calling xsc_cache_srq_wr(), meaning this kcalloc(..., GFP_KERNEL) happens
with interrupts disabled.

>  		if (prev_wr)
>  			prev_wr->next = new_wr;
>  		else
>  			head_wr = new_wr;
>  		new_wr->wr_id = wr->wr_id;
>  		new_wr->sg_list = kcalloc(wr->num_sge, sizeof(struct ib_sge), GFP_KERNEL);

[Severity: High]
Are NULL checks missing for the kcalloc() returns? If either allocation
fails, dereferencing new_wr->wr_id or passing new_wr->sg_list to memcpy()
will cause a deterministic NULL pointer dereference.

>  		for (i = 0; i < wr->num_sge; i++)
>  			memcpy(&new_wr->sg_list[i], &wr->sg_list[i], sizeof(struct ib_sge));

[ ... ]
> @@ -355,14 +355,15 @@ void xsc_post_cache_wrs(struct xsc_ib_srq *srq, struct xsc_ib_qp *qp, int wr_num
>  
>  	cache_wr[srqn].head_wr = qp->srq_tail_wr->next;
>  	qp->srq_tail_wr->next = NULL;
>  	tmp_wr = wr_head;
>  	err = xsc_ib_post_recv(&qp->ibqp, tmp_wr, &bad_wr);
>  	if (err) {
>  		qp->srq_tail_wr->next = cache_wr[srqn].head_wr;
>  		qp->srq_tail_wr = NULL;
>  		for (; wr_head && (bad_wr->wr_id != wr_head->wr_id); wr_head = wr_head->next)
>  			qp->srq_tail_wr = wr_head;
>  
>  		if (!qp->srq_tail_wr) {
>  			cache_wr[srqn].head_wr = wr_head;
>  			qp->srq_head_wr = NULL;
>  		} else {

[Severity: High]
Does this error path lead to memory leaks and a subsequent NULL pointer
dereference? If xsc_ib_post_recv() fails on the very first element (e.g.,
due to WQ overflow), qp->srq_tail_wr is left as NULL. The code then
executes qp->srq_head_wr = NULL, orphaning all previously successfully
posted Work Requests and leaking their memory.

Additionally, because qp->srq_tail_wr is left as NULL, the next
invocation of xsc_post_cache_wrs() with init = false will execute:

qp->srq_tail_wr->next = wr_head;

This will cause a deterministic NULL pointer dereference.

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