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

[netdev-bot+sashiko@kernel.org]

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