can: isotp: fix use-after-free race with concurrent NETDEV_UNREGISTER
Subsystems (2)
Oliver Hartkopp
maintainerPATCH 1/1 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.
--- net/can/isotp.c | 68 +++++++++++++++++++++++++++++-------------------- 1 file changed, 40 insertions(+), 28 deletions(-)
net/can/isotp.c +40 -28
@@ -150,10 +150,12 @@ struct tpcon {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;ktime_t lastrxcf_tstamp;struct hrtimer rxtimer, txtimer, txfrtimer;
@@ -1217,32 +1219,34 @@ static int isotp_release(struct socket *sock)spin_lock (&isotp_notifier_lock); }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 * for in-flight isotp_rcv() callers to finish, so this call must not * be skipped just because so->bound is already 0 here.
@@ -1251,13 +1255,10 @@ static int isotp_release(struct socket *sock)hrtimer_cancel (&so->txfrtimer);hrtimer_cancel (&so->txtimer);hrtimer_cancel (&so->rxtimer); - so->ifindex =0 ; - so->bound =0 ; -sock_orphan (sk); sock->sk =NULL ;release_sock (sk);sock_prot_inuse_add (net, sk->sk_prot,-1 );
@@ -1308,10 +1309,11 @@ 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) { err = -EINVAL ; goto out;
@@ -1327,18 +1329,16 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int lif (!dev) { err = -ENODEV ; 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 ; ifindex = dev->ifindex;
@@ -1352,20 +1352,29 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l/* register for echo skb's */ 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 ;if (!sock_flag (sk,SOCK_DEAD ))sk_error_report (sk);
@@ -1564,11 +1573,11 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,struct sock *sk = &so->sk;if (!net_eq (dev_net (dev),sock_net (sk)))return ; -if (so->ifindex != dev->ifindex) +if (so->dev != dev)return ;switch (msg) {case NETDEV_UNREGISTER :lock_sock (sk);
@@ -1580,14 +1589,16 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg, isotp_rcv, sk);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 ;if (!sock_flag (sk,SOCK_DEAD ))sk_error_report (sk);
@@ -1643,10 +1654,11 @@ static int isotp_init(struct sock *sk) {struct isotp_sock *so =isotp_sk (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 ; so->opt.rx_ext_address =CAN_ISOTP_DEFAULT_EXT_ADDRESS ; so->opt.rxpad_content =CAN_ISOTP_DEFAULT_PAD_CONTENT ;
Signature · 1 line
2.53.0