[PATCH rdma-next 1/2] RDMA/irdma: Fix potential UAF during SRQ limit AE handling

[PATCH rdma-next 1/2] RDMA/irdma: Fix potential UAF during SRQ limit AE handling

[Jacob Moroni]

Previously, the raw SRQ pointer was used for the AE cookie
without any mechanism to ensure that the SRQ object was valid
and remained valid while processing the AE.

This could lead to a UAF if the SRQ happens to get destroyed
before the AE handler has a chance to process the AE (and any
other AEs in the queue in front of it...). It also means that
a bogus cookie from HW could cause a wild dereference.

Fix this by using xarray to perform a lookup based on the SRQ
ID, along with reference counting to ensure that the object
remains alive until the AE handler is done with it.

Fixes: 563e1feb5f6e ("RDMA/irdma: Add SRQ support")
Signed-off-by: Jacob Moroni <jmoroni@google.com>
---
 drivers/infiniband/hw/irdma/ctrl.c  |  7 ++++---
 drivers/infiniband/hw/irdma/hw.c    | 19 ++++++++++++++++---
 drivers/infiniband/hw/irdma/main.h  |  2 ++
 drivers/infiniband/hw/irdma/utils.c | 10 ++++++++++
 drivers/infiniband/hw/irdma/verbs.c | 10 ++++++++++
 drivers/infiniband/hw/irdma/verbs.h |  1 +
 6 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/drivers/infiniband/hw/irdma/ctrl.c b/drivers/infiniband/hw/irdma/ctrl.c
index 1c2ad5773b9f..709fb5985acf 100644
--- a/drivers/infiniband/hw/irdma/ctrl.c
+++ b/drivers/infiniband/hw/irdma/ctrl.c
@@ -514,7 +514,8 @@ static int irdma_sc_srq_create(struct irdma_sc_srq *srq, u64 scratch,
 		      FIELD_PREP(IRDMA_CQPSQ_SRQ_SRQ_LIMIT, srq->srq_limit) |
 		      FIELD_PREP(IRDMA_CQPSQ_SRQ_RQSIZE, srq->hw_srq_size) |
 		      FIELD_PREP(IRDMA_CQPSQ_SRQ_RQ_WQE_SIZE, srq->srq_uk.wqe_size));
