netdev - Re: [syzbot] [net?] KASAN: slab-use-after-free Read in __ethtool_get_link

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: <20240929114601.1584-1-hdanton@sina.com>
Date: Sun, 29 Sep 2024 19:46:01 +0800
From: Hillf Danton <hdanton@...a.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: Denis Kirjanov <dkirjanov@...e.de>,
	syzbot <syzbot+5fe14f2ff4ccbace9a26@...kaller.appspotmail.com>,
	linux-kernel@...r.kernel.org,
	Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
	Boqun Feng <boqun.feng@...il.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	netdev@...r.kernel.org,
	syzkaller-bugs@...glegroups.com
Subject: Re: [syzbot] [net?] KASAN: slab-use-after-free Read in __ethtool_get_link_ksettings

On Sat, 28 Sep 2024 20:21:12 +0800 Hillf Danton <hdanton@...a.com>
> On Mon, 25 Mar 2024 14:08:36 +0100 Eric Dumazet <edumazet@...gle.com>
> > On Mon, Mar 25, 2024 at 1:10 PM Denis Kirjanov <dkirjanov@...e.de> wrote:
> > >
> > > Hmm, report says that we have a net_device freed even that we have a dev_hold()
> > > before __ethtool_get_link_ksettings()
> > 
> > dev_hold(dev) might be done too late, the device is already being dismantled.
> > 
> > ib_device_get_netdev() should probably be done under RTNL locking,
> > otherwise the final part is racy :
> > 
> > if (res && res->reg_state != NETREG_REGISTERED) {
> >      dev_put(res);
> >      return NULL;
> > }
> 
> Given paranoia in netdev_run_todo(),
> 
> 		/* paranoia */
> 		BUG_ON(netdev_refcnt_read(dev) != 1);
> 
> the claim that dev_hold(dev) might be done too late could not explain
> the success of checking NETREG_REGISTERED, because of checking 
> NETREG_UNREGISTERING after rcu barrier.
> 
> 	/* Wait for rcu callbacks to finish before next phase */
> 	if (!list_empty(&list))
> 		rcu_barrier();
> 
> 	list_for_each_entry_safe(dev, tmp, &list, todo_list) {
> 		if (unlikely(dev->reg_state != NETREG_UNREGISTERING)) {
> 			netdev_WARN(dev, "run_todo but not unregistering\n");
> 			list_del(&dev->todo_list);
> 			continue;
> 		}
> 
As simply bumping kref up could survive the syzbot reproducer [1], Eric's reclaim
is incorrect.

--- l/drivers/infiniband/core/verbs.c
+++ v/drivers/infiniband/core/verbs.c
@@ -1979,6 +1979,7 @@ int ib_get_eth_speed(struct ib_device *d
 	netdev = ib_device_get_netdev(dev, port_num);
 	if (!netdev)
 		return -ENODEV;
+	dev_hold(netdev);
 
 	rtnl_lock();
 	rc = __ethtool_get_link_ksettings(netdev, &lksettings);
@@ -1995,6 +1996,7 @@ int ib_get_eth_speed(struct ib_device *d
 				netdev->name, netdev_speed);
 	}
 
+	dev_put(netdev);
 	ib_get_width_and_speed(netdev_speed, lksettings.lanes,
 			       speed, width);
 
--
syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-by: syzbot+5fe14f2ff4ccbace9a26@...kaller.appspotmail.com
Tested-by: syzbot+5fe14f2ff4ccbace9a26@...kaller.appspotmail.com

[1] https://lore.kernel.org/lkml/66f9372f.050a0220.aab67.001a.GAE@google.com/