github-mirror/linux
1
0
Fork 0
mirror of https://github.com/torvalds/linux.git synced 2026-07-27 17:47:41 +02:00
Code Issues Actions Packages Projects Releases Wiki Activity Graph

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

Browse Source 
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: e057dd3fc2 ("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>
This commit is contained in:
Oliver Hartkopp 2026-07-12 19:59:42 +02:00 committed by Marc Kleine-Budde
parent 9b1a02e0d9
commit 20bab8b88b
1 changed files with 58 additions and 27 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

85
net/can/isotp.c
View File

@ -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;
/* 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);
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);
}
}
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;