Re: [PATCH v2 (repost)] can: j1939: use netdevice_tracker for j1939_{priv,session,ecu} tracking

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] In `j1939_netdev_start`, if `j1939_can_rx_register` fails, the error path forcefully calls `kfree(priv)` instead of properly dropping the reference via `j1939_priv_put(priv)`. This causes a use-after-free if a concurrent CAN RX interrupt accessed `priv` and created a session or ECU.
- [High] The destructors for `j1939_session` and `j1939_ecu` do not explicitly cancel their embedded hrtimers (`txtimer`, `rxtimer`, `ac_timer`) before freeing the memory.
- [High] Using `gfp_any()` under a locked context (`priv->lock`) may resolve to `GFP_KERNEL` and cause the thread to sleep while atomic.
- [High] Double dropping of SKB references in `j1939_session_destroy` causes premature SKB freeing, refcount underflow, and potential Use-After-Free.
- [High] Unprotected list deletion in `__j1939_ecu_release` risks list corruption if `j1939_ecu_put` is called without holding the global write lock.
--

commit 7da6da8c70916fd416a2173c592294f932e47769
Author: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Subject: can: j1939: use netdevice_tracker for j1939_{priv,session,ecu} tracking

This patch converts j1939_{priv,session,ecu} tracking to use the
netdevice_tracker API. It replaces dev_hold() and dev_put() with
netdev_hold() and netdev_put() to help debug a reference count leak
reported by syzbot.

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index dc374286eeb6e..cdc3c0a719379 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
> @@ -20,6 +20,7 @@ static void __j1939_ecu_release(struct kref *kref)
>  	struct j1939_priv *priv = ecu->priv;
>  
>  	list_del(&ecu->list);

[Severity: High]
This isn't a bug introduced by this patch, but does this unprotected list
deletion risk list corruption if __j1939_ecu_release() is called without
holding the global write lock?

If the lock is not held, it races with other readers/writers of priv->ecus.
Conversely, if the lock is required to be held, calling j1939_priv_put(priv)
later in this function could free priv while the lock is still held, leading
to a use-after-free on the lock itself during unlock.

> +	netdev_put(priv->ndev, &ecu->priv_dev_tracker);
>  	kfree(ecu);

[Severity: High]
This is a pre-existing issue, but does kfree(ecu) risk a use-after-free if
called while ecu->ac_timer is pending?

If this memory is freed without explicitly canceling the hrtimer, an
asynchronous timer callback might subsequently execute on the freed memory.

>  	j1939_priv_put(priv);
>  }
> @@ -155,6 +156,7 @@ struct j1939_ecu *j1939_ecu_create_locked(struct j1939_priv *priv, name_t name)
>  	if (!ecu)
>  		return ERR_PTR(-ENOMEM);
>  	kref_init(&ecu->kref);
> +	netdev_hold(priv->ndev, &ecu->priv_dev_tracker, gfp_any());

[Severity: High]
This is a pre-existing issue, but will passing gfp_any() while holding
priv->lock cause the thread to sleep while atomic?

The gfp_any() helper checks in_softirq(). If priv->lock (an rwlock) was
acquired via write_lock_irqsave or write_lock in process context, in_softirq()
will evaluate to false, causing gfp_any() to return GFP_KERNEL. Allocating
memory with GFP_KERNEL while holding an rwlock can cause the thread to sleep
while atomic.

>  	ecu->addr = J1939_IDLE_ADDR;
>  	ecu->name = name;
>  

[ ... ]

> diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
> index 9937c04241bc3..5e5e6c228f225 100644
> --- a/net/can/j1939/main.c
> +++ b/net/can/j1939/main.c

[ ... ]

> @@ -298,7 +298,7 @@ struct j1939_priv *j1939_netdev_start(struct net_device *ndev)
>  	j1939_priv_set(ndev, NULL);
>  	mutex_unlock(&j1939_netdev_lock);
>  
> -	dev_put(ndev);
> +	netdev_put(ndev, &priv->dev_tracker);
>  	kfree(priv);

[Severity: High]
This is a pre-existing issue, but does this kfree(priv) risk a use-after-free?

In j1939_netdev_start(), priv is created with a kref of 1 and exposed via
j1939_priv_set(ndev, priv). The call to j1939_can_rx_register(priv) then makes
priv accessible to incoming packets.

If a packet arrives concurrently, the rx handler could take a reference to
priv and create sessions or ecus. If j1939_can_rx_register() subsequently
fails, this error path forcefully calls kfree(priv) unconditionally, bypassing
the reference count. Any concurrently created sessions would then access the
freed priv memory. Should this use j1939_priv_put(priv) instead?

>  
>  	return ERR_PTR(ret);

[ ... ]

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index df93d57907da7..158433511552e 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -283,6 +283,7 @@ static void j1939_session_destroy(struct j1939_session *session)

[Severity: High]
This isn't a bug introduced by this patch, but does the loop in
j1939_session_destroy() underflow the skb reference count?

Looking at the loop just above this context:

	while ((skb = skb_dequeue(&session->skb_queue)) != NULL) {
		/* drop ref taken in j1939_session_skb_queue() */
		skb_unref(skb);
		kfree_skb(skb);
	}

j1939_session_new() only took one extra reference via skb_get(skb). Calling
both skb_unref() and kfree_skb() drops the reference count twice for each
dequeued skb, which can underflow the refcount and cause a use-after-free
if the original owner still holds a valid pointer.

>  		kfree_skb(skb);
>  	}
>  	__j1939_session_drop(session);
> +	netdev_put(session->priv->ndev, &session->priv_dev_tracker);
>  	j1939_priv_put(session->priv);
>  	kfree(session);

[Severity: High]
This is a pre-existing issue, but does freeing the session here without
stopping its timers risk a use-after-free?

If the session is destroyed while session->txtimer or session->rxtimer is
pending, the asynchronous timer callbacks could execute after kfree(session).
Should the hrtimers be explicitly canceled before the memory is freed?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/2bc332e0-b250-4679-a075-1c413ad843ce@I-love.SAKURA.ne.jp?part=1