[PATCH v2] dmaengine: idxd: Fix use-after-free of idxd_wq

[PATCH v2] dmaengine: idxd: Fix use-after-free of idxd_wq

[Guixin Liu]

We found an idxd_wq use-after-free issue with kasan
when remove the idxd PCI device:

BUG: KASAN: slab-use-after-free in idxd_device_drv_remove+0x1f8/0x240 [idxd]
Call Trace:
  <TASK>
  dump_stack_lvl+0x32/0x50
  print_address_description.constprop.0+0x2c/0x390
  ? idxd_device_drv_remove+0x1f8/0x240 [idxd]
  print_report+0xba/0x280
  ? kasan_addr_to_slab+0x9/0xa0
  ? idxd_device_drv_remove+0x1f8/0x240 [idxd]
  kasan_report+0xab/0xe0
  ? idxd_device_drv_remove+0x1f8/0x240 [idxd]
  idxd_device_drv_remove+0x1f8/0x240 [idxd]
  device_release_driver_internal+0x391/0x560
  bus_remove_device+0x1f5/0x3f0
  device_del+0x392/0x990
  ? __pfx_device_del+0x10/0x10
  ? kobject_cleanup+0x117/0x360
  ? idxd_unregister_devices+0x229/0x320 [idxd]
  device_unregister+0x13/0xa0
  idxd_remove+0x4f/0x1b0 [idxd]
  pci_device_remove+0xa7/0x1d0
  device_release_driver_internal+0x391/0x560
  ? pci_pme_active+0x1e/0x450
  pci_stop_bus_device+0x10a/0x150
  pci_stop_and_remove_bus_device_locked+0x16/0x30
  remove_store+0xcf/0xe0

Freed by task 15535:
  kasan_save_stack+0x1c/0x40
  kasan_set_track+0x21/0x30
  kasan_save_free_info+0x27/0x40
  ____kasan_slab_free+0x171/0x240
  slab_free_freelist_hook+0xde/0x190
  __kmem_cache_free+0x19e/0x310
  device_release+0x98/0x210
  kobject_cleanup+0x102/0x360
  idxd_unregister_devices+0xb3/0x320 [idxd]
  dxd_remove+0x3f/0x1b0 [idxd]
  pci_device_remove+0xa7/0x1d0
  device_release_driver_internal+0x391/0x560
  pci_stop_bus_device+0x10a/0x150
  pci_stop_and_remove_bus_device_locked+0x16/0x30
  remove_store+0xcf/0xe0

In the idxd_remove() flow, when execution reaches
idxd_unregister_devices(), all idxd_wq instances have already been
freed. Subsequently, when device_unregister(idxd_confdev(idxd)) is
executed, it calls into idxd_device_drv_remove() which accesses the
already-freed idxd_wq. This fix resolves the issue by calling
device_release_driver() before idxd_unregister_devices().

Fixes: 98da0106aac0d ("dmanegine: idxd: fix resource free ordering on driver removal")
Co-developed-by: Shuai Xue <xueshuai@linux.alibaba.com>
Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
v1 -> v2:
  1. Call device_release_driver() in advance instead of swapping the order of
     device_unregister() and idxd_unregister_devices().
  2. Add Co-developed-by: Shuai Xue <xueshuai@linux.alibaba.com>.
 drivers/dma/idxd/init.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index f1cfc7790d95..3b0a0363ca65 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -1293,13 +1293,30 @@ static void idxd_remove(struct pci_dev *pdev)
 {
 	struct idxd_device *idxd = pci_get_drvdata(pdev);
 
+	/*
+	 * The idxd sub-driver's remove callback (idxd_device_drv_remove())
+	 * iterates idxd->wqs[] and accesses wq objects. We must unbind the
+	 * sub-driver before idxd_unregister_devices() frees these objects,
+	 * otherwise a use-after-free occurs.
+	 *
+	 * We cannot simply reorder device_unregister(idxd_confdev) before
+	 * idxd_unregister_devices() because device_del() -> kobject_del()
+	 * recursively removes the parent's sysfs directory, which destroys
+	 * children's sysfs entries. Subsequent device_unregister() on the
+	 * children then fails with "sysfs group 'power' not found".
+	 *
+	 * Use device_release_driver() to only unbind the driver (triggering
+	 * idxd_device_drv_remove()) without touching sysfs. Then safely
+	 * unregister children before the parent.
+	 */
+	device_release_driver(idxd_confdev(idxd));
 	idxd_unregister_devices(idxd);
+
 	/*
-	 * When ->release() is called for the idxd->conf_dev, it frees all the memory related
-	 * to the idxd context. The driver still needs those bits in order to do the rest of
-	 * the cleanup. However, we do need to unbound the idxd sub-driver. So take a ref
-	 * on the device here to hold off the freeing while allowing the idxd sub-driver
-	 * to unbind.
+	 * When ->release() is called for the idxd->conf_dev, it frees all the
+	 * memory related to the idxd context. The driver still needs those bits
+	 * in order to do the rest of the cleanup. So take a ref on the device
+	 * here to hold off the freeing.
 	 */
 	get_device(idxd_confdev(idxd));
 	device_unregister(idxd_confdev(idxd));
-- 
2.32.0.3.g01195cf9f