[PATCH v13 0/6] can: j1939: make module more robust

[PATCH v13 0/6] can: j1939: make module more robust

[Tetsuo Handa]

This series is intended to address the following problems.

## Problem 1: Possible UAF on module unload.

Since module refcount is currently associated with lifecycle of j1939
sockets, we need to make sure that all resources associated with socket
("struct j1939_priv" and "struct j1939_ecu") are synchronously destructed
before socket is destructed. Otherwise, j1939_module_exit() can complete
and memory for this module is unmapped before j1939_ecu_timer_handler() or
__j1939_ecu_release() or __j1939_priv_release() completes (i.e. causing
UAF).

Calling module_put(THIS_MODULE) from a module function which is not
possible to wait for completion of that function from that module's exit
function is a source of UAF. But calling module_put(THIS_MODULE) from a WQ
function which is possible to wait for completion from that module's exit
function using flush_workqueue() etc. is fine.

Fix problem 1 by deferring release steps of "struct j1939_priv" to WQ
context where we can call module_put(THIS_MODULE) while allowing
j1939_module_exit() to wait for completion using destroy_workqueue().

By this change, we no longer need to make sure that all resources
associated with socket ("struct j1939_priv" and "struct j1939_ecu") are
synchronously destructed before socket is destructed, simplifying
further fixes.

## Problem 2: Possible UAF in __j1939_priv_release().

If a user calls j1939_ecu_put(ecu) with priv->lock held but without knowing
that this "struct j1939_ecu" is the last user of a "struct j1939_priv",
__j1939_priv_release_work() via __j1939_priv_release() from
j1939_priv_put() from __j1939_ecu_release() might call kfree(priv)
before priv->lock is released (i.e. causing UAF).

Fix problem 2 by adding a safeguard that delays kfree() in
__j1939_priv_release_work() until there is no priv->lock holder.

## Problem 3: Possible list corruption in __j1939_ecu_release().

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

Instead of requiring all j1939_ecu_put() callers to hold priv->lock for
write, fix problem 3 by deferring release steps of "struct j1939_ecu"
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).

## Problem 4: Possible UAF in j1939_ecu_timer_handler().

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() (i.e. causing UAF).

Fix problem 4 by adding a safeguard that explicitly cancels ecu->ac_timer
before kfree(). The fix for problem 3 makes it possible to call
hrtimer_cancel() from __j1939_ecu_release_work().

## Problem 5: Possible "struct j1939_ecu" refcount leak.

Since

  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,
"struct j1939_ecu" refcount leaks.

Fix problem 5 by

  (1) preventing ecu->ac_timer from re-arming in j1939_ecu_timer_start()
  (2) 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).

## Problem 6: Possible hard-lockup/deadlock in j1939_ecu_timer_cancel().

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 problem 6 by introducing two bool flags that implement
logical cancellation (negotiate hrtimer_try_to_cancel() == -1 condition)
and dropping address claim frames during logical cancellation.



[PATCH v13 1/6] can: j1939: fix possibility of UAF on module unload
[PATCH v13 2/6] can: j1939: add a safeguard to __j1939_priv_release_work()
[PATCH v13 3/6] can: j1939: fix possibility of list corruption in __j1939_ecu_release()
[PATCH v13 4/6] can: j1939: add a safeguard to __j1939_ecu_release_work()
[PATCH v13 5/6] can: j1939: fix "struct j1939_ecu" refcount leak
[PATCH v13 6/6] can: j1939: fix possibility of hard-lockup/deadlock in j1939_ecu_timer_cancel()
---
Changes since v12:
- Added a cover letter and split the series into 6 logical patches (1 patch per problem).
- Deferred __j1939_priv_release() to WQ context to simplify "struct j1939_ecu" refcount leak fix
- Fixed findings by sashiko in v12.

 net/can/j1939/address-claim.c |    7 +++-
 net/can/j1939/bus.c           |  121 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------
 net/can/j1939/j1939-priv.h    |   12 +++++++-
 net/can/j1939/main.c          |   75 ++++++++++++++++++++++++++++++++++++++++++++------
 net/can/j1939/socket.c        |   10 ++++++
 5 files changed, 189 insertions(+), 36 deletions(-)

