e1000: e1000_remove() skips cancel_work_sync() based on a racy __E1000_RESETTING test -- use-after-free write on the freed net_device

e1000: e1000_remove() skips cancel_work_sync() based on a racy __E1000_RESETTING test -- use-after-free write on the freed net_device

[徐晗翔]

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

e1000_remove() decides whether to wait for the driver's asynchronous
reset work by snapshotting a single bit ("Only kill reset task if
adapter is not resetting"), and when that test lands inside
e1000_reset_task's short __E1000_RESETTING window the driver skips
cancel_work_sync() entirely, so the remove-side frees (rings, iomaps,
free_netdev) race the tail of reset_task and its final
clear_bit(__E1000_RESETTING) becomes an atomic write into the already
freed net_device -- a real synchronization gap whose harmful
interleaving I verified with KASAN under a widened (timing-only)
window, and which the CVE-2025-38114 fix series carried into this
position verbatim, condition included.

The correct teardown for work items is an unconditional
cancel_work_sync(); the conditional form was inherited from the era
when the cancel sat inside the RTNL-held close path (that placement
was the deadlock CVE-2025-38114 fixed -- commits e400c7444d84, then
1fd4438ddcc4/b4a8085ceefb moved the three lines to e1000_remove() but
kept the condition).  At the current site, after unregister_netdev()
has returned, RTNL is no longer held, so waiting is safe.  Every other
__E1000_RESETTING setter runs under RTNL, and unregister_netdev()
deliberately releases RTNL at the start of its todo tail
(rcu_barrier + netdev_wait_allrefs, milliseconds) -- which is exactly
what makes the harmful interleaving reachable: a reset_task that was
scheduled before removal can acquire RTNL, enter
e1000_reinit_locked() and set __E1000_RESETTING during that tail,
after which e1000_remove()'s test_bit() reads true, the cancel is
skipped, and nothing else in the driver ever waits for reset_task
again.  The triggering inputs (tx timeout / link loss with queued TX
scheduling reset_task; rmmod, PCI hot-removal or sysfs unbind
entering e1000_remove) are ordinary operating conditions; the natural
timing window is narrow (detailed below), so I am reporting this as a
correctness/lifetime bug, not as an easily exploitable vulnerability.

The detailed description:

* Test environment / affected version
  Linux version 7.3.0 (gcc 13.3.0, GNU ld 2.42) #3 SMP PREEMPT_DYNAMIC
  -- vanilla mainline 7.3 release tarball (kernel.org), KASAN enabled,
  QEMU/KVM x86_64 guest with an emulated e1000 NIC on a user network.
  The controlled-window run used a kernel built with the attached
  timing-only verification patch (e1000.race_ctrl parameter, default
  off; with the parameter off the code path is the pristine one, and
  the negative control below was run exactly that way).

* The code (drivers/net/ethernet/intel/e1000/e1000_main.c, 7.3)
  e1000_remove(), after unregister_netdev():
      /* Only kill reset task if adapter is not resetting */
      if (!test_bit(__E1000_RESETTING, &adapter->flags))
              cancel_work_sync(&adapter->reset_task);
      e1000_phy_hw_reset(hw);          /* contains msleep(10) */
      kfree(adapter->tx_ring);
      kfree(adapter->rx_ring);
      ... iounmap ...
      free_netdev(netdev);            /* -> put_device -> netdev_release
                                         -> kvfree(netdev) */
  and the async counterpart, e1000_reset_task()
  (scheduled by e1000_tx_timeout() and by e1000_watchdog() on "link
  lost with outstanding TX"):
      rtnl_lock();
      e1000_reinit_locked(adapter);   /* spins on test_and_set_bit(
                                         __E1000_RESETTING), checks
                                         __E1000_DOWN, then: */
      rtnl_unlock();
      /* e1000_reinit_locked tail: */
      clear_bit(__E1000_RESETTING, &adapter->flags);
  This conditional cancel is the ONLY synchronization point for
  reset_task in the whole driver (e1000_close() does not cancel it;
  e1000_down_and_stop() only cancels the three delayed works).

* Why the skip is reachable (and why only via reset_task)
  Every __E1000_RESETTING setter runs inside an RTNL critical section
  (ethtool paths via the ethtool framework's RTNL, set_features,
  change_mtu, MII ioctls, e1000_close, reset_task itself).  The remove
  thread's unregister_netdev() takes and releases RTNL, then spends
  milliseconds in the todo tail (rcu_barrier(),
  netdev_wait_allrefs_any()) with RTNL already released.  In that
  interval a scheduled reset_task can take RTNL and set
  __E1000_RESETTING.  If e1000_remove()'s test_bit() runs inside
  reset_task's [set, clear] interval -- a few instructions wide --
  it reads true and skips the cancel.  All other RESETTING holders are
  excluded from this window: they either hold RTNL themselves (then
  unregister blocks on RTNL and the test happens after their clear) or
  hold a netdev reference (then netdev_wait_allrefs_any() blocks the
  todo tail).  reset_task is the only holder with neither.

* Consequence
  After the skipped cancel there is no happens-before between the
  remove tail (msleep(10) in the PHY reset, kfrees, iounmaps,
  free_netdev -> put_device -> netdev_release -> kvfree) and
  reset_task's tail (its __E1000_DOWN test and its
  clear_bit(__E1000_RESETTING)).  The adapter lives inside the
  net_device (netdev_priv), so if the worker is delayed past the
  remove tail, the DOWN test reads freed memory and the clear_bit
  writes freed memory.  Practically exploitable: essentially no
  (fixed bit position, dedicated slab, uncontrollable timing); it is
  a sporadic-crash-class defect that can accumulate under
  hot-removal/link-flap stress.

* Evidence (KASAN, controlled window; amplifier attached)
  The attached e1000-race-window.patch is timing-only and gated by
  e1000.race_ctrl (default off = pristine path).  With it, the
  RESETTING [set, clear] interval is widened to 1 s and e1000_remove()
  parks just before its test, dispatches the real reset_task and waits
  for the bit -- i.e. the audited interleaving is forced deterministi-
  cally, nothing else changes.  Result (7.3, KASAN):
    BUG: KASAN: slab-use-after-free in e1000_reinit_locked+0x67/0xb0
    Write of size 8 ... by task kworker/3:1
    Workqueue: events e1000_reset_task            <- the reset work
      (plus two reads at +0x47/+0x4f, the __E1000_DOWN test)
    Allocated by task 1:  alloc_netdev_mqs <- e1000_probe
    Freed by task 103:    kfree <- device_release <- kobject_put
                               <- e1000_remove <- unbind_store
    object: kmalloc-8k (struct net_device)
  Negative control: same kernel, same choreography, race_ctrl=0
  (pristine code path) -- zero splats over repeated unbind/bind with
  link churn, confirming the reports come from the widened window and
  that the natural nanosecond window is not hit by short hammering
  (matching the analysis: it needs the ns-scale [set, clear] plus a
  >=10 ms starvation of the worker afterwards).

  Reproducer usage (attached patch):
    patch -p1 < e1000-race-window.patch     # on a 7.3 tree
    # build a KASAN kernel; boot a VM with an e1000 NIC, then:
    echo 1 > /sys/module/e1000/parameters/race_ctrl
    ip link set eth0 up; ip addr add 10.0.2.15/24 dev eth0
    ping -i 0.3 10.0.2.2 &                  # some traffic
    # (drop the link or let the watchdog fire to arm reset_task)
    echo <bdf> > /sys/bus/pci/drivers/e1000/unbind
    # ~1 s later: the three KASAN reports above.

* Upstream lineage
  The conditional cancel originates from the CVE-2025-38114 story:
  e400c7444d84 (6.13) added cancel_work_sync(reset_task) inside the
  RTNL-held down path and deadlocked; 1fd4438ddcc4 (6.15.3) /
  b4a8085ceefb (6.16-rc2) moved the cancel to e1000_remove() but kept
  the "if not resetting" guard.  The guard's purpose (avoid waiting
  for RTNL while holding RTNL) does not apply at the new site, which
  runs after unregister_netdev() with no RTNL held.

