[PATCH v12 1/3] can: j1939: fix possibility of UAF in __j1939_ecu_release()

[PATCH v12 1/3] can: j1939: fix possibility of UAF in __j1939_ecu_release()

[Tetsuo Handa]

Since j1939_ac_recv() and j1939_ac_fixup() are calling j1939_ecu_put()
without priv->lock held for write, list corruption and/or UAF might happen
if the last user called j1939_ecu_put() without priv->lock held for write.

Fix this problem by deferring release steps to WQ context where we can
unconditionally hold priv->lock for write.

By deferring to WQ context, readers might find ECUs which have already
reached __j1939_ecu_release(). Therefore, allow using an ECU only when
__j1939_ecu_release() is not yet called (indicated by ecu->kref > 0).

Since pending ecu->ac_timer is not canceled during j1939_ecu_unmap_all()
or __j1939_rx_release(), ecu->ac_timer can call j1939_ecu_timer_handler()
after __j1939_ecu_release() or j1939_module_exit().

Fix this problem by explicitly canceling ecu->ac_timer before kfree().

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Cc: stable@vger.kernel.org
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 net/can/j1939/bus.c        | 68 ++++++++++++++++++++++++++++----------
 net/can/j1939/j1939-priv.h |  4 +++
 net/can/j1939/main.c       | 13 ++++++++
 3 files changed, 67 insertions(+), 18 deletions(-)

diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
index cdc3c0a71937..c49c96dea796 100644
--- a/net/can/j1939/bus.c
+++ b/net/can/j1939/bus.c
@@ -14,25 +14,54 @@
 
 #include "j1939-priv.h"
 
-static void __j1939_ecu_release(struct kref *kref)
+struct workqueue_struct *j1939_ecu_wq;
+
+static void __j1939_ecu_release_work(struct work_struct *work)
 {
-	struct j1939_ecu *ecu = container_of(kref, struct j1939_ecu, kref);
+	struct j1939_ecu *ecu = container_of(work, struct j1939_ecu, release_work);
 	struct j1939_priv *priv = ecu->priv;
 
+	write_lock_bh(&priv->lock);
 	list_del(&ecu->list);
+	write_unlock_bh(&priv->lock);
+	/* There can't be any ref on this ECU because __j1939_ecu_release()
+	 * was already called. But we need to make sure that
+	 * j1939_ecu_timer_handler() is no longer running before kfree().
+	 */
+	WARN_ON(hrtimer_cancel(&ecu->ac_timer));
 	netdev_put(priv->ndev, &ecu->priv_dev_tracker);
 	kfree(ecu);
 	j1939_priv_put(priv);
 }
 
+static void __j1939_ecu_release(struct kref *kref)
+{
+	struct j1939_ecu *ecu = container_of(kref, struct j1939_ecu, kref);
+
+	/* Since not all callers are calling j1939_ecu_put() with priv->lock
+	 * held for write, defer actual release to WQ context. By deferring to
+	 * WQ context, readers might find ECUs with ecu->kref == 0. Therefore,
+	 * readers must validate with j1939_ecu_valid() or j1939_ecu_get().
+	 */
+	INIT_WORK(&ecu->release_work, __j1939_ecu_release_work);
+	queue_work(j1939_ecu_wq, &ecu->release_work);
+}
+
 void j1939_ecu_put(struct j1939_ecu *ecu)
 {
 	kref_put(&ecu->kref, __j1939_ecu_release);
 }
 
-static void j1939_ecu_get(struct j1939_ecu *ecu)
+static bool __must_check j1939_ecu_get(struct j1939_ecu *ecu)
+{
+	/* Don't get an ECU ref if __j1939_ecu_release() was called. */
+	return kref_get_unless_zero(&ecu->kref);
+}
+
+static bool __must_check j1939_ecu_valid(struct j1939_ecu *ecu)
 {
-	kref_get(&ecu->kref);
+	/* Treat an ECU as invalid if __j1939_ecu_release() was called. */
+	return kref_read(&ecu->kref);
 }
 
 static bool j1939_ecu_is_mapped_locked(struct j1939_ecu *ecu)
@@ -56,15 +85,17 @@ static void j1939_ecu_map_locked(struct j1939_ecu *ecu)
 	if (!j1939_address_is_unicast(ecu->addr))
 		return;
 
+	if (!j1939_ecu_get(ecu))
+		return;
+
 	ent = &priv->ents[ecu->addr];
 
 	if (ent->ecu) {
 		netdev_warn(priv->ndev, "Trying to map already mapped ECU, addr: 0x%02x, name: 0x%016llx. Skip it.\n",
 			    ecu->addr, ecu->name);
+		j1939_ecu_put(ecu);
 		return;
 	}
-
-	j1939_ecu_get(ecu);
 	ent->ecu = ecu;
 	ent->nusers += ecu->nusers;
 }