[PATCH v13 1/6] can: j1939: fix possibility of UAF on module unload

[Tetsuo Handa]

Since module refcount is currently associated with lifecycle of j1939
sockets, we need to make sure that all resources associated with socket
("struct j1939_priv" and "struct j1939_ecu") are synchronously destructed
before socket is destructed. Otherwise, j1939_module_exit() can complete
and memory for this module is unmapped before j1939_ecu_timer_handler() or
__j1939_ecu_release() or __j1939_priv_release() completes (i.e. causing
UAF).

Calling module_put(THIS_MODULE) from a module function which is not
possible to wait for completion of that function from that module's exit
function is a source of UAF. But calling module_put(THIS_MODULE) from a WQ
function which is possible to wait for completion from that module's exit
function using flush_workqueue() etc. is fine.

Fix this problem by deferring release steps of "struct j1939_priv" to WQ
context where we can call module_put(THIS_MODULE) while allowing
j1939_module_exit() to wait for completion using destroy_workqueue().

By this change, we no longer need to make sure that all resources
associated with socket ("struct j1939_priv" and "struct j1939_ecu") are
synchronously destructed before socket is destructed, simplifying
further fixes.

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/j1939-priv.h |  4 ++++
 net/can/j1939/main.c       | 47 ++++++++++++++++++++++++++++++++------
 2 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h
index cf26352d1d8c..ff90683f33f4 100644
--- a/net/can/j1939/j1939-priv.h
+++ b/net/can/j1939/j1939-priv.h
@@ -93,6 +93,8 @@ struct j1939_priv {
 
 	struct kref rx_kref;
 	u32 rx_tskey;
+
+	struct work_struct release_work;
 };
 
 void j1939_ecu_put(struct j1939_ecu *ecu);
@@ -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_wq;
+
 #endif /* _J1939_PRIV_H_ */
diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
index 5e5e6c228f22..bfe3e6ada0a0 100644
--- a/net/can/j1939/main.c
+++ b/net/can/j1939/main.c
@@ -20,6 +20,8 @@
 
 #include "j1939-priv.h"
 
+struct workqueue_struct *j1939_wq __ro_after_init;
+
 MODULE_DESCRIPTION("PF_CAN SAE J1939");
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("EIA Electronics (Kurt Van Dijck & Pieter Beyens)");
@@ -132,6 +134,11 @@ static struct j1939_priv *j1939_priv_create(struct net_device *ndev)
 	if (!priv)
 		return NULL;
 
+	if (!try_module_get(THIS_MODULE)) {
+		kfree(priv);
+		return NULL;
+	}
+
 	rwlock_init(&priv->lock);
 	INIT_LIST_HEAD(&priv->ecus);
 	priv->ndev = ndev;
@@ -152,9 +159,9 @@ static inline void j1939_priv_set(struct net_device *ndev,
 	can_ml->j1939_priv = priv;
 }
 
-static void __j1939_priv_release(struct kref *kref)
+static void __j1939_priv_release_work(struct work_struct *work)
 {
-	struct j1939_priv *priv = container_of(kref, struct j1939_priv, kref);
+	struct j1939_priv *priv = container_of(work, struct j1939_priv, release_work);
 	struct net_device *ndev = priv->ndev;
 
 	netdev_dbg(priv->ndev, "%s: 0x%p\n", __func__, priv);
@@ -165,6 +172,22 @@ static void __j1939_priv_release(struct kref *kref)
 
 	netdev_put(ndev, &priv->dev_tracker);
 	kfree(priv);
+	/* This is safe, for j1939_module_exit() waits for completion of
+	 * this callback using destroy_workqueue().
+	 */
+	module_put(THIS_MODULE);
+}
+
+static void __j1939_priv_release(struct kref *kref)
+{
+	struct j1939_priv *priv = container_of(kref, struct j1939_priv, kref);
+
+	/* Since synchronously dropping module refcount of self is not safe,
+	 * defer actual release to WQ context.
+	 */
+	INIT_WORK(&priv->release_work, __j1939_priv_release_work);
+	queue_work(j1939_wq, &priv->release_work);
+
 }
 
 void j1939_priv_put(struct j1939_priv *priv)
