can: isotp: fix use-after-free race with concurrent NETDEV_UNREGISTER

isotp_release() looked up the bound network device via dev_get_by_index()
using the stored ifindex. During device unregistration the device is
unlisted from the ifindex hash before the NETDEV_UNREGISTER notifier
chain runs, so a concurrent isotp_release() could find no device, skip
can_rx_unregister() entirely, and still proceed to free the socket.
Since isotp_release() had already removed itself from the isotp
notifier list at that point, isotp_notify() would never get a chance to
clean up either, leaving a stale CAN filter that keeps pointing at the
freed socket.

Fix this the same way raw.c already does: hold a tracked reference to
the bound net_device in the socket (so->dev/so->dev_tracker) from
bind() onward instead of re-resolving it from the ifindex, and
serialize bind()/release() with rtnl_lock() so that so->dev is always
consistent with what the NETDEV_UNREGISTER notifier sees. so->dev
stays valid regardless of ifindex-hash unlisting, and is only ever
cleared by whichever of isotp_release()/isotp_notify() gets there
first, so the filter is always removed exactly once.

isotp_bind() now rejects a (re)bind with -EAGAIN while so->[tx|rx].state
isn't ISOTP_IDLE yet, so a timer left running by a prior
NETDEV_UNREGISTER can't act on a newly bound so->ifindex. Both checks
share the same lock_sock() section, so there is no window in which a
concurrent isotp_notify() clearing so->bound could be missed.

Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol")
Reported-by: sashiko-bot@kernel.org
Closes: https://lore.kernel.org/linux-can/20260707101420.47F261F000E9@smtp.kernel.org/
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Link: https://patch.msgid.link/20260712-isotp-fixes-v10-2-793a1b1ce17f@hartkopp.net
Cc: stable@kernel.org
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

diff --git a/net/can/isotp.c b/net/can/isotp.c
index d30937345bcdcec07507a91805431ac6b6ea9cdc..44c044eb83e12951c506d0245eb36cdadba5917b 100644 (file)
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -152,6 +152,8 @@ struct isotp_sock {
        struct sock sk;
        int bound;
        int ifindex;
+       struct net_device *dev;
+       netdevice_tracker dev_tracker;
        canid_t txid;
        canid_t rxid;
        ktime_t tx_gap;
@@ -978,6 +980,14 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
                        goto err_event_drop;
        }
 