-	set_64bit_val(wqe, 8, (uintptr_t)srq);
+	set_64bit_val(wqe, 8,
+		      FIELD_PREP(IRDMA_CQPSQ_SRQ_SRQCTX, srq->srq_uk.srq_id));
 	set_64bit_val(wqe, 16,
 		      FIELD_PREP(IRDMA_CQPSQ_SRQ_PD_ID, srq->pd->pd_id));
 	set_64bit_val(wqe, 32,
@@ -632,7 +633,8 @@ static int irdma_sc_srq_destroy(struct irdma_sc_srq *srq, u64 scratch,
 	if (!wqe)
 		return -ENOMEM;
 
-	set_64bit_val(wqe, 8, (uintptr_t)srq);
+	set_64bit_val(wqe, 8,
+		      FIELD_PREP(IRDMA_CQPSQ_SRQ_SRQCTX, srq->srq_uk.srq_id));
 
 	hdr = srq->srq_uk.srq_id |
 	      FIELD_PREP(IRDMA_CQPSQ_OPCODE, IRDMA_CQP_OP_DESTROY_SRQ) |
@@ -4730,7 +4732,6 @@ int irdma_sc_get_next_aeqe(struct irdma_sc_aeq *aeq,
 	switch (info->ae_id) {
 	case IRDMA_AE_SRQ_LIMIT:
 		info->srq = true;
-		/* [63:6] from CMPL_CTXT, [5:0] from WQDESCIDX. */
 		info->compl_ctx = compl_ctx;
 		ae_src = IRDMA_AE_SOURCE_RSVD;
 		break;
diff --git a/drivers/infiniband/hw/irdma/hw.c b/drivers/infiniband/hw/irdma/hw.c
index fbb8fb47d0d1..3d755cca243f 100644
--- a/drivers/infiniband/hw/irdma/hw.c
+++ b/drivers/infiniband/hw/irdma/hw.c
@@ -285,8 +285,9 @@ static void irdma_process_aeq(struct irdma_pci_f *rf)
 	struct irdma_sc_qp *qp = NULL;
 	struct irdma_qp_host_ctx_info *ctx_info = NULL;
 	struct irdma_device *iwdev = rf->iwdev;
-	struct irdma_sc_srq *srq;
+	struct irdma_srq *iwsrq;
 	unsigned long flags;
+	u32 srq_id;
 
 	u32 aeqcnt = 0;
 
@@ -437,8 +438,19 @@ static void irdma_process_aeq(struct irdma_pci_f *rf)
 			irdma_cq_rem_ref(&iwcq->ibcq);
 			break;
 		case IRDMA_AE_SRQ_LIMIT:
-			srq = (struct irdma_sc_srq *)(uintptr_t)info->compl_ctx;
-			irdma_srq_event(srq);
+			srq_id = FIELD_GET(IRDMA_CQPSQ_SRQ_SRQCTX,
+					   info->compl_ctx);
+			xa_lock_irqsave(&rf->srq_xa, flags);
+			iwsrq = xa_load(&rf->srq_xa, srq_id);
+			if (!iwsrq) {
+				xa_unlock_irqrestore(&rf->srq_xa, flags);
+				continue;
+			}
+			refcount_inc(&iwsrq->refcnt);
+			xa_unlock_irqrestore(&rf->srq_xa, flags);
+
+			irdma_srq_event(&iwsrq->sc_srq);
+			irdma_srq_rem_ref(iwsrq);
 			break;
 		case IRDMA_AE_SRQ_CATASTROPHIC_ERROR:
 			break;
@@ -2119,6 +2131,7 @@ static void irdma_set_hw_rsrc(struct irdma_pci_f *rf)
 	spin_lock_init(&rf->arp_lock);
 	spin_lock_init(&rf->qptable_lock);
 	spin_lock_init(&rf->cqtable_lock);
+	xa_init_flags(&rf->srq_xa, XA_FLAGS_LOCK_IRQ);
 	spin_lock_init(&rf->qh_list_lock);
 }
 
diff --git a/drivers/infiniband/hw/irdma/main.h b/drivers/infiniband/hw/irdma/main.h
index 8c17a201c1fd..813d0eb33ec8 100644
--- a/drivers/infiniband/hw/irdma/main.h
+++ b/drivers/infiniband/hw/irdma/main.h
@@ -324,6 +324,7 @@ struct irdma_pci_f {
 	spinlock_t cqtable_lock; /*protect CQ table access*/
 	struct irdma_qp **qp_table;
 	struct irdma_cq **cq_table;
+	struct xarray srq_xa; /* SRQ ID to struct irdma_srq */
 	spinlock_t qh_list_lock; /* protect mc_qht_list */
 	struct mc_table_list mc_qht_list;
 	struct irdma_msix_vector *iw_msixtbl;
@@ -526,6 +527,7 @@ void irdma_cq_add_ref(struct ib_cq *ibcq);
 void irdma_cq_rem_ref(struct ib_cq *ibcq);
 void irdma_cq_wq_destroy(struct irdma_pci_f *rf, struct irdma_sc_cq *cq);
 void irdma_srq_event(struct irdma_sc_srq *srq);
+void irdma_srq_rem_ref(struct irdma_srq *iwsrq);
 void irdma_srq_wq_destroy(struct irdma_pci_f *rf, struct irdma_sc_srq *srq);
 void irdma_cleanup_pending_cqp_op(struct irdma_pci_f *rf);
 int irdma_get_timeout_threshold(struct irdma_sc_dev *dev);
diff --git a/drivers/infiniband/hw/irdma/utils.c b/drivers/infiniband/hw/irdma/utils.c
index 1e5694a4fd91..f4bc08ae0553 100644
--- a/drivers/infiniband/hw/irdma/utils.c
+++ b/drivers/infiniband/hw/irdma/utils.c
@@ -834,6 +834,16 @@ void irdma_cq_rem_ref(struct ib_cq *ibcq)
 	complete(&iwcq->free_cq);
 }
 
+/**
+ * irdma_srq_rem_ref() - drop a reference to an SRQ
+ * @iwsrq: SRQ
+ */
+void irdma_srq_rem_ref(struct irdma_srq *iwsrq)
+{
+	if (refcount_dec_and_test(&iwsrq->refcnt))
+		complete(&iwsrq->free_srq);
+}
+
 struct ib_device *to_ibdev(struct irdma_sc_dev *dev)
 {
 	return &(container_of(dev, struct irdma_pci_f, sc_dev))->iwdev->ibdev;
diff --git a/drivers/infiniband/hw/irdma/verbs.c b/drivers/infiniband/hw/irdma/verbs.c
index 72de5738ae47..e07b6b40dd99 100644
--- a/drivers/infiniband/hw/irdma/verbs.c
+++ b/drivers/infiniband/hw/irdma/verbs.c
@@ -2114,6 +2114,10 @@ static int irdma_destroy_srq(struct ib_srq *ibsrq, struct ib_udata *udata)
 	if (ret)
 		return ret;
 
+	xa_erase_irq(&iwdev->rf->srq_xa, iwsrq->srq_num);
+	irdma_srq_rem_ref(iwsrq);
+	wait_for_completion(&iwsrq->free_srq);
+
 	irdma_srq_wq_destroy(iwdev->rf, srq);
 	irdma_srq_free_rsrc(iwdev->rf, iwsrq);
 	return 0;
@@ -2575,6 +2579,7 @@ static int irdma_create_srq(struct ib_srq *ibsrq,
 		return -EINVAL;
 
 	refcount_set(&iwsrq->refcnt, 1);
+	init_completion(&iwsrq->free_srq);
 	spin_lock_init(&iwsrq->lock);
 	err_code = irdma_alloc_rsrc(rf, rf->allocated_srqs, rf->max_srq,
 				    &iwsrq->srq_num, &rf->next_srq);
@@ -2643,6 +2648,11 @@ static int irdma_create_srq(struct ib_srq *ibsrq,
 			goto srq_destroy;
 	}
 
+	err_code = xa_insert_irq(&rf->srq_xa, iwsrq->srq_num, iwsrq,
+				 GFP_KERNEL);
+	if (err_code)
+		goto srq_destroy;
+
 	return 0;
 
 srq_destroy:
diff --git a/drivers/infiniband/hw/irdma/verbs.h b/drivers/infiniband/hw/irdma/verbs.h
index a1651641eb71..6f696a9cf107 100644
--- a/drivers/infiniband/hw/irdma/verbs.h
+++ b/drivers/infiniband/hw/irdma/verbs.h
@@ -129,6 +129,7 @@ struct irdma_srq {
 	struct irdma_dma_mem kmem;
 	u64 *srq_wrid_mem;
 	refcount_t refcnt;
+	struct completion free_srq;
 	spinlock_t lock; /* for poll srq */
 	struct irdma_pbl *iwpbl;
 	struct irdma_sge *sg_list;
-- 
2.56.0.542.g3ef89b7020-goog

[PATCH rdma-next 2/2] RDMA/irdma: Drop unused SRQ table from resource memory

[Jacob Moroni]

irdma_calc_mem_rsrc_size() reserves room for max_srq SRQ pointers at
the end of rf->mem_rsrc, but irdma_set_hw_rsrc() never sets up an SRQ
table there and nothing uses that memory. SRQs are looked up through
rf->srq_xa instead.

Remove the reservation so the allocation only covers what
irdma_set_hw_rsrc() actually carves out.

Signed-off-by: Jacob Moroni <jmoroni@google.com>
---
 drivers/infiniband/hw/irdma/hw.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/infiniband/hw/irdma/hw.c b/drivers/infiniband/hw/irdma/hw.c
index 3d755cca243f..db741afc80d5 100644
--- a/drivers/infiniband/hw/irdma/hw.c
+++ b/drivers/infiniband/hw/irdma/hw.c
@@ -2154,7 +2154,6 @@ static u32 irdma_calc_mem_rsrc_size(struct irdma_pci_f *rf)
 	rsrc_size += sizeof(unsigned long) * BITS_TO_LONGS(rf->max_mcg);
 	rsrc_size += sizeof(struct irdma_qp **) * rf->max_qp;
 	rsrc_size += sizeof(struct irdma_cq **) * rf->max_cq;
-	rsrc_size += sizeof(struct irdma_srq **) * rf->max_srq;
 
 	return rsrc_size;
 }
-- 
2.56.0.542.g3ef89b7020-goog

Re: [PATCH rdma-next 2/2] RDMA/irdma: Drop unused SRQ table from resource memory

[sashiko-bot]

> irdma_calc_mem_rsrc_size() reserves room for max_srq SRQ pointers at
> the end of rf->mem_rsrc, but irdma_set_hw_rsrc() never sets up an SRQ
> table there and nothing uses that memory. SRQs are looked up through
> rf->srq_xa instead.
> 
> Remove the reservation so the allocation only covers what
> irdma_set_hw_rsrc() actually carves out.
> 
> Signed-off-by: Jacob Moroni <jmoroni@google.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109202525.2937295-1-jmoroni@google.com?part=2

Re: [PATCH rdma-next 1/2] RDMA/irdma: Fix potential UAF during SRQ limit AE handling

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] RDMA/irdma: SRQ ID leak in irdma_create_srq() [1]
--

commit df1284da749f5b8315f44692f641894f41a2494c
Author: Jacob Moroni <jmoroni@google.com>

RDMA/irdma: Fix potential UAF during SRQ limit AE handling

This commit replaces the raw SRQ pointer in the AE cookie with the SRQ ID
and uses an XArray lookup with reference counting to prevent
use-after-free during SRQ limit AE processing.

> diff --git a/drivers/infiniband/hw/irdma/verbs.c b/drivers/infiniband/hw/irdma/verbs.c
> index 72de5738ae470..e07b6b40dd992 100644
> --- a/drivers/infiniband/hw/irdma/verbs.c
> +++ b/drivers/infiniband/hw/irdma/verbs.c
[ ... ]
> @@ -2575,6 +2579,7 @@ static int irdma_create_srq(struct ib_srq *ibsrq,
>  		return -EINVAL;
>  
>  	refcount_set(&iwsrq->refcnt, 1);
> +	init_completion(&iwsrq->free_srq);
>  	spin_lock_init(&iwsrq->lock);
>  	err_code = irdma_alloc_rsrc(rf, rf->allocated_srqs, rf->max_srq,
>  				    &iwsrq->srq_num, &rf->next_srq);

[Severity: High]
This problem wasn't introduced by this patch, but does irdma_create_srq()
leak the SRQ ID in iwsrq->srq_num [1] if irdma_get_srqdepth() fails?

drivers/infiniband/hw/irdma/verbs.c:irdma_create_srq() {
	...
	err_code = irdma_alloc_rsrc(rf, rf->allocated_srqs, rf->max_srq,
				    &iwsrq->srq_num, &rf->next_srq);
	if (err_code)
		return err_code;
	...
	err_code = irdma_get_srqdepth(ukinfo->uk_attrs, attr->max_wr,
				      shift, &depth);
	if (err_code)
		return err_code;
	...
}

When irdma_get_srqdepth() rejects invalid attr->max_wr values, returning
directly skips the free_rsrc cleanup path and leaves the bit set in
rf->allocated_srqs, allowing an unprivileged user to exhaust the device's
SRQ ID pool via uverbs.

[1] https://sashiko.dev/#/bug/linux-ec859386-8123-4946-9c12-458c13e3f421

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109202525.2937295-1-jmoroni@google.com?part=1

Re: [PATCH rdma-next 1/2] RDMA/irdma: Fix potential UAF during SRQ limit AE handling

[Jacob Moroni]

Yep, unrelated and will fix in a later patch.

- Jake