[PATCH] RDMA/cma: publish service records after initializing them

[PATCH] RDMA/cma: publish service records after initializing them

[lirongqing]

From: Li RongQing <lirongqing@baidu.com>

cma_query_ib_service_handler() stores the freshly allocated
service_recs array and its count before the records are filled in:

	id_priv->id.route.service_recs = kmalloc_objs(*recs, num_recs);
	...
	id_priv->id.route.num_service_recs = num_recs;
	memcpy(id_priv->id.route.service_recs, recs, ...);

ucma_query_ib_service() only holds ctx->mutex and is not serialized
against this SA callback.  It gates on service_recs being non-NULL
and then copies num_service_recs records out to user space.  A task
spinning on RDMA_USER_CM_CMD_QUERY while another one issues
RDMA_USER_CM_CMD_RESOLVE_IB_SERVICE can therefore see a non-NULL
pointer together with a non-zero count while the kmalloc'ed array
still holds uninitialized heap data, and leak it through
copy_to_user().  Nothing ordered the stores either, so the reader
could also observe them out of order.

Fill a local allocation first, set the count, then publish the array
with smp_store_release().  The reader fetches it once with a
matching smp_load_acquire() and copies from that value, so a visible
pointer now implies that both the count and the record contents are
visible as well.

Fixes: a6404823fe20 ("RDMA/cma: Support IB service record resolution")
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 drivers/infiniband/core/cma.c  | 14 ++++++++++----
 drivers/infiniband/core/ucma.c |  8 +++++---
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 73170b1..4df9b48 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -5556,6 +5556,7 @@ static void cma_query_ib_service_handler(int status,
 {
 	struct cma_work *work = context;
 	struct rdma_id_private *id_priv = work->id;
+	struct sa_service_rec *service_recs;
 	struct sockaddr_ib *addr;
 
 	if (status)
@@ -5571,15 +5572,20 @@ static void cma_query_ib_service_handler(int status,
 		goto fail;
 	}
 
-	id_priv->id.route.service_recs =
-		kmalloc_objs(*recs, num_recs);
-	if (!id_priv->id.route.service_recs) {
+	service_recs = kmalloc_objs(*recs, num_recs);
+	if (!service_recs) {
 		status = -ENOMEM;
 		goto fail;
 	}
 
+	memcpy(service_recs, recs, sizeof(*recs) * num_recs);
 	id_priv->id.route.num_service_recs = num_recs;
-	memcpy(id_priv->id.route.service_recs, recs, sizeof(*recs) * num_recs);
+	/*
+	 * Readers such as ucma_query_ib_service() are not serialized against
+	 * this handler and gate on service_recs, so publish it only once the
+	 * records and the count are fully written.
+	 */
+	smp_store_release(&id_priv->id.route.service_recs, service_recs);
 
 	addr = (struct sockaddr_ib *)&id_priv->id.route.addr.dst_addr;
 	addr->sib_family = AF_IB;
diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c
index 4929636..2b4762f7 100644
--- a/drivers/infiniband/core/ucma.c
+++ b/drivers/infiniband/core/ucma.c
@@ -1025,12 +1025,15 @@ static ssize_t ucma_query_ib_service(struct ucma_context *ctx,
 				     void __user *response, int out_len)
 {
 	struct rdma_ucm_query_ib_service_resp *resp;
+	struct sa_service_rec *service_recs;
 	int n, ret = 0;
 
 	if (out_len < sizeof(struct rdma_ucm_query_ib_service_resp))
 		return -ENOSPC;
 
-	if (!ctx->cm_id->route.service_recs)
+	/* Pairs with the release store in cma_query_ib_service_handler() */
+	service_recs = smp_load_acquire(&ctx->cm_id->route.service_recs);
+	if (!service_recs)
 		return -ENODATA;
 
 	resp = kzalloc(out_len, GFP_KERNEL);
@@ -1048,8 +1051,7 @@ static ssize_t ucma_query_ib_service(struct ucma_context *ctx,
 	if (n > ctx->cm_id->route.num_service_recs)
 		n = ctx->cm_id->route.num_service_recs;
 
-	memcpy(resp->recs, ctx->cm_id->route.service_recs,
-	       sizeof(*resp->recs) * n);
+	memcpy(resp->recs, service_recs, sizeof(*resp->recs) * n);
 	if (copy_to_user(response, resp, struct_size(resp, recs, n)))
 		ret = -EFAULT;
 
-- 
2.9.4

Re: [PATCH] RDMA/cma: publish service records after initializing them

[Leon Romanovsky]

On Wed, Aug 26, 2026 at 07:02:13PM +0800, lirongqing wrote:
> From: Li RongQing <lirongqing@baidu.com>
> 
> cma_query_ib_service_handler() stores the freshly allocated
> service_recs array and its count before the records are filled in:
> 
> 	id_priv->id.route.service_recs = kmalloc_objs(*recs, num_recs);
> 	...
> 	id_priv->id.route.num_service_recs = num_recs;
> 	memcpy(id_priv->id.route.service_recs, recs, ...);
> 
> ucma_query_ib_service() only holds ctx->mutex and is not serialized
> against this SA callback.

No, this sentence is wrong.
ucma_query_ib_service() holds ctx->mutex and cma_query_ib_service_handler()
is executed under same mutex too.

mutex_lock(&ctx->mutex)
rdma_resolve_ib_service() ->
	cma_resolve_ib_service() ->
		ib_sa_service_rec_get() ->
			cma_query_ib_service_handler()

Thanks