[PATCH v7 1/2] can: j1939: fix possibility of list corruption in __j1939_ecu_release()

[PATCH v7 1/2] can: j1939: fix possibility of list corruption in __j1939_ecu_release()

[Tetsuo Handa]

Since not all callers are calling j1939_ecu_put() with priv->lock held
for write, list corruption 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).

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
This patch (either RCU or WQ) allows us to let j1939_ecu_get() in
j1939_ecu_map_locked() fail when addressing "can: j1939: uncanceled
ac_timer in j1939_ecu_unmap_all() leaks resources" problem.

This patch (not RCU but WQ) allows us to use tasklet_kill() in
__j1939_ecu_release_work() when addressing "can: j1939: ABBA deadlock
in j1939_ecu_timer_cancel() under priv->lock" problem.

 net/can/j1939/bus.c        | 64 +++++++++++++++++++++++++++-----------
 net/can/j1939/j1939-priv.h |  4 +++
 net/can/j1939/main.c       | 13 ++++++++
 3 files changed, 63 insertions(+), 18 deletions(-)

diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
index cdc3c0a71937..3422e3133800 100644
--- a/net/can/j1939/bus.c
+++ b/net/can/j1939/bus.c
@@ -14,25 +14,50 @@
 
 #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);
 	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, priv->ecus readers might find ECUs with ecu->kref == 0.
+	 * Therefore, priv->ecus 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 __must_check bool j1939_ecu_get(struct j1939_ecu *ecu)
 {
-	kref_get(&ecu->kref);
+	/* Don't get an ECU ref if __j1939_ecu_release() was called. */
+	return kref_get_unless_zero(&ecu->kref);
+}
+
+static __must_check bool j1939_ecu_valid(struct j1939_ecu *ecu)
+{
+	/* 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 +81,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 +136,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 +150,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 +166,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 +216,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 +242,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 +259,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 v7 2/2] can: j1939: fix "struct j1939_ecu" refcount leak in j1939_ecu_map_locked()

[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 making j1939_ecu_get() also fail 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).

For now we don't cancel ecu->ac_timer from j1939_ecu_unmap_all(), for
we can't safely call hrtimer_cancel() with priv->lock held. However,
since ecu->ac_timer expires in 250ms, for now let's tolerate delaying
completion of NETDEV_UNREGISTER event for less than one second.

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

diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
index 3422e3133800..11d08b748548 100644
--- a/net/can/j1939/bus.c
+++ b/net/can/j1939/bus.c
@@ -50,8 +50,13 @@ void j1939_ecu_put(struct j1939_ecu *ecu)
 
 static __must_check bool 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);
+	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) && kref_get_unless_zero(&ecu->kref);
 }
 
 static __must_check bool j1939_ecu_valid(struct j1939_ecu *ecu)
-- 
2.56.0

Re: [PATCH v7 2/2] can: j1939: fix "struct j1939_ecu" refcount leak in j1939_ecu_map_locked()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] can: j1939: duplicate ECU creation and UAF when j1939_ecu_get() fails
--

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

can: j1939: fix "struct j1939_ecu" refcount leak in j1939_ecu_map_locked()

This commit changes j1939_ecu_get() to fail when the associated net_device is
no longer in NETREG_REGISTERED state or when priv->rx_kref is zero, preventing
j1939_ecu_map_locked() from remapping an ECU after j1939_ecu_unmap_all() has
completed.

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index 3422e31338004..11d08b7485489 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
> @@ -50,8 +50,13 @@ void j1939_ecu_put(struct j1939_ecu *ecu)
>  
>  static __must_check bool 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);
> +	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) && kref_get_unless_zero(&ecu->kref);
>  }

[Severity: Critical]
Can this cause duplicate struct j1939_ecu creation, use-after-free when
ecu->ac_timer fires, and a leaked net_device reference when
j1939_ecu_get() rejects a live ECU?

With this change, j1939_ecu_get() returns false whenever
priv->ndev->reg_state != NETREG_REGISTERED or kref_read(&priv->rx_kref) == 0,
even if an existing ECU on priv->ecus still has ecu->kref > 0 (for example,
while its 250ms ac_timer is pending or while another socket holds it).

However, both callers of j1939_ecu_get_by_name_locked() that manage named
ECUs assume a NULL return means no valid ECU with that name exists, and fall
back to j1939_ecu_create_locked():

net/can/j1939/bus.c:j1939_local_ecu_get() {
	ecu = j1939_ecu_get_by_name_locked(priv, name);
	if (!ecu)
		ecu = j1939_ecu_create_locked(priv, name);
	if (IS_ERR(ecu))
		goto unlock_priv;
}

net/can/j1939/address-claim.c:j1939_ac_process() {
	ecu = j1939_ecu_get_by_name_locked(priv, name);
	if (ecu && ecu->addr == skcb->addr.sa) {
		...
	}
	if (!ecu && j1939_address_is_unicast(skcb->addr.sa))
		ecu = j1939_ecu_create_locked(priv, name);
}

Because j1939_ecu_create_locked() does not check priv->ndev->reg_state or
priv->rx_kref, a concurrent j1939_sk_bind() -> j1939_local_ecu_get() or
in-flight j1939_can_recv() -> j1939_ac_process() running after reg_state
transitions to NETREG_UNREGISTERING (or after rx_kref reaches 0) will skip
the existing ECU on priv->ecus and append a second struct j1939_ecu with the
same name to priv->ecus.

When the socket later unbinds in j1939_sk_netdev_event_unregister() or
j1939_sk_release(), j1939_local_ecu_put() looks up the ECU via
j1939_ecu_find_by_name_locked(), which checks j1939_ecu_valid():

net/can/j1939/bus.c:j1939_ecu_valid() {
	/* Treat an ECU as invalid if __j1939_ecu_release() was called. */
	return kref_read(&ecu->kref);
}