@@ -109,10 +140,11 @@ void j1939_ecu_unmap_all(struct j1939_priv *priv)
 
 void j1939_ecu_timer_start(struct j1939_ecu *ecu)
 {
-	/* The ECU is held here and released in the
+	/* The corresponding j1939_ecu_put() is in
 	 * j1939_ecu_timer_handler() or j1939_ecu_timer_cancel().
 	 */
-	j1939_ecu_get(ecu);
+	if (!j1939_ecu_get(ecu))
+		return;
 
 	/* Schedule timer in 250 msec to commit address change. */
 	hrtimer_start(&ecu->ac_timer, ms_to_ktime(250),
@@ -122,6 +154,7 @@ void j1939_ecu_timer_start(struct j1939_ecu *ecu)
 void j1939_ecu_timer_cancel(struct j1939_ecu *ecu)
 {
 	if (hrtimer_cancel(&ecu->ac_timer))
+		/* Drop the ref from j1939_ecu_timer_start(). */
 		j1939_ecu_put(ecu);
 }
 
@@ -137,9 +170,7 @@ static enum hrtimer_restart j1939_ecu_timer_handler(struct hrtimer *hrtimer)
 	 */
 	j1939_ecu_map_locked(ecu);
 
-	/* The corresponding j1939_ecu_get() is in
-	 * j1939_ecu_timer_start().
-	 */
+	/* Drop the ref from j1939_ecu_timer_start(). */
 	j1939_ecu_put(ecu);
 	write_unlock_bh(&priv->lock);
 
@@ -189,8 +220,8 @@ struct j1939_ecu *j1939_ecu_get_by_addr_locked(struct j1939_priv *priv, u8 addr)
 		return NULL;
 
 	ecu = j1939_ecu_find_by_addr_locked(priv, addr);
-	if (ecu)
-		j1939_ecu_get(ecu);
+	if (ecu && !j1939_ecu_get(ecu))
+		ecu = NULL;
 
 	return ecu;
 }
@@ -215,7 +246,7 @@ static struct j1939_ecu *j1939_ecu_find_by_name_locked(struct j1939_priv *priv,
 	lockdep_assert_held(&priv->lock);
 
 	list_for_each_entry(ecu, &priv->ecus, list) {
-		if (ecu->name == name)
+		if (ecu->name == name && j1939_ecu_valid(ecu))
 			return ecu;
 	}
 
@@ -232,11 +263,12 @@ struct j1939_ecu *j1939_ecu_get_by_name_locked(struct j1939_priv *priv,
 	if (!name)
 		return NULL;
 
-	ecu = j1939_ecu_find_by_name_locked(priv, name);
-	if (ecu)
-		j1939_ecu_get(ecu);
+	list_for_each_entry(ecu, &priv->ecus, list) {
+		if (ecu->name == name && j1939_ecu_get(ecu))
+			return ecu;
+	}
 
-	return ecu;
+	return NULL;
 }
 
 struct j1939_ecu *j1939_ecu_get_by_name(struct j1939_priv *priv, name_t name)
diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h
index cf26352d1d8c..623aeb2e54e1 100644
--- a/net/can/j1939/j1939-priv.h
+++ b/net/can/j1939/j1939-priv.h
@@ -42,6 +42,8 @@ struct j1939_ecu {
 
 	/* count users, to help transport protocol decide for interaction */
 	int nusers;
+
+	struct work_struct release_work;
 };
 
 struct j1939_priv {
@@ -345,4 +347,6 @@ void j1939_session_timers_cancel(struct j1939_session *session);
 /* CAN protocol */
 extern const struct can_proto j1939_can_proto;
 
+extern struct workqueue_struct *j1939_ecu_wq;
+
 #endif /* _J1939_PRIV_H_ */
diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
index 5e5e6c228f22..22c272f8dfde 100644
--- a/net/can/j1939/main.c
+++ b/net/can/j1939/main.c
@@ -401,6 +401,12 @@ static __init int j1939_module_init(void)
 
 	pr_info("can: SAE J1939\n");
 
+	j1939_ecu_wq = alloc_workqueue("j1939_ecu", WQ_HIGHPRI | WQ_PERCPU, 0);
+	if (!j1939_ecu_wq) {
+		ret = -ENOMEM;
+		goto fail_wq;
+	}
+
 	ret = register_netdevice_notifier(&j1939_netdev_notifier);
 	if (ret)
 		goto fail_notifier;
@@ -416,6 +422,8 @@ static __init int j1939_module_init(void)
  fail_sk:
 	unregister_netdevice_notifier(&j1939_netdev_notifier);
  fail_notifier:
+	destroy_workqueue(j1939_ecu_wq);
+ fail_wq:
 	return ret;
 }
 
@@ -424,6 +432,11 @@ static __exit void j1939_module_exit(void)
 	can_proto_unregister(&j1939_can_proto);
 
 	unregister_netdevice_notifier(&j1939_netdev_notifier);
+
+	/* We need to make sure that __j1939_ecu_release_work() is
+	 * no longer running before unmapping memory for this module.
+	 */
+	destroy_workqueue(j1939_ecu_wq);
 }
 
 module_init(j1939_module_init);
-- 
2.56.0

[PATCH v12 2/3] can: j1939: fix possibility of hard-lockup/deadlock in j1939_ecu_timer_cancel()

[Tetsuo Handa]

Since j1939_ecu_timer_cancel() is calling hrtimer_cancel() with priv->lock
held for write while j1939_ecu_timer_handler() takes priv->lock, a hard-
lockup (if CONFIG_PREEMPT_RT=n && CONFIG_SMP=y) or a deadlock between
priv->lock and cpu_base->softirq_expiry_lock (if CONFIG_PREEMPT_RT=y) will
happen when hrtimer_cancel() calls hrtimer_cancel_wait_running().

The comment for "struct j1939_priv"->lock says that priv->lock is used for
protecting "struct list_head ecus" and "struct j1939_addr_ent ents[256]".
But j1939_ac_process() is using priv->lock for also serializing timer
management across multiple frames. This makes it impossible to temporarily
drop priv->lock in order to safely call hrtimer_cancel().

Therefore, fix this problem by introducing two bool flags that implement
logical cancellation (negotiate hrtimer_try_to_cancel() == -1 condition).

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Cc: stable@vger.kernel.org
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 net/can/j1939/bus.c        | 25 +++++++++++++++++++++++--
 net/can/j1939/j1939-priv.h |  4 ++++
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
index c49c96dea796..8cbf8716d286 100644
--- a/net/can/j1939/bus.c
+++ b/net/can/j1939/bus.c
@@ -147,15 +147,25 @@ void j1939_ecu_timer_start(struct j1939_ecu *ecu)
 		return;
 
 	/* Schedule timer in 250 msec to commit address change. */
+	ecu->ac_commit_done = false;
 	hrtimer_start(&ecu->ac_timer, ms_to_ktime(250),
 		      HRTIMER_MODE_REL_SOFT);
 }
 
 void j1939_ecu_timer_cancel(struct j1939_ecu *ecu)
 {
-	if (hrtimer_cancel(&ecu->ac_timer))
+	switch (hrtimer_try_to_cancel(&ecu->ac_timer)) {
+	case 0:	/* Not active or already finished. */
+		break;
+	case 1:	/* Successfully canceled. */
 		/* Drop the ref from j1939_ecu_timer_start(). */
 		j1939_ecu_put(ecu);
+		break;
+	default: /* Currently executing. */
+		/* Tell j1939_ecu_timer_handler() to abort unless committed. */
+		if (!ecu->ac_commit_done)
+			ecu->ac_want_abort = true;
+	}
 }
 
 static enum hrtimer_restart j1939_ecu_timer_handler(struct hrtimer *hrtimer)
