[PATCH] can: isotp: hold a reference to the bound netdevice

[Vasileios Almpanis]

isotp_release() looked up the bound netdevice with
dev_get_by_index(so->ifindex) and only unregistered the socket's CAN
receivers if that lookup succeeded. This is unreliable while the device
is going away: unregister_netdevice_many() removes the device from the
ifindex hash before the NETDEV_UNREGISTER notifier is fired, with a
synchronize_net() in between.

A close() that runs in that window first removes the socket from
isotp_notifier_list, so the pending notifier no longer sees it, and then
gets NULL back from dev_get_by_index() and skips the unregister. The two
struct receiver entries that were allocated by can_rx_register() in
isotp_bind() (the rx filter and the tx echo filter) are then never freed
and leak once the device is destroyed.

Fix it the same way can-raw already does: keep a tracked reference to the
bound device in so->dev and unregister through it, and take rtnl_lock()
in isotp_bind() and isotp_release() to serialise against the
NETDEV_UNREGISTER notifier. The dev_get_by_index() lookup in the release
path is dropped.

Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol")
Cc: stable@xxxxxxxxxxxxxxx
Reported-by: syzbot+24201717ed2da31b8fae@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?id=58676a0f698531996a42612c552e894a55b9732b
Signed-off-by: Vasileios Almpanis <vasilisalmpanis@xxxxxxxxx>
---
 net/can/isotp.c | 48 +++++++++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 21 deletions(-)

diff --git a/net/can/isotp.c b/net/can/isotp.c
index c48b4a818297..308d18040b6d 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -69,6 +69,7 @@
 #include <linux/can/skb.h>
 #include <linux/can/isotp.h>
 #include <linux/slab.h>
+#include <linux/rtnetlink.h>
 #include <net/can.h>
 #include <net/sock.h>
 #include <net/net_namespace.h>
@@ -152,6 +153,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;
@@ -1219,27 +1222,22 @@ 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);
-				synchronize_rcu();
-			}
-		}
+	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->dev = NULL;
+		synchronize_rcu();
 	}
 
 	hrtimer_cancel(&so->txfrtimer);
@@ -1253,6 +1251,7 @@ static int isotp_release(struct socket *sock)
 	sock->sk = NULL;
 
 	release_sock(sk);
+	rtnl_unlock();
 	sock_prot_inuse_add(net, sk->sk_prot, -1);
 	sock_put(sk);
 
@@ -1303,6 +1302,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) {
@@ -1347,16 +1347,20 @@ 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;
 
+	so->dev = dev;
+	netdev_hold(dev, &so->dev_tracker, GFP_KERNEL);
+
+	dev_put(dev);
+
 out:
 	release_sock(sk);
+	rtnl_unlock();
 
 	if (notify_enetdown) {
 		sk->sk_err = ENETDOWN;
@@ -1559,7 +1563,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) {
@@ -1575,10 +1579,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(dev, &so->dev_tracker);
 		}
 
 		so->ifindex = 0;
 		so->bound  = 0;
+		so->dev = NULL;
 		release_sock(sk);
 
 		sk->sk_err = ENODEV;
-- 
2.47.3