iommu: iommu_device_unregister() traversal racing concurrent client removal -> group use-after-free / refcount underflow (unconditional iommu_group_put, unlocked TOCTOU reads)

iommu: iommu_device_unregister() traversal racing concurrent client removal -> group use-after-free / refcount underflow (unconditional iommu_group_put, unlocked TOCTOU reads)

[徐晗翔]

[-- Attachment #1: Type: text/plain, Size: 7924 bytes --]

When iommu_device_unregister()'s bus traversal releases a client
device, nothing prevents iommu_release_device() from running on the
same device a second time from the device_del() bus notifier -- and
the release path's unlocked double-read of dev->iommu_group plus its
unconditional iommu_group_put() (outside the "device actually found"
branch) turn that double execution into a use-after-free of the group
and its devices kobject, or a refcount underflow that frees the group
early while other members still point at it.  Confirmed with KASAN on
mainline 7.3: in a 3000-round race, 7 slab-use-after-free reports in
kobject_put/refcount_warn_saturate with a closed three-stack witness
(group allocated by the probe side, freed by the unregister traversal,
touched again by the removal side), plus a NULL deref in
__iommu_probe_device.

The two kernel paths involved have no lock in common: the traversal
(remove_iommu_group(), drivers/iommu/iommu.c:273) and the bus notifier
(device_del -> BUS_NOTIFY_REMOVED_DEVICE -> iommu_bus_notifier,
iommu.c:1815) both funnel into iommu_release_device() (iommu.c:778),
which takes neither iommu_probe_device_lock nor the group lock before
its first read.  The kernel itself documents the hazard in the PCI
reset helpers ("callers must ensure there will be no racy
iommu_release_device() call, which otherwise would UAF the
dev->iommu_group pointer", iommu.c:4029-4031) -- that contract is a
caller-side convention with no mechanism behind it, and the traversal
and the notifier are exactly the callers that were never made to
ensure it.  Triggering needs a runtime-unregistrable IOMMU instance
(virtio-iommu sysfs unbind, several platform IOMMUs) plus a concurrent
client device removal (PCI remove/rescan, hotplug) -- both
administrator actions, but each entirely legitimate on its own.

The detailed description:

* Test environment / affected version

  Linux version 7.3.0 (gcc 13.3.0, GNU ld 2.42) #1 SMP PREEMPT_DYNAMIC
  -- vanilla mainline 7.3 release tarball (kernel.org, unmodified),
  rebuilt with CONFIG_KASAN_GENERIC=y, CONFIG_IOMMU_SUPPORT=y,
  CONFIG_IOMMU_API=y, QEMU/KVM x86_64 guest.  All functions cited
  below are byte-identical in 7.1.5 and 7.3 (function-level diff);
  line numbers are from 7.3.

* The two racing entries into the same release path

  A (instance unregister traversal):
      iommu_device_unregister() (iommu.c:316)
        -> bus_for_each_dev(..., remove_iommu_group)   /* :319 */
        -> remove_iommu_group()                        /* :273 */
             if (dev->iommu && dev->iommu->iommu_dev == data)   /* :275 */
                     iommu_release_device(dev);

  B (client removal notification):
      device_del() -> BUS_NOTIFY_REMOVED_DEVICE (fired without the
      device_lock held) -> iommu_bus_notifier() (iommu.c:1815)
        -> iommu_release_device(dev)

  Neither path takes iommu_probe_device_lock (which the probe side
  uses to make dev->iommu stable, iommu.c:626/715); the group lock is
  only acquired deep inside, after the unlocked reads.  The device
  model does not serialize them: klist_del() only marks the node dead
  and drops one reference, and the traversal's iterator pins the node
  with kref_get -- so A's callback and B's notifier run concurrently
  for the same device.

* What breaks inside iommu_release_device() (iommu.c:778-789)

      group = dev->iommu_group;             /* :781, unlocked read #1 */
      if (group)
              __iommu_group_remove_device(dev);
      if (dev->iommu)
              dev_iommu_free(dev);          /* :788, unlocked free when
                                                the group is gone */

  __iommu_group_remove_device() (iommu.c:751) then re-reads
  dev->iommu_group (read #2 at :753) and locks it (:755) -- between
  the two reads the other side can complete the whole removal inside
  the group lock (iommu_deinit_device() clears dev->iommu_group at
  :611 and frees the param at :613), so:

  - read #2 yields NULL -> mutex_lock(NULL) -> oops; or
  - read #2 yields the same group, the find loop finds nothing (the
    other side already list_del'ed it) and the code falls through to
    the UNCONDITIONAL iommu_group_put(group) at :775 -- a put with no
    matching get, because the only get paired with :775 is the group
    membership reference (iommu_group_add_device(), iommu.c:1338),
    which the winner already consumed.

  The group's reference sits on its devices kobject
  (kobject_get/put(group->devices_kobj), iommu.c:1448/1462), so the
  extra put either frees the group immediately (single-member,
  no-external-ref topology -> every later access, including the
  loser's own mutex_unlock and kobject_put, is a UAF) or drops the
  kref early (refcount underflow -> premature kfree while other
  members / the singleton pointer still reference it).

  Two more crossings of the same root, for completeness:
  - remove_iommu_group()'s second dereference at :275
    (dev->iommu->iommu_dev) races the param free (dev_iommu_free()
    clears dev->iommu and kfrees the param), including the
    probe-failure cleanup inside iommu_init_device() (iommu.c:544),
    which holds the probe lock that the traversal does not take:
    UAF read of the param.
  - the "group == NULL but param != NULL" branch free at :788 races
    the locked free at :613: double free of param/fwspec, or a
    NULL+0x28 read in the loser's path.

* Evidence (7.3, KASAN)

  Reproducer attached (iommurace.c): registers a fake IOMMU instance
  through the real exported core API and drives both entries with
  real platform-bus devices -- kthread A loops
  iommu_device_register/unregister, kthread B loops client
  platform_device_add/del.  Usage:

    insmod iommurace.ko
    echo setup > /sys/module/iommurace/parameters/action
    echo "ab 3000" > /sys/module/iommurace/parameters/action
    echo status > /sys/module/iommurace/parameters/action

  One 3000-round run (4 vCPU): 7 slab-use-after-free reports
  (kobject_put x4, refcount_warn_saturate x3 -- the refcount
  underflow family) plus one NULL deref at
  __iommu_probe_device+0x2ad (address 0x28) as B's probe walked state
  the concurrent unregister had already torn down; the module
  reference count stays pinned at 1 afterwards (the poisoned state
  skips the deinit that would module_put).  First report, closed
  three-stack witness:

    access:  slab-use-after-free in kobject_put
             by task iommurace_b (client removal side)
    allocated: kobject_create_and_add <- iommu_group_alloc
               <- __iommu_probe_device <- iommu_probe_device
               <- iommu_bus_notifier        (removal side's earlier probe)
    freed:     kfree <- kobject_put <- remove_iommu_group+0x67
               <- bus_for_each_dev
               <- iommu_device_unregister  (traversal side)

  This is exactly the "extra put frees the group / underflows the
  kref" interleaving described above.  (Sequential baselines --
  register/unregister loops and client add/del without the race --
  produce zero reports during the test phases.  Note for running the
  module: leave it loaded after the race actions; the poisoned state
  intentionally pins its refcount, and the module's own rmmod path
  has an unrelated software-node teardown ordering splat on 7.3.)

* Trigger model / affected configurations

  Any build where an IOMMU instance can be unregistered at runtime:
  virtio-iommu (sysfs unbind), several platform IOMMU drivers (some
  set suppress_bind_attrs; PCI hot-unplug paths remain), plus any
  concurrent client removal the platform produces (PCI remove/rescan,
  device_del on hotpluggable buses).  Both sides are administrator
  actions; the consequence is kernel memory corruption (UAF
  read/write, double free, premature group release leaving dangling
  dev->iommu_group / singleton pointers for later users).

Thanks for taking a look.

Regards,
Hanxiang Xu
xuhx@hust.edu.cn



[-- Attachment #2: iommurace.c --]
[-- Type: text/plain, Size: 12723 bytes --]

// SPDX-License-Identifier: GPL-2.0
/*
 * iommurace.c - dynamic verification driver for the IOMMU lifecycle race
 * audit (HV-3 / HV-4 in docs/manual-review-drivers-base-2026-10-20.md).
 *
 * Registers a FAKE IOMMU instance (struct iommu_device + struct iommu_ops)
 * through the real EXPORT_SYMBOL_GPL core APIs, and drives the REAL core
 * code paths of drivers/iommu/iommu.c with REAL platform-bus devices:
 *
 *   entry A (instance unregister traversal):
 *       iommu_device_unregister() -> bus_for_each_dev -> remove_iommu_group
 *   entry B (client removal):
 *       platform_device_unregister() -> device_del ->
 *       BUS_NOTIFY_REMOVED_DEVICE -> iommu_bus_notifier -> iommu_release_device
 *   entry C (client probe):
 *       platform_device_add() -> device_add -> BUS_NOTIFY_ADD_DEVICE ->
 *       iommu_bus_notifier -> iommu_probe_device -> iommu_init_device
 *
 * Clients are bound to the fake instance by calling iommu_fwspec_init()
 * (EXPORT_SYMBOL_GPL) between platform_device_alloc() and
 * platform_device_add(), so the ADD notifier's probe matches the fake ops
 * via iommu_fwspec_ops() on the shared software fwnode.
 *
 * actions (echo <cmd> > /sys/module/iommurace/parameters/action):
 *   setup           - carrier device + software fwnode + iommu_device_sysfs_add
 *                     + iommu_device_register (real bus_iommu_probe sweep)
 *   unreg           - one iommu_device_unregister (real traversal)
 *   reg             - one iommu_device_register
 *   regloop N       - N sequential register+unregister rounds (baseline)
 *   client N        - N clients added then removed sequentially (baseline;
 *                     expect zero splats with a stable registered instance)
 *   ab N            - HV-3 A x B: kthread A loops register+unregister,
 *                     kthread B loops client add+del; real paths on both sides
 *   ac N            - HV-4 A x C: kthread A loops register+unregister,
 *                     kthread C loops client add only (W1/W2 windows,
 *                     clients kept for post-mortem inspection)
 *   ac1 N           - same as ac but device_group = generic_single_device_group
 *                     (W3: unregister tail singleton_group race surface)
 *   cleanup         - best-effort teardown (unregister, drop clients, sysfs)
 *   status          - log stats
 *
 * Kernel source is NOT modified; everything is driven through exported
 * iommu core APIs, matching the methodology of pmu_race/mcrsim/regmap_race.
 *
 * NOTE: ab/ac/ac1 may leak module references when the audit's "W2 defective
 * state" hits (probe succeeded -> try_module_get, later removal skips
 * iommu_deinit_device -> no module_put). rmmod then fails with EBUSY; the
 * plans therefore do not rmmod after race actions - the leak itself is a
 * verified prediction of the report.
 */
#define pr_fmt(fmt) "IOMMURACE: " fmt

#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/kthread.h>
#include <linux/delay.h>
#include <linux/err.h>
#include <linux/string.h>
#include <linux/slab.h>
#include <linux/completion.h>
#include <linux/atomic.h>
#include <linux/platform_device.h>
#include <linux/iommu.h>
#include <linux/property.h>

/* ------------------------------------------------------------------ */
/* fake IOMMU instance                                                 */

static struct iommu_device fom_iommu;
static struct iommu_ops fom_ops;
static struct platform_device *carrier;
static struct fwnode_handle *sw_node;
static atomic_t reg_state = ATOMIC_INIT(0); /* 1 = registered */
static int use_singleton;

/* stats */
static unsigned long stat_reg, stat_unreg, stat_reg_fail;
static unsigned long stat_add_ok, stat_add_fail, stat_fw_fail, stat_del;
static unsigned long stat_b_done, stat_c_done, stat_a_done;

/* fake static identity domain: attach is a no-op, free must not kfree */
static int fom_attach_dev(struct iommu_domain *dom, struct device *dev,
			  struct iommu_domain *old)
{
	return 0;
}

static void fom_dom_free(struct iommu_domain *dom)
{
	pr_info("fom_dom_free on static identity domain (ignored)\n");
}

static const struct iommu_domain_ops fom_dom_ops = {
	.attach_dev = fom_attach_dev,
	.free = fom_dom_free,
};

static struct iommu_domain fom_identity_dom = {
	.type = IOMMU_DOMAIN_IDENTITY,
	.ops = &fom_dom_ops,
	.owner = &fom_ops,
	.pgsize_bitmap = 0,
	.geometry = {
		.aperture_start = 0,
		.aperture_end = ~0UL,
		.force_aperture = false,
	},
};

static struct iommu_device *fom_probe_device(struct device *dev)
{
	return &fom_iommu;
}

static void fom_release_device(struct device *dev)
{
}

static int fom_def_domain_type(struct device *dev)
{
	return IOMMU_DOMAIN_IDENTITY;
}

static int fom_register(void)
{
	int ret;

	if (atomic_xchg(&reg_state, 1) != 0)
		return 0; /* already registered */

	fom_ops = (struct iommu_ops){
		.owner			= THIS_MODULE,
		.probe_device		= fom_probe_device,
		.release_device		= fom_release_device,
		.device_group		= use_singleton ?
					  generic_single_device_group :
					  generic_device_group,
		.def_domain_type	= fom_def_domain_type,
		.identity_domain	= &fom_identity_dom,
	};

	ret = iommu_device_register(&fom_iommu, &fom_ops, &carrier->dev);
	if (ret) {
		atomic_set(&reg_state, 0);
		stat_reg_fail++;
		return ret;
	}
	stat_reg++;
	return 0;
}

static void fom_unregister(void)
{
	if (atomic_xchg(&reg_state, 0) != 1)
		return; /* not registered */
	iommu_device_unregister(&fom_iommu);
	stat_unreg++;
}

/* ------------------------------------------------------------------ */
/* fake client devices (real platform bus devices)                     */

#define MAX_CLI 512
static struct platform_device *cli[MAX_CLI];
static int nr_cli;

static int fom_new_client(struct platform_device **out)
{
	struct platform_device *pdev;
	int ret;

	pdev = platform_device_alloc("fomcli", PLATFORM_DEVID_AUTO);
	if (!pdev)
		return -ENOMEM;

	/*
	 * Bind the client's fwspec to the fake instance BEFORE device_add so
	 * the ADD notifier's iommu_probe_device() matches our fake ops.
	 * Fails with -ENODEV/-EPROBE_DEFER when the instance is currently
	 * unregistered / mid-registration: the natural non-racy branch.
	 */
	ret = iommu_fwspec_init(&pdev->dev, sw_node);
	if (ret) {
		stat_fw_fail++;
		platform_device_put(pdev);
		return ret;
	}

	ret = platform_device_add(pdev);
	if (ret) {
		stat_add_fail++;
		platform_device_put(pdev);
		return ret;
	}
	stat_add_ok++;
	*out = pdev;
	return 0;
}

static void fom_del_client(struct platform_device *pdev)
{
	platform_device_unregister(pdev);
	stat_del++;
}

/* ------------------------------------------------------------------ */
/* race threads                                                        */

struct rargs {
	int rounds;
};

static struct rargs ra, rb;

static int thread_a(void *data) /* entry A: register/unregister rounds */
{
	struct rargs *a = data;
	int i;

	for (i = 0; i < a->rounds; i++) {
		fom_register();
		fom_unregister();
		if ((i % 500) == 499)
			pr_info("thread_a round %d\n", i + 1);
		/*
		 * No sleep: maximize overlap of the unregister traversal
		 * with concurrent client add/del/probe on other CPUs.
		 */
	}
	stat_a_done++;
	pr_info("thread_a finished %d rounds\n", a->rounds);
	return 0;
}

static int thread_b(void *data) /* entry B: client add+del churn */
{
	struct rargs *a = data;
	int i;
	struct platform_device *pdev;

	for (i = 0; i < a->rounds; i++) {
		if (fom_new_client(&pdev))
			continue;
		fom_del_client(pdev);
		if ((i % 500) == 499)
			pr_info("thread_b round %d\n", i + 1);
	}
	stat_b_done++;
	pr_info("thread_b finished %d rounds (add_ok=%lu fw_fail=%lu)\n",
		a->rounds, stat_add_ok, stat_fw_fail);
	return 0;
}

static int thread_c(void *data) /* entry C: client add only (HV-4) */
{
	struct rargs *a = data;
	int i;

	for (i = 0; i < a->rounds; i++) {
		struct platform_device *pdev;

		if (nr_cli < MAX_CLI) {
			if (fom_new_client(&pdev) == 0)
				cli[nr_cli++] = pdev;
		}
		if ((i % 500) == 499)
			pr_info("thread_c round %d\n", i + 1);
	}
	stat_c_done++;
	pr_info("thread_c finished %d rounds (added=%d)\n", a->rounds, nr_cli);
	return 0;
}

static void run_race(const char *kind, int rounds)
{
	struct task_struct *ta, *tb;

	ra.rounds = rounds;
	rb.rounds = rounds;

	if (strcmp(kind, "ab") == 0)
		tb = kthread_run(thread_b, &rb, "iommurace_b");
	else
		tb = kthread_run(thread_c, &rb, "iommurace_c");
	ta = kthread_run(thread_a, &ra, "iommurace_a");
	if (IS_ERR(ta) || IS_ERR(tb)) {
		pr_err("kthread creation failed\n");
		return;
	}
	/* block until both sides complete (bounded: a crashed thread never
	 * increments its done counter - bail out after 150s and let the
	 * plan continue; the KASAN report in dmesg is the evidence) */
	{
		int waited = 0;

		while (stat_a_done + stat_b_done + stat_c_done < 2 &&
		       waited < 150000) {
			msleep(200);
			waited += 200;
		}
	}
	pr_info("race %s rounds=%d complete\n", kind, rounds);
}

/* ------------------------------------------------------------------ */
/* action parameter                                                    */

static char action_buf[64];

static int action_set(const char *val, const struct kernel_param *kp)
{
	char cmd[16];
	long n = 0;
	int got;

	strscpy(action_buf, val, sizeof(action_buf));
	got = sscanf(val, "%15s %ld", cmd, &n);
	if (got < 1)
		return -EINVAL;

	if (strcmp(cmd, "setup") == 0) {
		int ret;

		if (carrier) {
			pr_info("setup: already done\n");
			return 0;
		}
		sw_node = fwnode_create_software_node(NULL, NULL);
		if (IS_ERR(sw_node)) {
			sw_node = NULL;
			return PTR_ERR(sw_node);
		}
		carrier = platform_device_alloc("fomhw", PLATFORM_DEVID_AUTO);
		if (!carrier) {
			fwnode_remove_software_node(sw_node);
			sw_node = NULL;
			return -ENOMEM;
		}
		device_set_node(&carrier->dev, sw_node);
		ret = platform_device_add(carrier);
		if (ret) {
			platform_device_put(carrier);
			carrier = NULL;
			fwnode_remove_software_node(sw_node);
			sw_node = NULL;
			return ret;
		}
		ret = iommu_device_sysfs_add(&fom_iommu, &carrier->dev, NULL,
					     "fom_iommu0");
		if (ret) {
			platform_device_unregister(carrier);
			carrier = NULL;
			fwnode_remove_software_node(sw_node);
			sw_node = NULL;
			return ret;
		}
		ret = fom_register();
		pr_info("setup done (register ret=%d)\n", ret);
		return ret;
	}

	if (strcmp(cmd, "unreg") == 0) {
		fom_unregister();
		pr_info("unreg done\n");
		return 0;
	}
	if (strcmp(cmd, "reg") == 0) {
		int ret = fom_register();

		pr_info("reg ret=%d\n", ret);
		return ret;
	}
	if (strcmp(cmd, "regloop") == 0 && got == 2) {
		long i;

		for (i = 0; i < n; i++) {
			fom_register();
			fom_unregister();
		}
		pr_info("regloop %ld done\n", n);
		return 0;
	}
	if (strcmp(cmd, "client") == 0 && got == 2) {
		long i;
		int err = 0;

		if (n > MAX_CLI)
			n = MAX_CLI;
		for (i = 0; i < n; i++) {
			if (fom_new_client(&cli[nr_cli])) {
				err = 1;
				break;
			}
			nr_cli++;
		}
		msleep(500); /* let probes settle */
		for (i = 0; i < nr_cli; i++)
			fom_del_client(cli[i]);
		nr_cli = 0;
		pr_info("client %ld done (err=%d)\n", n, err);
		return 0;
	}
	if ((strcmp(cmd, "ab") == 0 || strcmp(cmd, "ac") == 0 ||
	     strcmp(cmd, "ac1") == 0) && got == 2) {
		use_singleton = (cmd[2] == '1');
		stat_a_done = stat_b_done = stat_c_done = 0;
		run_race(cmd, (int)n);
		return 0;
	}
	if (strcmp(cmd, "cleanup") == 0) {
		int i;

		fom_unregister();
		for (i = 0; i < nr_cli; i++)
			if (cli[i])
				fom_del_client(cli[i]);
		nr_cli = 0;
		pr_info("cleanup done (module refcount leak => rmmod EBUSY is expected after races)\n");
		return 0;
	}
	if (strcmp(cmd, "status") == 0) {
		pr_info("stats: reg=%lu unreg=%lu reg_fail=%lu add_ok=%lu add_fail=%lu fw_fail=%lu del=%lu\n",
			stat_reg, stat_unreg, stat_reg_fail, stat_add_ok,
			stat_add_fail, stat_fw_fail, stat_del);
		pr_info("clients tracked=%d singleton=%d\n", nr_cli, use_singleton);
		return 0;
	}

	return -EINVAL;
}

static const struct kernel_param_ops action_ops = {
	.set = action_set,
	.get = param_get_string,
};
module_param_cb(action, &action_ops, &action_buf, 0600);
MODULE_PARM_DESC(action, "setup|unreg|reg|regloop N|client N|ab N|ac N|ac1 N|cleanup|status");

static int __init iommurace_init(void)
{
	pr_info("loaded (kernel has IOMMU_API)\n");
	return 0;
}

static void __exit iommurace_exit(void)
{
	int i;

	fom_unregister();
	for (i = 0; i < nr_cli; i++)
		if (cli[i])
			fom_del_client(cli[i]);
	nr_cli = 0;
	if (carrier) {
		iommu_device_sysfs_remove(&fom_iommu);
		platform_device_unregister(carrier);
		fwnode_remove_software_node(sw_node);
	}
	pr_info("unloaded\n");
}

module_init(iommurace_init);
module_exit(iommurace_exit);

MODULE_LICENSE("GPL");
MODULE_DESCRIPTION("IOMMU lifecycle race verification driver (HV-3/HV-4)");

[-- Attachment #3: Makefile --]
[-- Type: application/octet-stream, Size: 502 bytes --]

# Common verification modules (no SPI dependency)
# Linux 7.3 extmod protocol: plain obj-m Makefile; build with
#   make -C <kernel-output-dir> M=<this-dir> modules
# mcrsim needs CONFIG_SPI=y (build-kasan-spi-7.3):
#   make MCRSIM=1 -C <spi-output-dir> M=<this-dir> modules
obj-m += pmu_race.o r4_stall.o regmap_race.o iommurace.o fibrace.o

ifeq ($(MCRSIM),1)
obj-m += mcrsim.o
endif

# userspace static helper for the fib verification (kernel-independent)
fibwr: fibwr.c
	$(CC) -static -O2 -o $@ $<

Re: iommu: iommu_device_unregister() traversal racing concurrent client removal -> group use-after-free / refcount underflow (unconditional iommu_group_put, unlocked TOCTOU reads)

[Robin Murphy]

On 22/10/2026 10:05 am, 徐晗翔 wrote:
> When iommu_device_unregister()'s bus traversal releases a client
> device, nothing prevents iommu_release_device() from running on the
> same device a second time from the device_del() bus notifier -- and
> the release path's unlocked double-read of dev->iommu_group plus its
> unconditional iommu_group_put() (outside the "device actually found"
> branch) turn that double execution into a use-after-free of the group
> and its devices kobject, or a refcount underflow that frees the group
> early while other members still point at it.  Confirmed with KASAN on
> mainline 7.3: in a 3000-round race, 7 slab-use-after-free reports in
> kobject_put/refcount_warn_saturate with a closed three-stack witness
> (group allocated by the probe side, freed by the unregister traversal,
> touched again by the removal side), plus a NULL deref in
> __iommu_probe_device.
> 
> The two kernel paths involved have no lock in common: the traversal
> (remove_iommu_group(), drivers/iommu/iommu.c:273) and the bus notifier
> (device_del -> BUS_NOTIFY_REMOVED_DEVICE -> iommu_bus_notifier,
> iommu.c:1815) both funnel into iommu_release_device() (iommu.c:778),
> which takes neither iommu_probe_device_lock nor the group lock before
> its first read.  The kernel itself documents the hazard in the PCI
> reset helpers ("callers must ensure there will be no racy
> iommu_release_device() call, which otherwise would UAF the
> dev->iommu_group pointer", iommu.c:4029-4031) -- that contract is a
> caller-side convention with no mechanism behind it, and the traversal
> and the notifier are exactly the callers that were never made to
> ensure it.  Triggering needs a runtime-unregistrable IOMMU instance
> (virtio-iommu sysfs unbind, several platform IOMMUs) plus a concurrent
> client device removal (PCI remove/rescan, hotplug) -- both
> administrator actions, but each entirely legitimate on its own.

Drivers really shouldn't allow unbinding - if we can pull an IOMMU out 
from underneath client devices with active DMA (especially to translated 
addresses), a theoretical race against device hot-remove is hardly the 
biggest problem facing the system in that case...

Properly-implemented modular drivers can only be removed if and when 
they have no client devices - which in practice means they're usually 
non-removable - so one way or another this race would not exist.

That does still leave the case of unwinding from iommu_device_register() 
failure, but again that's already a sufficiently unexpected and serious 
condition that we can't necessarily expect the system to continue in a 
usable state at all, and if you want to start worrying about hot-remove 
during IOMMU registration that's probably a much bigger can of worms anyway.

Thanks,
Robin.

> 
> The detailed description:
> 
> * Test environment / affected version
> 
>    Linux version 7.3.0 (gcc 13.3.0, GNU ld 2.42) #1 SMP PREEMPT_DYNAMIC
>    -- vanilla mainline 7.3 release tarball (kernel.org, unmodified),
>    rebuilt with CONFIG_KASAN_GENERIC=y, CONFIG_IOMMU_SUPPORT=y,
>    CONFIG_IOMMU_API=y, QEMU/KVM x86_64 guest.  All functions cited
>    below are byte-identical in 7.1.5 and 7.3 (function-level diff);
>    line numbers are from 7.3.
> 
> * The two racing entries into the same release path
> 
>    A (instance unregister traversal):
>        iommu_device_unregister() (iommu.c:316)
>          -> bus_for_each_dev(..., remove_iommu_group)   /* :319 */
>          -> remove_iommu_group()                        /* :273 */
>               if (dev->iommu && dev->iommu->iommu_dev == data)   /* :275 */
>                       iommu_release_device(dev);
> 
>    B (client removal notification):
>        device_del() -> BUS_NOTIFY_REMOVED_DEVICE (fired without the
>        device_lock held) -> iommu_bus_notifier() (iommu.c:1815)
>          -> iommu_release_device(dev)
> 
>    Neither path takes iommu_probe_device_lock (which the probe side
>    uses to make dev->iommu stable, iommu.c:626/715); the group lock is
>    only acquired deep inside, after the unlocked reads.  The device
>    model does not serialize them: klist_del() only marks the node dead
>    and drops one reference, and the traversal's iterator pins the node
>    with kref_get -- so A's callback and B's notifier run concurrently
>    for the same device.
> 
> * What breaks inside iommu_release_device() (iommu.c:778-789)
> 
>        group = dev->iommu_group;             /* :781, unlocked read #1 */
>        if (group)
>                __iommu_group_remove_device(dev);
>        if (dev->iommu)
>                dev_iommu_free(dev);          /* :788, unlocked free when
>                                                  the group is gone */
> 
>    __iommu_group_remove_device() (iommu.c:751) then re-reads
>    dev->iommu_group (read #2 at :753) and locks it (:755) -- between
>    the two reads the other side can complete the whole removal inside
>    the group lock (iommu_deinit_device() clears dev->iommu_group at
>    :611 and frees the param at :613), so:
> 
>    - read #2 yields NULL -> mutex_lock(NULL) -> oops; or
>    - read #2 yields the same group, the find loop finds nothing (the
>      other side already list_del'ed it) and the code falls through to
>      the UNCONDITIONAL iommu_group_put(group) at :775 -- a put with no
>      matching get, because the only get paired with :775 is the group
>      membership reference (iommu_group_add_device(), iommu.c:1338),
>      which the winner already consumed.
> 
>    The group's reference sits on its devices kobject
>    (kobject_get/put(group->devices_kobj), iommu.c:1448/1462), so the
>    extra put either frees the group immediately (single-member,
>    no-external-ref topology -> every later access, including the
>    loser's own mutex_unlock and kobject_put, is a UAF) or drops the
>    kref early (refcount underflow -> premature kfree while other
>    members / the singleton pointer still reference it).
> 
>    Two more crossings of the same root, for completeness:
>    - remove_iommu_group()'s second dereference at :275
>      (dev->iommu->iommu_dev) races the param free (dev_iommu_free()
>      clears dev->iommu and kfrees the param), including the
>      probe-failure cleanup inside iommu_init_device() (iommu.c:544),
>      which holds the probe lock that the traversal does not take:
>      UAF read of the param.
>    - the "group == NULL but param != NULL" branch free at :788 races
>      the locked free at :613: double free of param/fwspec, or a
>      NULL+0x28 read in the loser's path.
> 
> * Evidence (7.3, KASAN)
> 
>    Reproducer attached (iommurace.c): registers a fake IOMMU instance
>    through the real exported core API and drives both entries with
>    real platform-bus devices -- kthread A loops
>    iommu_device_register/unregister, kthread B loops client
>    platform_device_add/del.  Usage:
> 
>      insmod iommurace.ko
>      echo setup > /sys/module/iommurace/parameters/action
>      echo "ab 3000" > /sys/module/iommurace/parameters/action
>      echo status > /sys/module/iommurace/parameters/action
> 
>    One 3000-round run (4 vCPU): 7 slab-use-after-free reports
>    (kobject_put x4, refcount_warn_saturate x3 -- the refcount
>    underflow family) plus one NULL deref at
>    __iommu_probe_device+0x2ad (address 0x28) as B's probe walked state
>    the concurrent unregister had already torn down; the module
>    reference count stays pinned at 1 afterwards (the poisoned state
>    skips the deinit that would module_put).  First report, closed
>    three-stack witness:
> 
>      access:  slab-use-after-free in kobject_put
>               by task iommurace_b (client removal side)
>      allocated: kobject_create_and_add <- iommu_group_alloc
>                 <- __iommu_probe_device <- iommu_probe_device
>                 <- iommu_bus_notifier        (removal side's earlier probe)
>      freed:     kfree <- kobject_put <- remove_iommu_group+0x67
>                 <- bus_for_each_dev
>                 <- iommu_device_unregister  (traversal side)
> 
>    This is exactly the "extra put frees the group / underflows the
>    kref" interleaving described above.  (Sequential baselines --
>    register/unregister loops and client add/del without the race --
>    produce zero reports during the test phases.  Note for running the
>    module: leave it loaded after the race actions; the poisoned state
>    intentionally pins its refcount, and the module's own rmmod path
>    has an unrelated software-node teardown ordering splat on 7.3.)
> 
> * Trigger model / affected configurations
> 
>    Any build where an IOMMU instance can be unregistered at runtime:
>    virtio-iommu (sysfs unbind), several platform IOMMU drivers (some
>    set suppress_bind_attrs; PCI hot-unplug paths remain), plus any
>    concurrent client removal the platform produces (PCI remove/rescan,
>    device_del on hotpluggable buses).  Both sides are administrator
>    actions; the consequence is kernel memory corruption (UAF
>    read/write, double free, premature group release leaving dangling
>    dev->iommu_group / singleton pointers for later users).
> 
> Thanks for taking a look.
> 
> Regards,
> Hanxiang Xu
> xuhx@hust.edu.cn
> 
>