@@ -281,8 +304,7 @@ struct j1939_priv *j1939_netdev_start(struct net_device *ndev)
 		 */
 		kref_get(&priv_new->rx_kref);
 		mutex_unlock(&j1939_netdev_lock);
-		netdev_put(ndev, &priv->dev_tracker);
-		kfree(priv);
+		j1939_priv_put(priv);
 		return priv_new;
 	}
 	j1939_priv_set(ndev, priv);
@@ -297,9 +319,7 @@ struct j1939_priv *j1939_netdev_start(struct net_device *ndev)
  out_priv_put:
 	j1939_priv_set(ndev, NULL);
 	mutex_unlock(&j1939_netdev_lock);
-
-	netdev_put(ndev, &priv->dev_tracker);
-	kfree(priv);
+	j1939_priv_put(priv);
 
 	return ERR_PTR(ret);
 }
@@ -401,6 +421,12 @@ static __init int j1939_module_init(void)
 
 	pr_info("can: SAE J1939\n");
 
+	j1939_wq = alloc_workqueue("j1939_wq", WQ_HIGHPRI | WQ_PERCPU, 0);
+	if (!j1939_wq) {
+		ret = -ENOMEM;
+		goto fail_wq;
+	}
+
 	ret = register_netdevice_notifier(&j1939_netdev_notifier);
 	if (ret)
 		goto fail_notifier;
@@ -416,6 +442,8 @@ static __init int j1939_module_init(void)
  fail_sk:
 	unregister_netdevice_notifier(&j1939_netdev_notifier);
  fail_notifier:
+	destroy_workqueue(j1939_wq);
+ fail_wq:
 	return ret;
 }
 
@@ -424,6 +452,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_priv_release_work() is
+	 * no longer running before unmapping memory for this module.
+	 */
+	destroy_workqueue(j1939_wq);
 }
 
 module_init(j1939_module_init);
-- 
2.56.0

[PATCH v13 2/6] can: j1939: add a safeguard to __j1939_priv_release_work()

[Tetsuo Handa]

If a user calls j1939_ecu_put(ecu) with priv->lock held but without knowing
that this "struct j1939_ecu" is the last user of a "struct j1939_priv",
__j1939_priv_release_work() via __j1939_priv_release() from
j1939_priv_put() from __j1939_ecu_release() might call kfree(priv)
before priv->lock is released (i.e. causing UAF).

Add a safeguard that delays kfree() in __j1939_priv_release_work() until
there is no priv->lock holder.

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/main.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
index bfe3e6ada0a0..a28216021020 100644
--- a/net/can/j1939/main.c
+++ b/net/can/j1939/main.c
@@ -164,6 +164,29 @@ static void __j1939_priv_release_work(struct work_struct *work)
 	struct j1939_priv *priv = container_of(work, struct j1939_priv, release_work);
 	struct net_device *ndev = priv->ndev;
 
+	/* Wait for priv->lock owner to release the lock.
+	 * This is a protection for
+	 *
+	 *   (1) Read priv from e.g. ecu->priv.
+	 *
+	 *   (2) Take priv->lock.
+	 *
+	 *   (3) A user who unexpectedly became the last user of "struct j1939_priv"
+	 *       indirectly calls j1939_priv_put() via e.g. j1939_ecu_put(ecu).
+	 *
+	 *   (4) j1939_priv_put() calls __j1939_priv_release() because it was the last
+	 *       user.
+	 *
+	 *   (5) __j1939_priv_release_work() which was scheduled via
+	 *       __j1939_priv_release() starts and calls kfree(priv).
+	 *
+	 *   (6) Releases priv->lock, but this is UAF if (5) already called kfree().
+	 *
+	 * pattern.
+	 */
+	write_lock_bh(&priv->lock);
+	write_unlock_bh(&priv->lock);
+
 	netdev_dbg(priv->ndev, "%s: 0x%p\n", __func__, priv);
 
 	WARN_ON_ONCE(!list_empty(&priv->active_session_list));
-- 
2.56.0

[PATCH v13 3/6] can: j1939: fix possibility of list corruption 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 might happen if the
last user called j1939_ecu_put() without priv->lock held for write.