@@ -165,14 +175,25 @@ static enum hrtimer_restart j1939_ecu_timer_handler(struct hrtimer *hrtimer)
 	struct j1939_priv *priv = ecu->priv;
 
 	write_lock_bh(&priv->lock);
+
+	/* Abort if j1939_ecu_timer_cancel() told me to abort. */
+	if (ecu->ac_want_abort) {
+		ecu->ac_want_abort = false;
+		goto out;
+	}
+
 	/* TODO: can we test if ecu->addr is unicast before starting
 	 * the timer?
 	 */
 	j1939_ecu_map_locked(ecu);
 
+	/* Commit and tell j1939_ecu_timer_cancel() not to ask me to abort. */
+	ecu->ac_commit_done = true;
+out:
+	write_unlock_bh(&priv->lock);
+
 	/* Drop the ref from j1939_ecu_timer_start(). */
 	j1939_ecu_put(ecu);
-	write_unlock_bh(&priv->lock);
 
 	return HRTIMER_NORESTART;
 }
diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h
index 623aeb2e54e1..d50b6e21d688 100644
--- a/net/can/j1939/j1939-priv.h
+++ b/net/can/j1939/j1939-priv.h
@@ -43,6 +43,10 @@ struct j1939_ecu {
 	/* count users, to help transport protocol decide for interaction */
 	int nusers;
 
+	/* protected by priv->lock */
+	bool ac_want_abort;
+	bool ac_commit_done;
+
 	struct work_struct release_work;
 };
 
-- 
2.56.0

[PATCH v12 3/3] can: j1939: fix "struct j1939_ecu" refcount leak

[Tetsuo Handa]

syzbot is reporting "struct j1939_ecu" refcount leak, for

  j1939_ecu_get(ecu);
  priv->ents[ecu->addr] = ecu;

in j1939_ecu_map_locked() from j1939_ecu_timer_handler() can succeed
even after

  priv->ents[ecu->addr] = NULL;
  j1939_ecu_put(ecu);

in j1939_ecu_unmap_locked() from j1939_ecu_unmap_all() from
__j1939_rx_release() from j1939_netdev_stop() has completed.

Fix this problem by

  (1) canceling ecu->ac_timer from j1939_ecu_unmap_all()

unconditionally and

  (2) preventing ecu->ac_timer from re-arming in j1939_ecu_timer_start()
  (3) making j1939_ecu_map_locked() no-op

when a netdev which is associated with j1939_priv is no longer registered
(indicated by ecu->priv->ndev->reg_state != NETREG_REGISTERED) or
__j1939_rx_release() was called (indicated by ecu->priv->rx_kref == 0).

