Re: [PATCH v1 net] ipvlan: Fix use-after-free in ipvlan_get_iflink().

[Jakub Kicinski <kuba@kernel.org>]

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();