[PATCH] IB/cm: hold a cm_id reference across the send error callback

[PATCH] IB/cm: hold a cm_id reference across the send error callback

[Yehyeong Lee]

cm_process_send_error() lets cm_free_priv_msg() release the reference the
message held on the cm_id and then keeps using cm_id_priv, dropping
cm_id_priv->lock and calling the consumer's cm_handler() callback. If that
was the last reference, a concurrent ib_destroy_cm_id() takes the count to
zero and returns while the callback is still running, so the consumer frees
its context underneath it. For an RDMA CM consumer that context is the
rdma_id_private which _destroy_id() kfree()s once ib_destroy_cm_id() has
returned, and the callback reads it through cm_id->context before any state
check.

Reaching this needs a send that expects a response, so a REQ, REP, DREQ or
SIDR_REQ, to complete with a real error rather than a flush, and the
consumer to destroy the id while that error is being processed.

The report below comes from an out-of-tree KUnit test. The race is a few
instructions wide, so the test blocks after the spin_unlock_irq() until the
racing destroy has finished and KASAN can observe the access. The cm_kt_
frames stand in for a consumer and its destroy path, and the test is not
part of this patch.

  BUG: KASAN: slab-use-after-free in cm_kt_handler+0xaa/0xd0
  Read of size 4 at addr ffff8880029582c0 by task kunit_try_catch/39

  CPU: 0 UID: 0 PID: 39 Comm: kunit_try_catch Tainted: G                 N   7.3.0-07363-g734bef548a96-dirty #1 PREEMPT(full)
  Tainted: [N]=TEST
  Call Trace:
   <TASK>
   print_report+0xcd/0x620
   ? cm_kt_handler+0xaa/0xd0
   ? __virt_addr_valid+0x208/0x3f0
   ? cm_kt_handler+0xaa/0xd0
   kasan_report+0xfb/0x130
   ? cm_kt_handler+0xaa/0xd0
   cm_kt_handler+0xaa/0xd0
   cm_process_send_error+0x2d9/0x560
   ? __pfx_cm_process_send_error+0x10/0x10
   ? __lock_acquire+0x57d/0x24a0
   ? trace_hardirqs_on+0x17/0x100
   cm_kt_destroy_returns_during_callback+0x1e8/0x700
   ? __pfx_cm_kt_destroy_returns_during_callback+0x10/0x10
   ? kunit_try_run_case+0x1a4/0x4c0
   ? lock_release+0xc3/0x250
   ? lockdep_hardirqs_on_prepare+0xd5/0x180
   ? ktime_get_ts64+0x1e5/0x2e0
   ? trace_hardirqs_on+0x17/0x100
   ? __pfx_read_tsc+0x10/0x10
   ? ktime_get_ts64+0x242/0x2e0
   ? __init_swait_queue_head+0xca/0x150
   kunit_try_run_case+0x1ca/0x4c0
   ? __pfx_kunit_try_run_case+0x10/0x10
   ? lock_release+0xc3/0x250
   ? lockdep_hardirqs_on_prepare+0xd5/0x180
   ? _raw_spin_unlock_irqrestore+0x3f/0x50
   ? trace_hardirqs_on+0x17/0x100
   ? __pfx_kunit_try_run_case+0x10/0x10
   ? __pfx_kunit_generic_run_threadfn_adapter+0x10/0x10
   kunit_generic_run_threadfn_adapter+0x7b/0xe0
   kthread+0x3aa/0x550
   ? _raw_spin_unlock_irq+0x23/0x40
   ? __pfx_kthread+0x10/0x10
   ret_from_fork+0x493/0x940
   ? __pfx_ret_from_fork+0x10/0x10
   ? __switch_to+0x22/0xdb0
   ? __pfx_kthread+0x10/0x10
   ret_from_fork_asm+0x19/0x30
   </TASK>

  Allocated by task 39:
   kasan_save_stack+0x33/0x60
   kasan_save_track+0x14/0x30
   __kasan_kmalloc+0x8f/0xa0
   __kmalloc_cache_noprof+0x201/0x4a0
   cm_kt_make_id.constprop.0+0xa0/0x470
   cm_kt_destroy_returns_during_callback+0x84/0x700
   kunit_try_run_case+0x1ca/0x4c0
   kunit_generic_run_threadfn_adapter+0x7b/0xe0
   kthread+0x3aa/0x550
   ret_from_fork+0x493/0x940
   ret_from_fork_asm+0x19/0x30

  Freed by task 40:
   kasan_save_stack+0x33/0x60
   kasan_save_track+0x14/0x30
   kasan_save_free_info+0x3b/0x60
   __kasan_slab_free+0x43/0x70
   kfree+0x119/0x490
   cm_kt_destroyer+0x31/0x50
   kthread+0x3aa/0x550
   ret_from_fork+0x493/0x940
   ret_from_fork_asm+0x19/0x30

  The buggy address belongs to the object at ffff8880029582c0
   which belongs to the cache kmalloc-8 of size 8
  The buggy address is located 0 bytes inside of
   freed 8-byte region [ffff8880029582c0, ffff8880029582c8)

  The buggy address belongs to the physical page:
  page: refcount:0 mapcount:0 mapping:0000000000000000 index:0xffff888002958900 pfn:0x2958
  flags: 0x4000000000000200(workingset|zone=1)
  page_type: f5(slab)
  raw: 4000000000000200 ffff888001041500 ffff888001040348 ffff888001040348
  raw: ffff888002958900 0000000000800048 00000000f5000000 0000000000000000
  page dumped because: kasan: bad access detected

  Memory state around the buggy address:
   ffff888002958180: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
   ffff888002958200: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
  >ffff888002958280: fc fc fc fc 00 fc fc fc fa fc fc fc 00 fc fc fc
                                             ^
   ffff888002958300: 02 fc fc fc 02 fc fc fc 02 fc fc fc 02 fc fc fc
   ffff888002958380: 02 fc fc fc 02 fc fc fc 02 fc fc fc 02 fc fc fc

