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

[Kuniyuki Iwashima <kuniyu@amazon.com>]

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