Reported-by: syzbot+e2af46126e0644cbebdd@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=e2af46126e0644cbebdd
Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Cc: stable@vger.kernel.org
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 net/can/j1939/bus.c    | 52 +++++++++++++++++++++++++++++++++++++++---
 net/can/j1939/socket.c | 10 +++++++-
 2 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
index 8cbf8716d286..a0120567bb8f 100644
--- a/net/can/j1939/bus.c
+++ b/net/can/j1939/bus.c
@@ -58,6 +58,17 @@ static bool __must_check j1939_ecu_get(struct j1939_ecu *ecu)
 	return kref_get_unless_zero(&ecu->kref);
 }
 
+static bool __must_check j1939_ecu_verify_get(struct j1939_ecu *ecu)
+{
+	struct j1939_priv *priv = ecu->priv;
+
+	/* Don't get an ECU ref if netdev is no longer registered or
+	 * __j1939_rx_release() was called or __j1939_ecu_release() was called.
+	 */
+	return READ_ONCE(priv->ndev->reg_state) == NETREG_REGISTERED &&
+		kref_read(&priv->rx_kref) && j1939_ecu_get(ecu);
+}
+
 static bool __must_check j1939_ecu_valid(struct j1939_ecu *ecu)
 {
 	/* Treat an ECU as invalid if __j1939_ecu_release() was called. */
@@ -85,7 +96,7 @@ static void j1939_ecu_map_locked(struct j1939_ecu *ecu)
 	if (!j1939_address_is_unicast(ecu->addr))
 		return;
 
-	if (!j1939_ecu_get(ecu))
+	if (!j1939_ecu_verify_get(ecu))
 		return;
 
 	ent = &priv->ents[ecu->addr];
@@ -130,8 +141,43 @@ void j1939_ecu_unmap(struct j1939_ecu *ecu)
 void j1939_ecu_unmap_all(struct j1939_priv *priv)
 {
 	int i;
+	struct j1939_ecu *ecu;
 
+rescan:
 	write_lock_bh(&priv->lock);
+	/* Need to shutdown timers now, for j1939_module_exit() might be called
+	 * relatively shortly after returning from j1939_ecu_unmap_all() from
+	 * __j1939_rx_release() from j1939_netdev_stop() from j1939_sk_release().
+	 */
+	list_for_each_entry(ecu, &priv->ecus, list) {
+		if (!j1939_ecu_valid(ecu))
+			continue;
+		switch (hrtimer_try_to_cancel(&ecu->ac_timer)) {
+		case 0:	/* Not active or already finished. */
+			continue;
+		case 1:	/* Successfully canceled. */
+			/* Drop the ref from j1939_ecu_timer_start(). */
+			j1939_ecu_put(ecu);
+			continue;
+		default: /* Currently executing. */
+			/* Get a ref, in order to call hrtimer_cancel() without priv->lock.
+			 * If we can't get a ref, __j1939_ecu_release_work() will call
+			 * hrtimer_cancel() without priv->lock, and j1939_module_exit()
+			 * will wait for completion of __j1939_ecu_release_work().
+			 */
+			if (!j1939_ecu_get(ecu))
+				continue;
+			break;
+		}
+		write_unlock_bh(&priv->lock);
+		if (hrtimer_cancel(&ecu->ac_timer))
+			/* Drop the ref from j1939_ecu_timer_start(). */
+			j1939_ecu_put(ecu);
+		/* Drop the ref from default: above. */
+		j1939_ecu_put(ecu);
+		goto rescan;
+	}
+	/* Unmap any remaining mapped ECUs */
 	for (i = 0; i < ARRAY_SIZE(priv->ents); i++)
 		if (priv->ents[i].ecu)
 			j1939_ecu_unmap_locked(priv->ents[i].ecu);
@@ -141,9 +187,9 @@ void j1939_ecu_unmap_all(struct j1939_priv *priv)
 void j1939_ecu_timer_start(struct j1939_ecu *ecu)
 {
 	/* The corresponding j1939_ecu_put() is in
-	 * j1939_ecu_timer_handler() or j1939_ecu_timer_cancel().
+	 * j1939_ecu_timer_handler() or j1939_ecu_timer_cancel() or j1939_ecu_unmap_all().
 	 */
-	if (!j1939_ecu_get(ecu))
+	if (!j1939_ecu_verify_get(ecu))
 		return;
 
 	/* Schedule timer in 250 msec to commit address change. */
diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
index 50a598ef5fd4..6e22ec5e88f5 100644
--- a/net/can/j1939/socket.c
+++ b/net/can/j1939/socket.c
@@ -484,7 +484,15 @@ static int j1939_sk_bind(struct socket *sock, struct sockaddr_unsized *uaddr, in
 			goto out_release_sock;
 		}
 
-		if (ndev->reg_state != NETREG_REGISTERED) {
+		/* Do nothing and fail early if netdev is no longer registered.
+		 * This check is racy but we don't need to recheck after j1939_jsk_add()
+		 * is called, for either j1939_sk_netdev_event_unregister() from
+		 * j1939_netdev_notify(NETDEV_UNREGISTER) (which is retried by
+		 * netdev_wait_allrefs_any() as long as priv->rx_kref held by
+		 * j1939_netdev_start() is preventing j1939_priv_set(priv->ndev, NULL)
+		 * from being called) or j1939_sk_release() will take care of this race.
+		 */
+		if (READ_ONCE(ndev->reg_state) != NETREG_REGISTERED) {
 			dev_put(ndev);
 			ret = -ENODEV;
 			goto out_release_sock;
-- 
2.56.0

Re: [PATCH v12 1/3] can: j1939: fix possibility of UAF in __j1939_ecu_release()

[Marc Kleine-Budde]

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

On 14.11.2026 02:13:05, Tetsuo Handa wrote:
> Since j1939_ac_recv() and j1939_ac_fixup() are calling j1939_ecu_put()
> without priv->lock held for write, list corruption and/or UAF might happen
> if the last user called j1939_ecu_put() without priv->lock held for write.

Your series should include a cover letter. It should provide a brief,
informative overview of the series. In addition, a brief, concise change
log in the form of a bulleted list should be included in the cover
letter.

All patches should be a reply to the cover letter.

Consider using "b4" [1] to send the patches, there is even a web-hook to
avoid local smtp servers.

[1] https://b4.docs.kernel.org/en/latest/

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v12 2/3] can: j1939: fix possibility of hard-lockup/deadlock in j1939_ecu_timer_cancel()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] can: j1939: ecu ref leak and uncanceled timer in
  j1939_ecu_timer_cancel()
- [Medium] can: j1939: stale ac_want_abort on abort path in
  j1939_ecu_timer_handler()
--

commit fa21532ae1ca00bb699863f7fb0ec168d518ddc4
Author: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

can: j1939: fix possibility of hard-lockup/deadlock in j1939_ecu_timer_cancel()

This commit replaces hrtimer_cancel() in j1939_ecu_timer_cancel() with
hrtimer_try_to_cancel() and two boolean flags on struct j1939_ecu to
avoid waiting for j1939_ecu_timer_handler() while holding priv->lock.

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index c49c96dea7960..8cbf8716d286e 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
> @@ -147,15 +147,25 @@ void j1939_ecu_timer_start(struct j1939_ecu *ecu)
>  		return;
>  
>  	/* Schedule timer in 250 msec to commit address change. */
> +	ecu->ac_commit_done = false;
>  	hrtimer_start(&ecu->ac_timer, ms_to_ktime(250),
>  		      HRTIMER_MODE_REL_SOFT);
>  }
>  
>  void j1939_ecu_timer_cancel(struct j1939_ecu *ecu)
>  {
> -	if (hrtimer_cancel(&ecu->ac_timer))
> +	switch (hrtimer_try_to_cancel(&ecu->ac_timer)) {
> +	case 0:	/* Not active or already finished. */
> +		break;
> +	case 1:	/* Successfully canceled. */
>  		/* Drop the ref from j1939_ecu_timer_start(). */
>  		j1939_ecu_put(ecu);
> +		break;
> +	default: /* Currently executing. */
> +		/* Tell j1939_ecu_timer_handler() to abort unless committed. */
> +		if (!ecu->ac_commit_done)
> +			ecu->ac_want_abort = true;
> +	}
>  }

[Severity: High]
Can j1939_ecu_timer_cancel() fail to dequeue ecu->ac_timer and leak an
ecu->kref reference when two address-claim frames arrive while a previous
j1939_ecu_timer_handler() callback is still running?

In __run_hrtimer(), base->running remains set to &ecu->ac_timer from
before j1939_ecu_timer_handler() enters write_lock_bh(&priv->lock) until
after it releases priv->lock and returns.  Whenever
hrtimer_callback_running(&ecu->ac_timer) is true, hrtimer_try_to_cancel()
returns -1 without calling remove_hrtimer(), even if ecu->ac_timer was
re-enqueued in the meantime:

CPU0                              CPU1
__run_hrtimer()
  base->running = &ecu->ac_timer
  j1939_ecu_timer_handler()
                                  j1939_ac_process() [frame 1, SA1]
                                    write_lock_bh(&priv->lock)
                                    j1939_ecu_timer_cancel(ecu)
                                      hrtimer_try_to_cancel() -> -1
                                    j1939_ecu_timer_start(ecu)
                                      j1939_ecu_get(ecu)
                                      hrtimer_start(&ecu->ac_timer, ...)
                                    write_unlock_bh(&priv->lock)

                                  j1939_ac_process() [frame 2, SA2]
                                    write_lock_bh(&priv->lock)
                                    j1939_ecu_timer_cancel(ecu)
                                      hrtimer_try_to_cancel() -> -1
                                        <-- stays queued, no put
                                    j1939_ecu_timer_start(ecu)
                                      j1939_ecu_get(ecu)
                                      hrtimer_start(&ecu->ac_timer, ...)
                                    write_unlock_bh(&priv->lock)

Frame 2's j1939_ecu_timer_start() overwrites the already-queued timer in
place while taking another reference, so only one future callback runs and
frame 1's ecu->kref reference is leaked (along with j1939_priv and the
netdev reference on priv->ndev).

If frame 2 instead carries J1939_IDLE_ADDR or loses address contention and
does not call j1939_ecu_timer_start(), frame 1's timer is never dequeued;
once CPU0 acquires priv->lock and clears ecu->ac_want_abort, frame 1's
timer fires 250 ms later and maps the canceled address.  Conversely, if
CPU0 had already passed write_unlock_bh(&priv->lock) when frames 1 and 2
ran, j1939_ecu_timer_start() does not clear ecu->ac_want_abort, so frame
2's newly started timer aborts when it expires.

> @@ -165,14 +175,25 @@ static enum hrtimer_restart j1939_ecu_timer_handler(struct hrtimer *hrtimer)
>  	struct j1939_priv *priv = ecu->priv;
>  
>  	write_lock_bh(&priv->lock);
> +
> +	/* Abort if j1939_ecu_timer_cancel() told me to abort. */
> +	if (ecu->ac_want_abort) {
> +		ecu->ac_want_abort = false;
> +		goto out;
> +	}
> +
>  	/* TODO: can we test if ecu->addr is unicast before starting
>  	 * the timer?
>  	 */
>  	j1939_ecu_map_locked(ecu);
>  
> +	/* Commit and tell j1939_ecu_timer_cancel() not to ask me to abort. */
> +	ecu->ac_commit_done = true;
> +out:
> +	write_unlock_bh(&priv->lock);
> +
>  	/* Drop the ref from j1939_ecu_timer_start(). */
>  	j1939_ecu_put(ecu);
> -	write_unlock_bh(&priv->lock);
>  
>  	return HRTIMER_NORESTART;
>  }