net/can/j1939/bus.c:j1939_local_ecu_put() {
	ecu = j1939_ecu_find_by_name_locked(priv, name);
	if (WARN_ON_ONCE(!ecu))
		goto done;

	ecu->nusers--;
	/* TODO: do we care if ecu->addr != sa? */
	if (j1939_ecu_is_mapped_locked(ecu))
		/* ecu's sa is active already */
		priv->ents[ecu->addr].nusers--;
	j1939_ecu_put(ecu);
}

Because j1939_ecu_valid() only checks ecu->kref, j1939_local_ecu_put()
matches the first ECU on priv->ecus instead of the newly created duplicate.
This drops the first ECU's reference (which may belong to its pending
ac_timer, freeing the first ECU while ac_timer is still armed) and leaves the
second ECU at the tail of priv->ecus permanently leaked along with its
struct j1939_priv and net_device references.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/00aae7aa-8351-4d69-8938-4d8e4eee78c2@I-love.SAKURA.ne.jp?part=2

Re: [PATCH v7 1/2] can: j1939: fix possibility of list corruption in __j1939_ecu_release()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit "can: j1939: fix possibility of list corruption in
  __j1939_ecu_release()" fixes a real, reachable memory-safety bug but…
- [Low] The commit message calls the bug only a "possibility of list
  corruption", which understates it.

Pre-existing issues:
- [High] Nothing cancels a pending ecu->ac_timer on teardown, so the new
  destroy_workqueue(j1939_ecu_wq) in j1939_module_exit() does not…
- [High] ABBA-style deadlock in j1939_ac_process()
  (net/can/j1939/address-claim.c).

--

can: j1939: fix possibility of list corruption in __j1939_ecu_release()

This patch moves the final release of a struct j1939_ecu to a dedicated
workqueue, so list_del() on priv->ecus always runs under
write_lock_bh(&priv->lock). ECUs can now stay on priv->ecus with a zero
refcount, so j1939_ecu_get() switches to kref_get_unless_zero() and the
list walkers skip ECUs whose kref has dropped to zero.

> Since not all callers are calling j1939_ecu_put() with priv->lock held
> for write, list corruption might happen if the last user called
> j1939_ecu_put() without priv->lock held for write.

[Severity: Low]
Is list corruption the whole problem here? Before this patch, a final put
without the lock ran list_del() and then kfree() straight away:

__j1939_ecu_release()
    list_del(&ecu->list);
    netdev_put(priv->ndev, &ecu->priv_dev_tracker);
    kfree(ecu);

Some readers walk priv->ecus under read_lock_bh(&priv->lock) and
dereference ecu->name and ecu->list.next:

  - j1939_ecu_find_by_name_locked(), via j1939_name_to_addr() and
    j1939_local_ecu_put()
  - j1939_ecu_get_by_name_locked()

For example:

CPU A: j1939_ac_recv()
    ecu = j1939_ecu_get_by_addr(priv, skcb->addr.sa);
CPU B: unmaps the same ECU under write_lock_bh(&priv->lock)
CPU A:
    j1939_ecu_put(ecu);   <- final put, kfree(ecu) without priv->lock