Instead of requiring all j1939_ecu_put() callers to hold priv->lock for
write, fix this problem by deferring release steps of "struct j1939_ecu"
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).

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        | 61 +++++++++++++++++++++++++++-----------
 net/can/j1939/j1939-priv.h |  2 ++
 net/can/j1939/main.c       |  5 ++--
 3 files changed, 48 insertions(+), 20 deletions(-)

diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
index cdc3c0a71937..0aeeee1491e4 100644
--- a/net/can/j1939/bus.c
+++ b/net/can/j1939/bus.c
@@ -14,25 +14,47 @@
 
 #include "j1939-priv.h"
 
-static void __j1939_ecu_release(struct kref *kref)
+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);
 	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_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 +78,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 +133,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 +147,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 +163,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 +213,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 +239,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 +256,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 ff90683f33f4..55ded221cd82 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 {
diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
index a28216021020..d5e06b1beff5 100644
--- a/net/can/j1939/main.c
+++ b/net/can/j1939/main.c
@@ -476,8 +476,9 @@ static __exit void j1939_module_exit(void)
 
 	unregister_netdevice_notifier(&j1939_netdev_notifier);
 
-	/* We need to make sure that __j1939_priv_release_work() is
-	 * no longer running before unmapping memory for this module.
+	/* We need to make sure that __j1939_priv_release_work() and
+	 * __j1939_ecu_release_work() are no longer running before
+	 * unmapping memory for this module.
 	 */
 	destroy_workqueue(j1939_wq);
 }
-- 
2.56.0

[PATCH v13 4/6] can: j1939: add a safeguard to __j1939_ecu_release_work()

[Tetsuo Handa]

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() (i.e. causing UAF).

Add a safeguard that explicitly cancels 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 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
index 0aeeee1491e4..3924404152a2 100644
--- a/net/can/j1939/bus.c
+++ b/net/can/j1939/bus.c
@@ -22,6 +22,11 @@ static void __j1939_ecu_release_work(struct work_struct *work)
 	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);
-- 
2.56.0

[PATCH v13 5/6] 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) preventing ecu->ac_timer from re-arming in j1939_ecu_timer_start()
  (2) 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    | 15 +++++++++++++--
 net/can/j1939/socket.c | 10 +++++++++-
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
index 3924404152a2..9b40f3754623 100644
--- a/net/can/j1939/bus.c
+++ b/net/can/j1939/bus.c
@@ -56,6 +56,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. */
@@ -83,7 +94,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];
@@ -141,7 +152,7 @@ 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().
 	 */
-	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

[PATCH v13 6/6] 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)
and dropping address claim frames during logical cancellation.

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/address-claim.c |  7 ++++--
 net/can/j1939/bus.c           | 40 ++++++++++++++++++++++++++++++++---
 net/can/j1939/j1939-priv.h    |  6 +++++-
 3 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/net/can/j1939/address-claim.c b/net/can/j1939/address-claim.c
index ca4ad6cdd5cb..e6c9fdeba9c7 100644
--- a/net/can/j1939/address-claim.c
+++ b/net/can/j1939/address-claim.c
@@ -211,8 +211,11 @@ static void j1939_ac_process(struct j1939_priv *priv, struct sk_buff *skb)
 	if (IS_ERR_OR_NULL(ecu))
 		goto out_unlock_bh;
 
-	/* cancel pending (previous) address claim */
-	j1939_ecu_timer_cancel(ecu);
+	/* Cancel pending (previous) address claim. If we can't cancel,
+	 * do nothing until pending (previous) address claim commits or aborts.
+	 */
+	if (!j1939_ecu_timer_cancel(ecu))
+		goto out_ecu_put;
 
 	if (j1939_address_is_idle(skcb->addr.sa)) {
 		j1939_ecu_unmap_locked(ecu);
diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
index 9b40f3754623..6520c91a6e1e 100644
--- a/net/can/j1939/bus.c
+++ b/net/can/j1939/bus.c
@@ -156,15 +156,35 @@ void j1939_ecu_timer_start(struct j1939_ecu *ecu)
 		return;
 
 	/* Schedule timer in 250 msec to commit address change. */
+	ecu->abort_ac_handler = false;
+	ecu->done_ac_handler = false;
 	hrtimer_start(&ecu->ac_timer, ms_to_ktime(250),
 		      HRTIMER_MODE_REL_SOFT);
 }
 
