netdev - Re: [PATCH v1 net] ipvlan: Fix use-after-free in ipvlan_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250103133749.76202-1-kuniyu@amazon.com>
Date: Fri, 3 Jan 2025 22:37:49 +0900
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <kuba@...nel.org>
CC: <andrew+netdev@...n.ch>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<horms@...nel.org>, <kuni1840@...il.com>, <kuniyu@...zon.com>,
	<maheshb@...gle.com>, <netdev@...r.kernel.org>, <pabeni@...hat.com>,
	<syzkaller@...glegroups.com>
Subject: Re: [PATCH v1 net] ipvlan: Fix use-after-free in ipvlan_get_iflink().

From: Jakub Kicinski <kuba@...nel.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();