[Severity: Medium]
Does jumping to out leave ecu->ac_commit_done false after an aborting
handler drops priv->lock, causing the next timer to abort as well?

CPU0                              CPU1
j1939_ecu_timer_handler()
  write_lock_bh(&priv->lock)
  if (ecu->ac_want_abort) {
    ecu->ac_want_abort = false;
    goto out;
  }
  write_unlock_bh(&priv->lock)
                                  j1939_ac_process()
                                    write_lock_bh(&priv->lock)
                                    j1939_ecu_timer_cancel(ecu)
                                      hrtimer_try_to_cancel() -> -1
                                      if (!ecu->ac_commit_done)
                                        ecu->ac_want_abort = true;
                                    j1939_ecu_timer_start(ecu)
                                      ecu->ac_commit_done = false;
                                      hrtimer_start(&ecu->ac_timer, ...)
                                        <-- ac_want_abort stays true
                                    write_unlock_bh(&priv->lock)
  j1939_ecu_put(ecu)
__run_hrtimer()
  base->running = NULL

When an earlier j1939_ecu_timer_cancel() aborts the running handler (for
example on J1939_IDLE_ADDR without restarting the timer), the handler
unlocks priv->lock with ecu->ac_commit_done still false.  If a new unicast
address claim arrives before __run_hrtimer() clears base->running,
j1939_ecu_timer_cancel() sees hrtimer_try_to_cancel() == -1 and
!ecu->ac_commit_done, setting ecu->ac_want_abort = true.  Because the
old handler has already passed write_unlock_bh(&priv->lock) and
j1939_ecu_timer_start() does not clear ecu->ac_want_abort, the new 250 ms
timer will see ecu->ac_want_abort == true and abort without mapping the
ECU.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/8de287d7-b4e1-4062-b40e-b193f5ec68ca@I-love.SAKURA.ne.jp?part=2