-void j1939_ecu_timer_cancel(struct j1939_ecu *ecu)
+bool 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. */
+		/* OK to call j1939_ecu_timer_start() again if
+		 * j1939_ecu_timer_handler() already committed or aborted.
+		 */
+		if (ecu->done_ac_handler)
+			break;
+		/* Tell j1939_ecu_timer_handler() to abort. */
+		ecu->abort_ac_handler = true;
+		/* Don't call j1939_ecu_timer_start() until
+		 * j1939_ecu_timer_handler() commits or aborts.
+		 */
+		return false;
+	}
+	return true;
 }
 
 static enum hrtimer_restart j1939_ecu_timer_handler(struct hrtimer *hrtimer)
@@ -174,14 +194,28 @@ 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->abort_ac_handler) {
+		ecu->abort_ac_handler = false;
+		goto out;
+	}
+
 	/* TODO: can we test if ecu->addr is unicast before starting
 	 * the timer?
 	 */
 	j1939_ecu_map_locked(ecu);
 
+out:
+	/* Tell j1939_ecu_timer_cancel() that this handler has finished
+	 * its critical section (either committed or aborted), so no need
+	 * to set abort_ac_handler again while __run_hrtimer() is finishing.
+	 */
+	ecu->done_ac_handler = true;
+	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 55ded221cd82..23f37e0b17bc 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 abort_ac_handler;
+	bool done_ac_handler;
+
 	struct work_struct release_work;
 };
 
@@ -207,7 +211,7 @@ void j1939_simple_recv(struct j1939_priv *priv, struct sk_buff *skb);
 struct j1939_ecu *j1939_ecu_create_locked(struct j1939_priv *priv, name_t name);
 
 void j1939_ecu_timer_start(struct j1939_ecu *ecu);
-void j1939_ecu_timer_cancel(struct j1939_ecu *ecu);
+bool __must_check j1939_ecu_timer_cancel(struct j1939_ecu *ecu);
 void j1939_ecu_unmap_all(struct j1939_priv *priv);
 
 struct j1939_priv *j1939_netdev_start(struct net_device *ndev);
-- 
2.56.0

Re: [PATCH v13 6/6] 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:
- [Medium] can: j1939: j1939_ac_process() drops new claim and leaves
  stale addr on cancel
- [Medium] can: j1939: j1939_ecu_timer_start() races with finishing
  __run_hrtimer()
--

commit d92351ad8ae3e165027e4084ec974a0caa02eabc
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 to avoid waiting for the
timer callback while holding priv->lock.

> diff --git a/net/can/j1939/address-claim.c b/net/can/j1939/address-claim.c
> index ca4ad6cdd5cbf..eb5916ac97bb4 100644
> --- a/net/can/j1939/address-claim.c
> +++ b/net/can/j1939/address-claim.c
> @@ -211,8 +211,11 @@ static void j1939_ac_process(struct j1939_priv *priv, struct sk_buff *skb)
>  	if (IS_ERR_OR_NULL(ecu))
>  		goto out_unlock_bh;
>  
> -	/* cancel pending (previous) address claim */
> -	j1939_ecu_timer_cancel(ecu);
> +	/* Cancel pending (previous) address claim. If we can't cancel,
> +	 * do nothing until pending (previous) address claim commits or aborts.
> +	 */
> +	if (!j1939_ecu_timer_cancel(ecu))
> +		goto out_ecu_put;

[Severity: Medium]
Does returning early here while setting ecu->abort_ac_handler = true drop
both the expiring address claim and the newly received replacement claim?

CPU0                              CPU1
j1939_ecu_timer_handler()
  /* waiting on priv->lock */
                                  j1939_ac_process() /* new claim for SA2 */
                                    write_lock_bh(&priv->lock)
                                    j1939_ecu_timer_cancel(ecu)
                                      hrtimer_try_to_cancel() -> -1
                                      ecu->abort_ac_handler = true
                                      return false
                                    goto out_ecu_put  <-- drops SA2 claim
                                    write_unlock_bh(&priv->lock)
  write_lock_bh(&priv->lock)
  if (ecu->abort_ac_handler)
    goto out                      <-- aborts SA1 claim without mapping
  j1939_ecu_put(ecu)