CPU C: j1939_ecu_find_by_name_locked()
    list_for_each_entry(ecu, &priv->ecus, list)
        if (ecu->name == name)   <- reads the freed ecu

Wouldn't that be a use-after-free, reachable from the CAN RX softirq and
from sendmsg?

The code change covers this, because the release now runs under
write_lock_bh() in __j1939_ecu_release_work(). Could the commit message
mention the use-after-free too?

> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

[Severity: Low]
Should this patch carry a Fixes: tag and Cc: stable? The unlocked
list_del() and kfree() in __j1939_ecu_release() come from:

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")

j1939_ac_fixup() and j1939_ac_recv() can both drop the last ECU reference
without priv->lock:

j1939_ac_recv()
    ecu = j1939_ecu_get_by_addr(priv, skcb->addr.sa);
    if (ecu) {
        skcb->addr.src_name = ecu->name;
        j1939_ecu_put(ecu);
    }

The next patch in the series, "can: j1939: fix "struct j1939_ecu"
refcount leak in j1939_ecu_map_locked()", carries Fixes: 9d71dd0c7009. It
also edits the bool-returning j1939_ecu_get() that this patch adds.

With no tags on this patch, could stable tooling take the follow-up
without its prerequisite, or skip this fix altogether?

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index cdc3c0a719379..3422e31338004 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c

[ ... ]

> @@ -122,6 +150,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);
>  }

[Severity: High]
This is a pre-existing issue, but can this hrtimer_cancel() deadlock
against j1939_ecu_timer_handler()? j1939_ac_process() calls it with
priv->lock held for write:

j1939_ac_process()
    write_lock_bh(&priv->lock);
    ...
    /* cancel pending (previous) address claim */
    j1939_ecu_timer_cancel(ecu);
        hrtimer_cancel(&ecu->ac_timer);

The timer callback takes the same lock first:

j1939_ecu_timer_handler()
    write_lock_bh(&priv->lock);

Suppose the soft hrtimer callback is already running on CPU1 and spinning
on priv->lock. Then hrtimer_try_to_cancel() on CPU0 returns -1, and
hrtimer_cancel() keeps looping in hrtimer_cancel_wait_running() for a
callback that can never finish. On PREEMPT_RT it waits on the expiry lock,
which is held by the softirq that is blocked on priv->lock.

The trigger is an address-claim frame for the same NAME with a different
SA, arriving while the previous ac_timer is expiring. Any node on the CAN
bus can send such a frame.

The commit notes list this as a separate problem ("ABBA deadlock in
j1939_ecu_timer_cancel() under priv->lock"). Neither this patch nor the
next one in the series changes it.

[ ... ]

> diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
> index d9384cf0e356f..2030f24fce704 100644
> --- a/net/can/j1939/main.c
> +++ b/net/can/j1939/main.c

[ ... ]

> @@ -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);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does destroy_workqueue()
here give the guarantee the comment describes? It only drains work that is
already queued. Nothing cancels an ecu->ac_timer that is still armed at
teardown.

j1939_ecu_timer_start() takes an ECU reference and arms a 250 ms timer.
If the last j1939 socket is then closed:

j1939_netdev_stop()
  __j1939_rx_release()
    j1939_ecu_unmap_all()

only the priv->ents[] references are dropped, and hrtimer_cancel() is
never called. With the sockets gone, the module refcount can reach zero,
and rmmod can run j1939_module_exit() while the timer is still pending.

When the timer fires, j1939_ecu_timer_handler() runs from unloaded module
text. If its put is the final one:

j1939_ecu_timer_handler()
  j1939_ecu_put(ecu)
    __j1939_ecu_release()
      queue_work(j1939_ecu_wq, &ecu->release_work);

wouldn't queue_work() operate on the workqueue that destroy_workqueue()
has already freed?

The timer firing into unloaded module text predates this patch, and the
commit notes list the uncanceled ac_timer as separate work.

The follow-up "can: j1939: fix "struct j1939_ecu" refcount leak in
j1939_ecu_map_locked()" fixes the re-map leak by making j1939_ecu_get()
fail once rx_kref is 0. Its commit message says ac_timer is still not
cancelled, so this race is still present at the end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/00aae7aa-8351-4d69-8938-4d8e4eee78c2%40I-love.SAKURA.ne.jp

