Re: [PATCH v2] can: j1939: j1939_sk_bind(): fix j1939_ecu leak when re-bind failed

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] `__j1939_ecu_release()` removes an ECU from the `priv->ecus` list without holding `priv->lock`, which leads to concurrent list corruption if the last reference is dropped outside the lock.
--

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

can: j1939: j1939_sk_bind(): fix j1939_ecu leak when re-bind failed

This commit fixes a refcount leak in struct j1939_ecu when a re-bind
operation fails. It defers the removal and release of old ECU references
until after new resources are successfully acquired.

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

> diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
> index 50a598ef5fd4a..24efb25c58c3f 100644
> --- a/net/can/j1939/socket.c
> +++ b/net/can/j1939/socket.c
[ ... ]
> @@ -519,22 +517,31 @@ static int j1939_sk_bind(struct socket *sock, struct sockaddr_unsized *uaddr, in
>  		jsk->priv = priv;
>  	}
>  
> +	/* get new references without dropping old references */
> +	ret = j1939_local_ecu_get(priv, addr->can_addr.j1939.name, addr->can_addr.j1939.addr);
[ ... ]
> +	/* drop old references after re-bind() succeeded */
> +	if (was_bound) {
> +		j1939_jsk_del(priv, jsk);
> +		j1939_local_ecu_put(priv, jsk->addr.src_name, jsk->addr.sa);

[Severity: Critical]
This is a pre-existing issue, but does dropping the ECU reference here expose
a race condition with incoming address claim packets?

If this j1939_local_ecu_put() drops the primary reference to the ECU while
holding priv->lock, could a concurrent j1939_ac_recv() processing an address
claim packet drop the final reference without holding the lock?

Looking at j1939_ac_recv() in net/can/j1939/address-claim.c, it temporarily
gets an ECU and then drops it locklessly:

net/can/j1939/address-claim.c:j1939_ac_recv() {
    ...
    ecu = j1939_ecu_get_by_addr(priv, sa);
    ...
    j1939_ecu_put(ecu);
}

If the lockless j1939_ecu_put() in j1939_ac_recv() drops the final reference
to 0, it invokes __j1939_ecu_release(), which performs list deletion:

net/can/j1939/bus.c:__j1939_ecu_release() {
    ...
    list_del(&ecu->list);
    ...
}

Since this list_del() executes entirely outside of priv->lock, can this lead
to concurrent modifications to the priv->ecus doubly-linked list and cause
memory corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/45b82053-7f32-43fc-b37b-067058a72c98@I-love.SAKURA.ne.jp?part=1