Because SAE J1939 address claim messages are one-shot announcements,
neither SA1 nor SA2 is claimed.  For a remote ECU (ecu->nusers == 0),
j1939_ecu_put() in the timer handler drops the last reference and frees
the struct j1939_ecu.  For a surviving ECU (such as a local ECU with
ecu->nusers > 0), ecu remains unmapped with no running timer while
ecu->addr still holds SA1, so any later claim for SA1 hits the early check
in j1939_ac_process() and is ignored:

net/can/j1939/address-claim.c:j1939_ac_process() {
	...
	if (ecu && ecu->addr == skcb->addr.sa) {
		...
		goto out_ecu_put;
	}
	...
}

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index 9b40f37546233..6520c91a6e1ec 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
> @@ -156,15 +156,35 @@ void j1939_ecu_timer_start(struct j1939_ecu *ecu)
>  		return;
>  
>  	/* Schedule timer in 250 msec to commit address change. */
> +	ecu->abort_ac_handler = false;
> +	ecu->done_ac_handler = false;
>  	hrtimer_start(&ecu->ac_timer, ms_to_ktime(250),
>  		      HRTIMER_MODE_REL_SOFT);
>  }
>  
> -void j1939_ecu_timer_cancel(struct j1939_ecu *ecu)
> +bool 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. */
> +		/* OK to call j1939_ecu_timer_start() again if
> +		 * j1939_ecu_timer_handler() already committed or aborted.
> +		 */
> +		if (ecu->done_ac_handler)
> +			break;
> +		/* Tell j1939_ecu_timer_handler() to abort. */
> +		ecu->abort_ac_handler = true;
> +		/* Don't call j1939_ecu_timer_start() until
> +		 * j1939_ecu_timer_handler() commits or aborts.
> +		 */
> +		return false;
> +	}
> +	return true;
>  }
>  
>  static enum hrtimer_restart j1939_ecu_timer_handler(struct hrtimer *hrtimer)
> @@ -174,14 +194,28 @@ 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->abort_ac_handler) {
> +		ecu->abort_ac_handler = false;
> +		goto out;
> +	}
> +
>  	/* TODO: can we test if ecu->addr is unicast before starting
>  	 * the timer?
>  	 */
>  	j1939_ecu_map_locked(ecu);
>  
> +out:
> +	/* Tell j1939_ecu_timer_cancel() that this handler has finished
> +	 * its critical section (either committed or aborted), so no need
> +	 * to set abort_ac_handler again while __run_hrtimer() is finishing.
> +	 */
> +	ecu->done_ac_handler = true;
> +	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]
Can allowing j1939_ecu_timer_start() while __run_hrtimer() is still
finishing cause a subsequent j1939_ecu_timer_cancel() to fail to cancel
the newly started timer and Poison it with ecu->abort_ac_handler = true?

__run_hrtimer() keeps base->running set to &ecu->ac_timer until after
j1939_ecu_timer_handler() returns and cpu_base->lock is re-acquired.  If
two address claim frames arrive after write_unlock_bh(&priv->lock) but
before __run_hrtimer() clears base->running:

CPU0                              CPU1
__run_hrtimer()
  j1939_ecu_timer_handler()
    ecu->done_ac_handler = true
    write_unlock_bh(&priv->lock)
                                  j1939_ac_process() /* frame 1 */
                                    j1939_ecu_timer_cancel(ecu)
                                      hrtimer_try_to_cancel() -> -1
                                      /* done_ac_handler is true */
                                      return true
                                    j1939_ecu_timer_start(ecu)
                                      ecu->done_ac_handler = false
                                      hrtimer_start(&ecu->ac_timer, ...)
                                  j1939_ac_process() /* frame 2 */
                                    j1939_ecu_timer_cancel(ecu)
                                      hrtimer_try_to_cancel() -> -1
                                      /* done_ac_handler is now false */
                                      ecu->abort_ac_handler = true
                                      return false  <-- drops frame 2
    j1939_ecu_put(ecu)
  base->running = NULL