Re: [PATCH v12 3/3] can: j1939: fix "struct j1939_ecu" refcount leak

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] can: j1939: ecu refcount leak in j1939_ecu_timer_start() when
  handler runs
- [Medium] can: j1939: stale ac_want_abort in j1939_ecu_timer_handler()
  aborts claim
- [Medium] can: j1939: race in j1939_ecu_unmap_all() when ecu kref drops
  to zero
--

commit 9ea6a93bf539408118e87c073de50599bff887f6
Author: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

can: j1939: fix "struct j1939_ecu" refcount leak

This commit cancels pending ecu->ac_timer instances in
j1939_ecu_unmap_all() and adds j1939_ecu_verify_get() so that
j1939_ecu_map_locked() and j1939_ecu_timer_start() do not take new ECU
references after netdev unregistration or __j1939_rx_release().

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index 8cbf8716d286e..a0120567bb8f5 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
[ ... ]
> @@ -130,8 +141,43 @@ void j1939_ecu_unmap(struct j1939_ecu *ecu)
>  void j1939_ecu_unmap_all(struct j1939_priv *priv)
>  {
>  	int i;
> +	struct j1939_ecu *ecu;
>  
> +rescan:
>  	write_lock_bh(&priv->lock);
> +	/* Need to shutdown timers now, for j1939_module_exit() might be called
> +	 * relatively shortly after returning from j1939_ecu_unmap_all() from
> +	 * __j1939_rx_release() from j1939_netdev_stop() from j1939_sk_release().
> +	 */
> +	list_for_each_entry(ecu, &priv->ecus, list) {
> +		if (!j1939_ecu_valid(ecu))
> +			continue;
> +		switch (hrtimer_try_to_cancel(&ecu->ac_timer)) {
> +		case 0:	/* Not active or already finished. */
> +			continue;
> +		case 1:	/* Successfully canceled. */
> +			/* Drop the ref from j1939_ecu_timer_start(). */
> +			j1939_ecu_put(ecu);
> +			continue;
> +		default: /* Currently executing. */
> +			/* Get a ref, in order to call hrtimer_cancel() without priv->lock.
> +			 * If we can't get a ref, __j1939_ecu_release_work() will call
> +			 * hrtimer_cancel() without priv->lock, and j1939_module_exit()
> +			 * will wait for completion of __j1939_ecu_release_work().
> +			 */
> +			if (!j1939_ecu_get(ecu))
> +				continue;
> +			break;
> +		}

[Severity: Medium]
Can j1939_ecu_unmap_all() return before __j1939_ecu_release() queues
ecu->release_work on j1939_ecu_wq?

In j1939_ecu_timer_handler(), j1939_ecu_put(ecu) is called after
write_unlock_bh(&priv->lock).  When that drops the final reference,
kref_put() decrements ecu->kref to 0 before __j1939_ecu_release() calls
queue_work(j1939_ecu_wq, &ecu->release_work):

CPU0                              CPU1
j1939_ecu_timer_handler()
  write_unlock_bh(&priv->lock)
  j1939_ecu_put(ecu)
    refcount_dec_and_test() -> 0
                                  j1939_ecu_unmap_all()
                                    write_lock_bh(&priv->lock)
                                    !j1939_ecu_valid(ecu) -> skips ecu
                                    write_unlock_bh(&priv->lock)
                                  j1939_module_exit()
                                    destroy_workqueue(j1939_ecu_wq)
    __j1939_ecu_release()
      queue_work(j1939_ecu_wq, ...)  <-- use-after-free

Because j1939_ecu_valid(ecu) and j1939_ecu_get(ecu) both see ecu->kref == 0
before queue_work() runs, j1939_ecu_unmap_all() neither cancels
ecu->ac_timer nor waits for the running handler, and destroy_workqueue()
in j1939_module_exit() can complete while CPU0 is still in
__j1939_ecu_release() and j1939_ecu_timer_handler().

> +		write_unlock_bh(&priv->lock);
> +		if (hrtimer_cancel(&ecu->ac_timer))
> +			/* Drop the ref from j1939_ecu_timer_start(). */
> +			j1939_ecu_put(ecu);
> +		/* Drop the ref from default: above. */
> +		j1939_ecu_put(ecu);
> +		goto rescan;
> +	}
> +	/* Unmap any remaining mapped ECUs */
>  	for (i = 0; i < ARRAY_SIZE(priv->ents); i++)
>  		if (priv->ents[i].ecu)
>  			j1939_ecu_unmap_locked(priv->ents[i].ecu);
>  	write_unlock_bh(&priv->lock);
>  }
>  
>  void j1939_ecu_timer_start(struct j1939_ecu *ecu)
>  {
>  	/* The corresponding j1939_ecu_put() is in
> -	 * j1939_ecu_timer_handler() or j1939_ecu_timer_cancel().
> +	 * j1939_ecu_timer_handler() or j1939_ecu_timer_cancel() or j1939_ecu_unmap_all().
>  	 */
> -	if (!j1939_ecu_get(ecu))
> +	if (!j1939_ecu_verify_get(ecu))
>  		return;
>  
>  	/* Schedule timer in 250 msec to commit address change. */

[Severity: High]
Does j1939_ecu_timer_start() leak an ecu reference if two Address Claim
frames are processed while j1939_ecu_timer_handler() is still running on
another CPU?

In __run_hrtimer(), hrtimer_callback_running(&ecu->ac_timer) remains true
until after j1939_ecu_timer_handler() returns, and hrtimer_try_to_cancel()
always returns -1 while the callback is running even if the timer has
already been re-queued:

net/can/j1939/bus.c:j1939_ecu_timer_cancel() {
	switch (hrtimer_try_to_cancel(&ecu->ac_timer)) {
	case 0:	/* Not active or already finished. */
		break;
	case 1:	/* Successfully canceled. */
		/* Drop the ref from j1939_ecu_timer_start(). */
		j1939_ecu_put(ecu);
		break;
	default: /* Currently executing. */
		/* Tell j1939_ecu_timer_handler() to abort unless committed. */
		if (!ecu->ac_commit_done)
			ecu->ac_want_abort = true;
	}
}

If two Address Claim frames with changed unicast source addresses arrive on
CPU1 while j1939_ecu_timer_handler() is running on CPU0:

CPU0                              CPU1
j1939_ecu_timer_handler()
                                  j1939_ac_process() [frame 1]
                                    j1939_ecu_timer_cancel(ecu) -> -1
                                      ecu->ac_want_abort = true
                                    j1939_ecu_timer_start(ecu)
                                      j1939_ecu_verify_get(ecu)
                                      hrtimer_start(&ecu->ac_timer, ...)
                                  j1939_ac_process() [frame 2]
                                    j1939_ecu_timer_cancel(ecu) -> -1
                                      (does not put ref from frame 1)
                                    j1939_ecu_timer_start(ecu)
                                      j1939_ecu_verify_get(ecu)
                                      hrtimer_start(&ecu->ac_timer, ...)
                                        <-- overwrites queued timer,
                                            leaking frame 1's ecu ref

The leaked ecu->kref also pins priv->kref and ecu->priv_dev_tracker, which
prevents unregister_netdevice() from completing.  In addition, if frame 2
cancels the claim without restarting the timer (for example, with
J1939_IDLE_ADDR) while CPU0 is waiting on write_lock_bh(&priv->lock), CPU0
clears ecu->ac_want_abort = false and exits while the timer queued by
frame 1 stays armed and maps the canceled ECU 250 ms later.

[Severity: Medium]
Can a new address claim timer started by j1939_ecu_timer_start() leave a
stale ecu->ac_want_abort set and abort when it fires?

When j1939_ecu_timer_handler() aborts, it leaves ecu->ac_commit_done as
false and unlocks priv->lock before returning:

net/can/j1939/bus.c:j1939_ecu_timer_handler() {
...
	if (ecu->ac_want_abort) {
		ecu->ac_want_abort = false;
		goto out;
	}
...
	ecu->ac_commit_done = true;
out:
	write_unlock_bh(&priv->lock);

	/* Drop the ref from j1939_ecu_timer_start(). */
	j1939_ecu_put(ecu);
}

If j1939_ac_process() runs on another CPU after write_unlock_bh(&priv->lock)
but before __run_hrtimer() clears base->running:

CPU0                              CPU1
j1939_ecu_timer_handler()
  if (ecu->ac_want_abort)
    ecu->ac_want_abort = false
    goto out
  write_unlock_bh(&priv->lock)
                                  j1939_ac_process()
                                    j1939_ecu_timer_cancel(ecu) -> -1
                                      (!ecu->ac_commit_done is true)
                                      ecu->ac_want_abort = true
                                    j1939_ecu_timer_start(ecu)
                                      ecu->ac_commit_done = false
                                      hrtimer_start(&ecu->ac_timer, ...)
  j1939_ecu_put(ecu)
                                  (250 ms later)
                                  j1939_ecu_timer_handler()
                                    if (ecu->ac_want_abort)
                                      goto out  <-- aborts new claim

Because j1939_ecu_timer_start() does not clear ecu->ac_want_abort and CPU0
has already passed the check, the newly started timer aborts without
mapping the ECU.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/8de287d7-b4e1-4062-b40e-b193f5ec68ca@I-love.SAKURA.ne.jp?part=3

Re: [PATCH v12 1/3] can: j1939: fix possibility of UAF in __j1939_ecu_release()

[Tetsuo Handa]

On 2026/11/14 2:21, Marc Kleine-Budde wrote:
> On 14.11.2026 02:13:05, Tetsuo Handa wrote:
>> Since j1939_ac_recv() and j1939_ac_fixup() are calling j1939_ecu_put()
>> without priv->lock held for write, list corruption and/or UAF might happen
>> if the last user called j1939_ecu_put() without priv->lock held for write.
> 
> Your series should include a cover letter. It should provide a brief,
> informative overview of the series. In addition, a brief, concise change
> log in the form of a bulleted list should be included in the cover
> letter.
> 
> All patches should be a reply to the cover letter.
> 
> Consider using "b4" [1] to send the patches, there is even a web-hook to
> avoid local smtp servers.
> 
> [1] https://b4.docs.kernel.org/en/latest/
> 
> Marc
> 

The reason this series is growing is that AI reviewers are not happy
unless we fix the deadlock in j1939_ecu_timer_cancel() at the same time.

Can you take patches incrementally? At least, I consider that
"can: j1939: fix possibility of UAF in __j1939_ecu_release()" is
ready to apply.

By the way, I don't plan to use b4, for my spam mail filter drops
mails sent from me.

Re: [PATCH v12 1/3] can: j1939: fix possibility of UAF in __j1939_ecu_release()

[Marc Kleine-Budde]

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

On 14.11.2026 02:44:31, Tetsuo Handa wrote:
> By the way, I don't plan to use b4, for my spam mail filter drops
> mails sent from me.

You have several options to send mail via b4 [1]:

a)
You can deliver with authenticated SMTP.

b)
If you use the web-hook, the emails will be send from:

| "Tetsuo Handa via B4 Relay <devnull+penguin-kernel.I-love.SAKURA.ne.jp@kernel.org>"

not by "you".

regards,
Marc

[1] https://b4.docs.kernel.org/en/latest/contributor/send.html

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]