* Suggested fix (one line, not attached -- happy to send as a formal
  patch)
      -	/* Only kill reset task if adapter is not resetting */
      -	if (!test_bit(__E1000_RESETTING, &adapter->flags))
      -		cancel_work_sync(&adapter->reset_task);
      +	cancel_work_sync(&adapter->reset_task);


Regards,
Hanxiang Xu
xuhx@hust.edu.cn



[-- Attachment #2: e1000-race-window.patch --]
[-- Type: application/octet-stream, Size: 2497 bytes --]

e1000-race-window.patch — controlled-window amplifier for the
e1000 remove()/reset_task TOCTOU bug report.

Timing-only, gated by e1000.race_ctrl (default OFF = pristine code
path).  Usage: apply on Linux 7.3 with `patch -p1 <`, build a KASAN
kernel, boot a VM with an (QEMU) e1000 NIC, then in the guest:

  echo 1 > /sys/module/e1000/parameters/race_ctrl
  ip link set eth0 up; ip addr add 10.0.2.15/24 dev eth0
  ping -i 0.3 10.0.2.2 &        # traffic; drop the NIC link to arm watchdog
  echo <bdf> > /sys/bus/pci/drivers/e1000/unbind
  # ~1 s later: KASAN slab-use-after-free in e1000_reinit_locked
  # (race_ctrl=0 over the same choreography = negative control)

The archived evidence quoted in the report (three KASAN reports with
alloc/free/work stacks, and the race_ctrl=0 negative control) was
produced with the same two window-widening edits this patch
reconstructs.
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -195,6 +195,11 @@
 module_param(debug, int, 0);
 MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
 
+/* VERIFICATION ONLY: widen natural timing windows for observation */
+static bool race_ctrl;
+module_param(race_ctrl, bool, 0600);
+MODULE_PARM_DESC(race_ctrl, "verification: widen RESETTING/remove windows");
+
 /**
  * e1000_get_hw_dev - helper function for getting netdev
  * @hw: pointer to HW struct
@@ -533,6 +538,12 @@
 	while (test_and_set_bit(__E1000_RESETTING, &adapter->flags))
 		msleep(1);
 
+	/* VERIFICATION ONLY (race_ctrl): widen [RESETTING set, clear] to 1s. */
+	if (race_ctrl) {
+		pr_info("E1000RACE: RESETTING set; widening [set, clear] to 1s, handshake with remove\n");
+		msleep(1000);
+	}
+
 	/* only run the task if not already down */
 	if (!test_bit(__E1000_DOWN, &adapter->flags)) {
 		e1000_down(adapter);
@@ -1261,6 +1272,16 @@
 
 	unregister_netdev(netdev);
 
+	/* VERIFICATION ONLY (race_ctrl): park before the racy test_bit,
+	 * dispatch the real reset_task, wait for RESETTING so test_bit
+	 * reads true and cancel_work_sync() is skipped (the audited race). */
+	if (race_ctrl) {
+		pr_info("E1000RACE: remove parked before test_bit; dispatching reset_task and waiting for RESETTING\n");
+		schedule_work(&adapter->reset_task);
+		while (!test_bit(__E1000_RESETTING, &adapter->flags))
+			msleep(10);
+	}
+
 	/* Only kill reset task if adapter is not resetting */
 	if (!test_bit(__E1000_RESETTING, &adapter->flags))
 		cancel_work_sync(&adapter->reset_task);