This reference used to be released after the callback, which kept the cm_id
alive for its duration. Restore that ordering and drop it on every return
path.

Fixes: c1cf6d9f743aa ("IB/cm: Call the correct message free functions in cm_send_handler()")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/infiniband/core/cm.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
index 1a2c2775b14d5..67efb70ba58d1 100644
--- a/drivers/infiniband/core/cm.c
+++ b/drivers/infiniband/core/cm.c
@@ -3768,6 +3768,13 @@ static void cm_process_send_error(struct cm_id_private *cm_id_priv,
 		cm_deref_id(cm_id_priv);
 		return;
 	}
+
+	/*
+	 * cm_free_priv_msg() drops the reference the message held, so hold one
+	 * across the callback below or a concurrent ib_destroy_cm_id() can free
+	 * the id underneath it.
+	 */
+	refcount_inc(&cm_id_priv->refcount);
 	cm_free_priv_msg(msg);
 
 	if (state != cm_id_priv->id.state || wc_status == IB_WC_SUCCESS ||
@@ -3802,11 +3809,13 @@ static void cm_process_send_error(struct cm_id_private *cm_id_priv,
 
 	/* No other events can occur on the cm_id at this point. */
 	ret = cm_id_priv->id.cm_handler(&cm_id_priv->id, &cm_event);
+	cm_deref_id(cm_id_priv);
 	if (ret)
 		ib_destroy_cm_id(&cm_id_priv->id);
 	return;
 out_unlock:
 	spin_unlock_irq(&cm_id_priv->lock);
+	cm_deref_id(cm_id_priv);
 }
 
 static void cm_send_handler(struct ib_mad_agent *mad_agent,
-- 
2.43.0

Re: [PATCH] IB/cm: hold a cm_id reference across the send error callback

[sashiko-bot]

> cm_process_send_error() lets cm_free_priv_msg() release the reference the
> message held on the cm_id and then keeps using cm_id_priv, dropping
> cm_id_priv->lock and calling the consumer's cm_handler() callback. If that
> was the last reference, a concurrent ib_destroy_cm_id() takes the count to
> zero and returns while the callback is still running, so the consumer frees
> its context underneath it. For an RDMA CM consumer that context is the
> rdma_id_private which _destroy_id() kfree()s once ib_destroy_cm_id() has
> returned, and the callback reads it through cm_id->context before any state
> check.
> 
> Reaching this needs a send that expects a response, so a REQ, REP, DREQ or
> SIDR_REQ, to complete with a real error rather than a flush, and the
> consumer to destroy the id while that error is being processed.
> 
> The report below comes from an out-of-tree KUnit test. The race is a few
> [ ... ]
> path.
> 
> Fixes: c1cf6d9f743aa ("IB/cm: Call the correct message free functions in cm_send_handler()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024155507.3418345-1-yhlee@isslab.korea.ac.kr?part=1