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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250102174400.085fd8ac@kernel.org>
Date: Thu, 2 Jan 2025 17:44:00 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: "David S. Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Andrew Lunn
 <andrew+netdev@...n.ch>, Simon Horman <horms@...nel.org>, Mahesh Bandewar
 <maheshb@...gle.com>, Kuniyuki Iwashima <kuni1840@...il.com>,
 <netdev@...r.kernel.org>, syzkaller <syzkaller@...glegroups.com>
Subject: Re: [PATCH v1 net] ipvlan: Fix use-after-free in
 ipvlan_get_iflink().

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