+       /* so->bound is only checked once above - a wakeup may have
+        * unbound/rebound the socket meanwhile, so re-validate it
+        */
+       if (!so->bound) {
+               err = -EADDRNOTAVAIL;
+               goto err_out_drop;
+       }
+
        /* PDU size > default => try max_pdu_size */
        if (size > so->tx.buflen && so->tx.buflen < max_pdu_size) {
                u8 *newbuf = kmalloc(max_pdu_size, GFP_KERNEL);
@@ -1219,28 +1229,30 @@ static int isotp_release(struct socket *sock)
        list_del(&so->notifier);
        spin_unlock(&isotp_notifier_lock);
 
+       rtnl_lock();
        lock_sock(sk);
 
-       /* remove current filters & unregister */
-       if (so->bound) {
-               if (so->ifindex) {
-                       struct net_device *dev;
-
-                       dev = dev_get_by_index(net, so->ifindex);
-                       if (dev) {
-                               if (isotp_register_rxid(so))
-                                       can_rx_unregister(net, dev, so->rxid,
-                                                         SINGLE_MASK(so->rxid),
-                                                         isotp_rcv, sk);
-
-                               can_rx_unregister(net, dev, so->txid,
-                                                 SINGLE_MASK(so->txid),
-                                                 isotp_rcv_echo, sk);
-                               dev_put(dev);
-                       }
-               }
+       /* remove current filters & unregister
+        * tracked reference so->dev is taken at bind() time with rtnl_lock
+        */
+       if (so->bound && so->dev) {
+               if (isotp_register_rxid(so))
+                       can_rx_unregister(net, so->dev, so->rxid,
+                                         SINGLE_MASK(so->rxid),
+                                         isotp_rcv, sk);
+
+               can_rx_unregister(net, so->dev, so->txid,
+                                 SINGLE_MASK(so->txid),
+                                 isotp_rcv_echo, sk);
+               netdev_put(so->dev, &so->dev_tracker);
        }
 
+       so->ifindex = 0;
+       so->bound = 0;
+       so->dev = NULL;
+
+       rtnl_unlock();
+
        /* Always wait for a grace period before touching the timers below.
         * A concurrent NETDEV_UNREGISTER may have already unregistered our
         * filters and cleared so->bound in isotp_notify() without waiting
@@ -1253,9 +1265,6 @@ static int isotp_release(struct socket *sock)
        hrtimer_cancel(&so->txtimer);
        hrtimer_cancel(&so->rxtimer);
 
-       so->ifindex = 0;
-       so->bound = 0;
-
        sock_orphan(sk);
        sock->sk = NULL;
 
@@ -1310,6 +1319,7 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
        if (!addr->can_ifindex)
                return -ENODEV;
 
+       rtnl_lock();
        lock_sock(sk);
 
        if (so->bound) {
@@ -1317,6 +1327,17 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
                goto out;
        }
 
+       /* A transmission or reception that outlived a previous binding
+        * (unbound by NETDEV_UNREGISTER) may still be draining; the FC/echo
+        * and RX watchdog timers bound how long this takes. Checked together
+        * with so->bound in the same lock_sock() section above, so there is
+        * no window in which a concurrent isotp_notify() could be missed.
+        */
+       if (so->tx.state != ISOTP_IDLE || so->rx.state != ISOTP_IDLE) {
+               err = -EAGAIN;
+               goto out;
+       }
+
        /* ensure different CAN IDs when the rx_id is to be registered */
        if (isotp_register_rxid(so) && rx_id == tx_id) {
                err = -EADDRNOTAVAIL;
@@ -1329,14 +1350,12 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
                goto out;
        }
        if (dev->type != ARPHRD_CAN) {
-               dev_put(dev);
                err = -ENODEV;
-               goto out;
+               goto out_put_dev;
        }
        if (READ_ONCE(dev->mtu) < so->ll.mtu) {
-               dev_put(dev);
                err = -EINVAL;
-               goto out;
+               goto out_put_dev;
        }
        if (!(dev->flags & IFF_UP))
                notify_enetdown = 1;
@@ -1354,16 +1373,25 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
        can_rx_register(net, dev, tx_id, SINGLE_MASK(tx_id),
                        isotp_rcv_echo, sk, "isotpe", sk);
 
-       dev_put(dev);
-
        /* switch to new settings */
        so->ifindex = ifindex;
        so->rxid = rx_id;
        so->txid = tx_id;
        so->bound = 1;
 
+       /* bind() ok -> hold a reference for so->dev so that isotp_release()
+        * can safely reach the device later, even if a concurrent
+        * NETDEV_UNREGISTER has already unlisted it by ifindex.
+        */
+       so->dev = dev;
+       netdev_hold(so->dev, &so->dev_tracker, GFP_KERNEL);
+
+out_put_dev:
+       /* remove potential reference from dev_get_by_index() */
+       dev_put(dev);
 out:
        release_sock(sk);
+       rtnl_unlock();
 
        if (notify_enetdown) {
                sk->sk_err = ENETDOWN;
@@ -1566,7 +1594,7 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
        if (!net_eq(dev_net(dev), sock_net(sk)))
                return;
 
-       if (so->ifindex != dev->ifindex)
+       if (so->dev != dev)
                return;
 
        switch (msg) {
@@ -1582,10 +1610,12 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
                        can_rx_unregister(dev_net(dev), dev, so->txid,
                                          SINGLE_MASK(so->txid),
                                          isotp_rcv_echo, sk);
+                       netdev_put(so->dev, &so->dev_tracker);
                }
 
                so->ifindex = 0;
                so->bound  = 0;
+               so->dev = NULL;
                release_sock(sk);
 
                sk->sk_err = ENODEV;
@@ -1645,6 +1675,7 @@ static int isotp_init(struct sock *sk)
 
        so->ifindex = 0;
        so->bound = 0;
+       so->dev = NULL;
 
        so->opt.flags = CAN_ISOTP_DEFAULT_FLAGS;
        so->opt.ext_address = CAN_ISOTP_DEFAULT_EXT_ADDRESS;