[PATCH] can: j1939: close theoretical race window in j1939_sk_netdev_event_unregister()

[PATCH] can: j1939: close theoretical race window in j1939_sk_netdev_event_unregister()

[Tetsuo Handa]

There is a theoretical race window in j1939_sk_netdev_event_unregister()
where two j1939_sk_bind() calls jump in between read_unlock_bh() and
lock_sock().

The assumption jsk->priv == priv can fail if the first j1939_sk_bind()
call once made jsk->priv == NULL due to failed j1939_local_ecu_get() call
and the second call again made jsk->priv != NULL due to successful
j1939_local_ecu_get() call.

Close this race by also checking jsk->priv == priv.

Fixes: 7fcbe5b2c6a4 ("can: j1939: implement NETDEV_UNREGISTER notification handler")
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
Do we want to make j1939_sk_bind() transactional (i.e. don't make any changes
when it fails) after j1939_sk_bind() once succeeded, for the J1939_SOCK_BOUND
state being cleared might be an unexpected behavior?
Or, programs will just close() or exit() if re-bind() request failed?

 net/can/j1939/socket.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
index 88e7160d4248..0a377ad75038 100644
--- a/net/can/j1939/socket.c
+++ b/net/can/j1939/socket.c
@@ -1313,7 +1313,7 @@ void j1939_sk_netdev_event_unregister(struct j1939_priv *priv)
 	read_lock_bh(&priv->j1939_socks_lock);
 	list_for_each_entry(jsk, &priv->j1939_socks, list) {
 		/* Skip if j1939_jsk_add() is not called on this socket. */
-		if (!(jsk->state & J1939_SOCK_BOUND))
+		if (!(jsk->state & J1939_SOCK_BOUND) || jsk->priv != priv)
 			continue;
 		sk = &jsk->sk;
 		sock_hold(sk);
@@ -1323,7 +1323,7 @@ void j1939_sk_netdev_event_unregister(struct j1939_priv *priv)
 		 * j1939_jsk_del() with socket's lock held.
 		 */
 		lock_sock(sk);
-		if (jsk->state & J1939_SOCK_BOUND) {
+		if ((jsk->state & J1939_SOCK_BOUND) && jsk->priv == priv) {
 			/* Neither j1939_sk_bind() nor j1939_sk_release() called j1939_jsk_del().
 			 * Make this socket no longer bound, by pretending as if j1939_sk_bind()
 			 * dropped old references but did not get new references.
-- 
2.47.3

[PATCH v2] can: j1939: make j1939_sk_bind() fail if device is no longer registered

[Tetsuo Handa]

There is a theoretical race window in j1939_sk_netdev_event_unregister()
where two j1939_sk_bind() calls jump in between read_unlock_bh() and
lock_sock().

The assumption jsk->priv == priv can fail if the first j1939_sk_bind()
call once made jsk->priv == NULL due to failed j1939_local_ecu_get() call
and the second j1939_sk_bind() call again made jsk->priv != NULL due to
successful j1939_local_ecu_get() call.

Since the socket lock is held by both j1939_sk_netdev_event_unregister()
and j1939_sk_bind(), checking ndev->reg_state with the socket lock held can
reliably make the second j1939_sk_bind() call fail (and close this race
window).

Fixes: 7fcbe5b2c6a4 ("can: j1939: implement NETDEV_UNREGISTER notification handler")
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 net/can/j1939/socket.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
index 88e7160d4248..e3ba2e9fc0e9 100644
--- a/net/can/j1939/socket.c
+++ b/net/can/j1939/socket.c
@@ -482,6 +482,12 @@ static int j1939_sk_bind(struct socket *sock, struct sockaddr *uaddr, int len)
 			goto out_release_sock;
 		}
 
+		if (ndev->reg_state != NETREG_REGISTERED) {
+			dev_put(ndev);
+			ret = -ENODEV;
+			goto out_release_sock;
+		}
+
 		can_ml = can_get_ml_priv(ndev);
 		if (!can_ml) {
 			dev_put(ndev);
-- 
2.47.3

Re: [PATCH v2] can: j1939: make j1939_sk_bind() fail if device is no longer registered

[Oleksij Rempel]

On Tue, Nov 25, 2025 at 10:43:12PM +0900, Tetsuo Handa wrote:
> There is a theoretical race window in j1939_sk_netdev_event_unregister()
> where two j1939_sk_bind() calls jump in between read_unlock_bh() and
> lock_sock().
> 
> The assumption jsk->priv == priv can fail if the first j1939_sk_bind()
> call once made jsk->priv == NULL due to failed j1939_local_ecu_get() call
> and the second j1939_sk_bind() call again made jsk->priv != NULL due to
> successful j1939_local_ecu_get() call.
> 
> Since the socket lock is held by both j1939_sk_netdev_event_unregister()
> and j1939_sk_bind(), checking ndev->reg_state with the socket lock held can
> reliably make the second j1939_sk_bind() call fail (and close this race
> window).
> 
> Fixes: 7fcbe5b2c6a4 ("can: j1939: implement NETDEV_UNREGISTER notification handler")
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

I guess, this one is needed too:
Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2] can: j1939: make j1939_sk_bind() fail if device is no longer registered

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1276 bytes --]

On 25.11.2025 22:43:12, Tetsuo Handa wrote:
> There is a theoretical race window in j1939_sk_netdev_event_unregister()
> where two j1939_sk_bind() calls jump in between read_unlock_bh() and
> lock_sock().
>
> The assumption jsk->priv == priv can fail if the first j1939_sk_bind()
> call once made jsk->priv == NULL due to failed j1939_local_ecu_get() call
> and the second j1939_sk_bind() call again made jsk->priv != NULL due to
> successful j1939_local_ecu_get() call.
>
> Since the socket lock is held by both j1939_sk_netdev_event_unregister()
> and j1939_sk_bind(), checking ndev->reg_state with the socket lock held can
> reliably make the second j1939_sk_bind() call fail (and close this race
> window).
>
> Fixes: 7fcbe5b2c6a4 ("can: j1939: implement NETDEV_UNREGISTER notification handler")
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

Applied to linux-can.

Thanks,
Marc

P.S.: Don't send a -vN+1 patch as a reply to a -vN. Please start a new thread.

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]