Because hrtimer_callback_running(&ecu->ac_timer) is still true on CPU0,
hrtimer_try_to_cancel() on frame 2 returns -1 without dequeuing frame 1's
newly started timer.  With ecu->done_ac_handler cleared by frame 1,
j1939_ecu_timer_cancel() sets ecu->abort_ac_handler = true and drops frame
2, while frame 1's timer remains queued for 250 ms only to abort when it
fires.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/7cc08b0f-290e-464b-ab13-0a5ce3c57f4f@I-love.SAKURA.ne.jp?part=6

Re: [PATCH v13 1/6] can: j1939: fix possibility of UAF on module unload

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] can: j1939: premature module_put() in
  __j1939_priv_release_work()
--

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

can: j1939: fix possibility of UAF on module unload

This commit ties a module reference to each struct j1939_priv and defers
its final release to a dedicated workqueue that is destroyed in
j1939_module_exit().

> diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
> index d9384cf0e356f..0b8ac4fe0479d 100644
> --- a/net/can/j1939/main.c
> +++ b/net/can/j1939/main.c
[ ... ]
> @@ -152,9 +159,9 @@ static inline void j1939_priv_set(struct net_device *ndev,
>  	can_ml->j1939_priv = priv;
>  }
>  
> -static void __j1939_priv_release(struct kref *kref)
> +static void __j1939_priv_release_work(struct work_struct *work)
>  {
> -	struct j1939_priv *priv = container_of(kref, struct j1939_priv, kref);
> +	struct j1939_priv *priv = container_of(work, struct j1939_priv, release_work);
>  	struct net_device *ndev = priv->ndev;
>  
>  	netdev_dbg(priv->ndev, "%s: 0x%p\n", __func__, priv);
> @@ -165,6 +172,22 @@ static void __j1939_priv_release(struct kref *kref)
>  
>  	netdev_put(ndev, &priv->dev_tracker);
>  	kfree(priv);
> +	/* This is safe, for j1939_module_exit() waits for completion of
> +	 * this callback using destroy_workqueue().
> +	 */
> +	module_put(THIS_MODULE);
> +}
> +
> +static void __j1939_priv_release(struct kref *kref)
> +{
> +	struct j1939_priv *priv = container_of(kref, struct j1939_priv, kref);
> +
> +	/* Since synchronously dropping module refcount of self is not safe,
> +	 * defer actual release to WQ context.
> +	 */
> +	INIT_WORK(&priv->release_work, __j1939_priv_release_work);
> +	queue_work(j1939_wq, &priv->release_work);
> +
>  }

[Severity: Medium]
Can __j1939_priv_release_work() drop the final module reference and allow
can-j1939 to be unmapped before the caller of j1939_priv_put() has finished
executing module code?

Later patches in this series defer struct j1939_ecu destruction to
j1939_wq, but struct j1939_session destruction remains synchronous.  An RX
session created by j1939_xtp_rx_rts_session_new() has no associated socket
and holds a reference to priv, so it can outlive all J1939 sockets.  When
its timer expires, j1939_session_destroy() drops the last reference to
priv before freeing the session and returning:

net/can/j1939/transport.c:j1939_session_destroy() {
    ...
	__j1939_session_drop(session);
	netdev_put(session->priv->ndev, &session->priv_dev_tracker);
	j1939_priv_put(session->priv);
	kfree(session);
}

Because j1939_wq is a WQ_HIGHPRI workqueue and destroy_workqueue(j1939_wq)
only waits for work items on j1939_wq, the module can be unloaded while
the timer callback is still unwinding:

CPU0                              CPU1
j1939_tp_rxtimer()
  j1939_session_put()
    j1939_session_destroy()
      j1939_priv_put()
        __j1939_priv_release()
          queue_work(j1939_wq, ...)
  [preempted]
                                  __j1939_priv_release_work()
                                    kfree(priv)
                                    module_put(THIS_MODULE)
                                  delete_module()
                                    j1939_module_exit()
                                      destroy_workqueue(j1939_wq)
                                    free_module()
      kfree(session)  <-- executes unmapped module text

Deferring session destruction (or the j1939_priv_put() call in
j1939_session_destroy()) to j1939_wq as well would ensure
destroy_workqueue(j1939_wq) waits for all module code paths to finish.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/7cc08b0f-290e-464b-ab13-0a5ce3c57f4f@I-love.SAKURA.ne.jp?part=1