Re: [PATCH v7 2/2] can: j1939: fix "struct j1939_ecu" refcount leak in j1939_ecu_map_locked()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 1 · High: 0 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Critical] The patch adds the new conditions
  (`READ_ONCE(priv->ndev->reg_state) == NETREG_REGISTERED` and…

--

can: j1939: fix "struct j1939_ecu" refcount leak in j1939_ecu_map_locked()

With this commit, j1939_ecu_get() fails if the netdev tied to j1939_priv
is no longer NETREG_REGISTERED, or if __j1939_rx_release() has already
dropped priv->rx_kref to zero. The aim is to stop
j1939_ecu_timer_handler() from mapping an ECU back into priv->ents[]
after j1939_ecu_unmap_all() has run.

Closes: https://syzkaller.appspot.com/bug?extid=e2af46126e0644cbebdd

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index 3422e31338004..11d08b7485489 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
> @@ -50,8 +50,13 @@ void j1939_ecu_put(struct j1939_ecu *ecu)
>  
>  static __must_check bool 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);
> +	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) && kref_get_unless_zero(&ecu->kref);

[Severity: Critical]
The new gate is in the shared j1939_ecu_get(), not only on the map path.
Does that make the two name lookups in bus.c disagree?

Once reg_state leaves NETREG_REGISTERED, j1939_ecu_get_by_name_locked()
fails even for a live ECU. j1939_ecu_find_by_name_locked() still only
checks j1939_ecu_valid():

net/can/j1939/bus.c:j1939_ecu_find_by_name_locked() {
	...
	list_for_each_entry(ecu, &priv->ecus, list) {
		if (ecu->name == name && j1939_ecu_valid(ecu))
			return ecu;
	}
	...
}

j1939_local_ecu_get() takes a NULL from the gated lookup to mean there is
no ECU with this NAME, so it creates one:

	ecu = j1939_ecu_get_by_name_locked(priv, name);
	if (!ecu)
		ecu = j1939_ecu_create_locked(priv, name);

The duplicate (E2) is added after the original (E1) and holds its own
kref, netdev_hold() and j1939_priv_get(). j1939_local_ecu_put() uses the
ungated j1939_ecu_find_by_name_locked(), so it finds and puts E1, not E2.

unregister_netdevice_many_notify() sets reg_state to NETREG_UNREGISTERING
before it calls synchronize_net() and before it sends NETDEV_UNREGISTER.
In that window, the re-bind branch of j1939_sk_bind() runs without any
reg_state check:

	/* drop old references */
	j1939_jsk_del(priv, jsk);
	j1939_local_ecu_put(priv, jsk->addr.src_name, jsk->addr.sa);
	...
	ret = j1939_local_ecu_get(priv, jsk->addr.src_name, jsk->addr.sa);

The first-bind reg_state check runs without rtnl, so it can race too.

Could this sequence happen?

1. Socket A is bound with NAME N, so E1.kref == 1.

2. An address claim for N goes through
   j1939_ac_process()->j1939_ecu_timer_start(E1). E1.kref == 2 and the
   250ms ac_timer is armed.

3. The device is unregistered and reg_state becomes NETREG_UNREGISTERING.

4. A re-binds. j1939_local_ecu_put() drops E1 to 1. Then
   j1939_ecu_get_by_name_locked() fails and E2 is created.

5. NETDEV_UNREGISTER runs
   j1939_sk_netdev_event_unregister()->j1939_local_ecu_put(). That finds
   E1 again and drops it to 0, using up the reference held by the
   pending timer.

6. __j1939_ecu_release_work() calls kfree(ecu) without cancelling
   ac_timer. j1939_ecu_timer_handler() would then run on freed memory.

There is also a variant. Suppose the timer fires and re-maps E1 before
reg_state changes. The misdirected put then frees E1 while
priv->ents[addr].ecu still points at it. After that,
j1939_netdev_stop()->__j1939_rx_release()->j1939_ecu_unmap_all() would
dereference and put the freed ECU. E1->nusers and ents[].nusers would
also go negative.

In both cases E2 is never put. It leaks together with its netdev and priv
references, so unregister_netdevice() would wait forever. Isn't that the
same kind of j1939_ecu refcount leak this commit is meant to fix?

Also, the comment in __j1939_ecu_release() still says readers can
validate with either j1939_ecu_valid() or j1939_ecu_get(), as if they
were the same check.

Is the reg_state test needed for the reported bug at all? The rx_kref
test under priv->lock already looks enough to order the timer handler's
map against __j1939_rx_release().

Could the new checks go only in j1939_ecu_map_locked() and
j1939_ecu_timer_start()? Or could both name lookups use the same check?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/00aae7aa-8351-4d69-8938-4d8e4eee78c2%40I-love.SAKURA.ne.jp