| Message ID | 20250101091008.27533-1-kuniyu@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v1,net] ipvlan: Fix use-after-free in ipvlan_get_iflink(). | expand |
On Wed, 1 Jan 2025 18:10:08 +0900 Kuniyuki Iwashima wrote: > syzbot presented a use-after-free report [0] regarding ipvlan and > linkwatch. > > ipvlan does not hold a refcnt of the lower device. > > When the linkwatch work is triggered for the ipvlan dev, the lower > dev might have already been freed. > > Let's hold the lower dev's refcnt in dev->netdev_ops->ndo_init() > and release it in dev->priv_destructor() as done for vlan and macvlan. Hmmm, random ndo calls after unregister_netdevice() has returned are very error prone, if we can address this in the core - I think that's better. Perhaps we could take Eric's commit 750e51603395 ("net: avoid potential UAF in default_operstate()") even further? If the device is unregistering we can just assume DOWN. And we can use RCU protection to make sure the unregistration doesn't race with us? Just to give the idea (not even compile tested): diff --git a/net/core/link_watch.c b/net/core/link_watch.c index 1b4d39e38084..985273bc7c0d 100644 --- a/net/core/link_watch.c +++ b/net/core/link_watch.c @@ -42,14 +42,20 @@ static unsigned int default_operstate(const struct net_device *dev) * first check whether lower is indeed the source of its down state. */ if (!netif_carrier_ok(dev)) { - int iflink = dev_get_iflink(dev); struct net_device *peer; + int iflink; /* If called from netdev_run_todo()/linkwatch_sync_dev(), * dev_net(dev) can be already freed, and RTNL is not held. */ - if (dev->reg_state == NETREG_UNREGISTERED || - iflink == dev->ifindex) + rcu_read_lock(); + if (dev->reg_state <= NETREG_REGISTERED) + iflink = dev_get_iflink(dev); + else + iflink = dev->ifindex; + rcu_read_unlock(); + + if (iflink == dev->ifindex) return IF_OPER_DOWN; ASSERT_RTNL();
From: Jakub Kicinski <kuba@kernel.org> Date: Thu, 2 Jan 2025 17:44:00 -0800 > On Wed, 1 Jan 2025 18:10:08 +0900 Kuniyuki Iwashima wrote: > > syzbot presented a use-after-free report [0] regarding ipvlan and > > linkwatch. > > > > ipvlan does not hold a refcnt of the lower device. > > > > When the linkwatch work is triggered for the ipvlan dev, the lower > > dev might have already been freed. > > > > Let's hold the lower dev's refcnt in dev->netdev_ops->ndo_init() > > and release it in dev->priv_destructor() as done for vlan and macvlan. > > Hmmm, random ndo calls after unregister_netdevice() has returned > are very error prone, if we can address this in the core - I think > that's better. > > Perhaps we could take Eric's commit 750e51603395 ("net: avoid potential > UAF in default_operstate()") even further? > > If the device is unregistering we can just assume DOWN. And we can use > RCU protection to make sure the unregistration doesn't race with us? Sounds good to me. Will post v2, thanks! > Just to give the idea (not even compile tested): > > diff --git a/net/core/link_watch.c b/net/core/link_watch.c > index 1b4d39e38084..985273bc7c0d 100644 > --- a/net/core/link_watch.c > +++ b/net/core/link_watch.c > @@ -42,14 +42,20 @@ static unsigned int default_operstate(const struct net_device *dev) > * first check whether lower is indeed the source of its down state. > */ > if (!netif_carrier_ok(dev)) { > - int iflink = dev_get_iflink(dev); > struct net_device *peer; > + int iflink; > > /* If called from netdev_run_todo()/linkwatch_sync_dev(), > * dev_net(dev) can be already freed, and RTNL is not held. > */ > - if (dev->reg_state == NETREG_UNREGISTERED || > - iflink == dev->ifindex) > + rcu_read_lock(); > + if (dev->reg_state <= NETREG_REGISTERED) > + iflink = dev_get_iflink(dev); > + else > + iflink = dev->ifindex; > + rcu_read_unlock(); > + > + if (iflink == dev->ifindex) > return IF_OPER_DOWN; > > ASSERT_RTNL();
diff --git a/drivers/net/ipvlan/ipvlan.h b/drivers/net/ipvlan/ipvlan.h index 025e0c19ec25..3d22e0c7805d 100644 --- a/drivers/net/ipvlan/ipvlan.h +++ b/drivers/net/ipvlan/ipvlan.h @@ -70,6 +70,7 @@ struct ipvl_dev { netdev_features_t sfeatures; u32 msg_enable; spinlock_t addrs_lock; + netdevice_tracker dev_tracker; }; struct ipvl_addr { diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c index ee2c3cf4df36..3566b21f49d5 100644 --- a/drivers/net/ipvlan/ipvlan_main.c +++ b/drivers/net/ipvlan/ipvlan_main.c @@ -160,6 +160,9 @@ static int ipvlan_init(struct net_device *dev) } port = ipvlan_port_get_rtnl(phy_dev); port->count += 1; + + netdev_hold(phy_dev, &ipvlan->dev_tracker, GFP_KERNEL); + return 0; } @@ -669,6 +672,13 @@ void ipvlan_link_delete(struct net_device *dev, struct list_head *head) } EXPORT_SYMBOL_GPL(ipvlan_link_delete); +static void ipvlan_link_destruct(struct net_device *dev) +{ + struct ipvl_dev *ipvlan = netdev_priv(dev); + + netdev_put(ipvlan->phy_dev, &ipvlan->dev_tracker); +} + void ipvlan_link_setup(struct net_device *dev) { ether_setup(dev); @@ -678,6 +688,7 @@ void ipvlan_link_setup(struct net_device *dev) dev->priv_flags |= IFF_UNICAST_FLT | IFF_NO_QUEUE; dev->netdev_ops = &ipvlan_netdev_ops; dev->needs_free_netdev = true; + dev->priv_destructor = ipvlan_link_destruct; dev->header_ops = &ipvlan_header_ops; dev->ethtool_ops = &ipvlan_ethtool_ops; }