| 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
| ||
|
Message-ID: <20250313203158.45057-1-kuniyu@amazon.com>
Date: Thu, 13 Mar 2025 13:31:34 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <kangyan91@...look.com>
CC: <aleksander.lobakin@...el.com>, <davem@...emloft.net>,
<edumazet@...gle.com>, <florian.fainelli@...adcom.com>, <horms@...nel.org>,
<kory.maincent@...tlin.com>, <kuba@...nel.org>, <kuniyu@...zon.com>,
<linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>,
<pabeni@...hat.com>, <syzkaller@...glegroups.com>
Subject: Re: unregister_netdevice: waiting for DEV to become free [unbalanced refcount bug in dev_ifsioc]
From: YAN KANG <kangyan91@...look.com>
Date: Thu, 13 Mar 2025 16:18:22 +0000
> Dear maintainers,
>
> I found a an unbalanced refcount bug titiled "unregister_netdevice:
> waiting for DEV to become free" while using modified syzkaller fuzzing
> tool. I tested it on the latest Linux upstream version (6.14.0-rc6) .
> I have repro to trigger it.
>
> RootCause Analysis:
> function dev_ifsioc in /net/core/dev_ioctl.c use netdev_hold and
> netdev_put try to balance refcount but meet a concurrent Error.
> case SIOCBRDELIF:
> if (!netif_device_present(dev))
> return -ENODEV;
> if (!netif_is_bridge_master(dev))
> return -EOPNOTSUPP;
>
> netdev_hold(dev, &dev_tracker, GFP_KERNEL); //dev_refcnt += 1
> rtnl_net_unlock(net); //unlock
>
> err = br_ioctl_call(net, netdev_priv(dev), cmd, ifr, NULL); // lock --dosomething ---unlock
>
> netdev_put(dev, &dev_tracker); //dev_refcnt -= 1
> rtnl_net_lock(net);
> return err;
>
> Bug occurs:
>
> Thread 1: Thread 2:
>
> sock_ioctl: sock_ioctl:
> dev_ifsioc:( cmd = SIOCBRDELIF) br_ioctl_call:
> netdev_hold(dev...) br_ioctl_stub: (cmd = SIOCBRDELBR)
> rtnl_net_unlock(net)
> rtnl_unlock() [1]
> rtnl_lock() // lock [2]
> br_del_bridge:
> br_dev_delete:
> unregister_netdevice_queue(br->dev, head); // add dev to net_todo_list
> rtnl_unlock() // trigger NETDEV_UNREGISTER event and check refcount [3]
> refcount != 1 panic !
> ...
> rtnl_net_lock(net)[4]
> netdev_put(dev...)
Thanks for the report !
This looks valid to me.
>
> Fix Suggestion:
> Remove netdev_hold and netdev_put in dev_ifsioc. Because br_ioctl_call will lock and getdev reference by net.
This doesn't work because we can't touch dev without refcnt bumped
after releaseing RTNL. Note that the dev here is the bridge device,
not the slave that is looked up add_del_if().
The fix would be move SIOCBRDELIF into sock_ioctl().
I'll post a patch after checking if there was any reason to put
SIOCBRDELIF in dev_ioctl().
---8<---
diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
index 3ff96ae31bf6..c5fe3b2a53e8 100644
--- a/include/linux/if_bridge.h
+++ b/include/linux/if_bridge.h
@@ -65,11 +65,9 @@ struct br_ip_list {
#define BR_DEFAULT_AGEING_TIME (300 * HZ)
struct net_bridge;
-void brioctl_set(int (*hook)(struct net *net, struct net_bridge *br,
- unsigned int cmd, struct ifreq *ifr,
+void brioctl_set(int (*hook)(struct net *net, unsigned int cmd,
void __user *uarg));
-int br_ioctl_call(struct net *net, struct net_bridge *br, unsigned int cmd,
- struct ifreq *ifr, void __user *uarg);
+int br_ioctl_call(struct net *net, unsigned int cmd, void __user *uarg);
#if IS_ENABLED(CONFIG_BRIDGE) && IS_ENABLED(CONFIG_BRIDGE_IGMP_SNOOPING)
int br_multicast_list_adjacent(struct net_device *dev,
diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c
index f213ed108361..4667635ff588 100644
--- a/net/bridge/br_ioctl.c
+++ b/net/bridge/br_ioctl.c
@@ -394,10 +394,28 @@ static int old_deviceless(struct net *net, void __user *data)
return -EOPNOTSUPP;
}
-int br_ioctl_stub(struct net *net, struct net_bridge *br, unsigned int cmd,
- struct ifreq *ifr, void __user *uarg)
+int br_ioctl_stub(struct net *net, unsigned int cmd, void __user *uarg)
{
int ret = -EOPNOTSUPP;
+ struct ifreq ifr;
+
+ switch (cmd) {
+ case SIOCBRADDIF:
+ case SIOCBRDELIF: {
+ void __user *data;
+ char *colon;
+
+ if (get_user_ifreq(&ifr, &data, uarg))
+ return -EFAULT;
+
+ ifr.ifr_name[IFNAMSIZ - 1] = 0;
+ colon = strchr(ifr.ifr_name, ':');
+ if (colon)
+ *colon = 0;
+
+ dev_load(net, ifr.ifr_name);
+ }
+ }
rtnl_lock();
@@ -430,9 +448,24 @@ int br_ioctl_stub(struct net *net, struct net_bridge *br, unsigned int cmd,
break;
case SIOCBRADDIF:
case SIOCBRDELIF:
- ret = add_del_if(br, ifr->ifr_ifindex, cmd == SIOCBRADDIF);
+ {
+ struct net_device *dev;
+
+ dev = __dev_get_by_name(net, ifr.ifr_name);
+ if (!dev || !netif_device_present(dev)) {
+ ret = -ENODEV;
+ break;
+ }
+
+ if (!netif_is_bridge_master(dev)) {
+ ret = -EOPNOTSUPP;
+ break;
+ }
+
+ ret = add_del_if(netdev_priv(dev), ifr.ifr_ifindex, cmd == SIOCBRADDIF);
break;
}
+ }
rtnl_unlock();
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 1054b8a88edc..d5b3c5936a79 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -949,8 +949,7 @@ br_port_get_check_rtnl(const struct net_device *dev)
/* br_ioctl.c */
int br_dev_siocdevprivate(struct net_device *dev, struct ifreq *rq,
void __user *data, int cmd);
-int br_ioctl_stub(struct net *net, struct net_bridge *br, unsigned int cmd,
- struct ifreq *ifr, void __user *uarg);
+int br_ioctl_stub(struct net *net, unsigned int cmd, void __user *uarg);
/* br_multicast.c */
#ifdef CONFIG_BRIDGE_IGMP_SNOOPING
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 4c2098ac9d72..57f79f8e8466 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -551,7 +551,6 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,
int err;
struct net_device *dev = __dev_get_by_name(net, ifr->ifr_name);
const struct net_device_ops *ops;
- netdevice_tracker dev_tracker;
if (!dev)
return -ENODEV;
@@ -614,22 +613,6 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,
case SIOCWANDEV:
return dev_siocwandev(dev, &ifr->ifr_settings);
- case SIOCBRADDIF:
- case SIOCBRDELIF:
- if (!netif_device_present(dev))
- return -ENODEV;
- if (!netif_is_bridge_master(dev))
- return -EOPNOTSUPP;
-
- netdev_hold(dev, &dev_tracker, GFP_KERNEL);
- rtnl_net_unlock(net);
-
- err = br_ioctl_call(net, netdev_priv(dev), cmd, ifr, NULL);
-
- netdev_put(dev, &dev_tracker);
- rtnl_net_lock(net);
- return err;
-
case SIOCDEVPRIVATE ... SIOCDEVPRIVATE + 15:
return dev_siocdevprivate(dev, ifr, data, cmd);
@@ -812,8 +795,6 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr,
case SIOCBONDRELEASE:
case SIOCBONDSETHWADDR:
case SIOCBONDCHANGEACTIVE:
- case SIOCBRADDIF:
- case SIOCBRDELIF:
case SIOCSHWTSTAMP:
if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
return -EPERM;
diff --git a/net/socket.c b/net/socket.c
index 28bae5a94234..ad1f89b6ddf9 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1145,12 +1145,10 @@ static ssize_t sock_write_iter(struct kiocb *iocb, struct iov_iter *from)
*/
static DEFINE_MUTEX(br_ioctl_mutex);
-static int (*br_ioctl_hook)(struct net *net, struct net_bridge *br,
- unsigned int cmd, struct ifreq *ifr,
+static int (*br_ioctl_hook)(struct net *net, unsigned int cmd,
void __user *uarg);
-void brioctl_set(int (*hook)(struct net *net, struct net_bridge *br,
- unsigned int cmd, struct ifreq *ifr,
+void brioctl_set(int (*hook)(struct net *net, unsigned int cmd,
void __user *uarg))
{
mutex_lock(&br_ioctl_mutex);
@@ -1159,8 +1157,7 @@ void brioctl_set(int (*hook)(struct net *net, struct net_bridge *br,
}
EXPORT_SYMBOL(brioctl_set);
-int br_ioctl_call(struct net *net, struct net_bridge *br, unsigned int cmd,
- struct ifreq *ifr, void __user *uarg)
+int br_ioctl_call(struct net *net, unsigned int cmd, void __user *uarg)
{
int err = -ENOPKG;
@@ -1169,7 +1166,7 @@ int br_ioctl_call(struct net *net, struct net_bridge *br, unsigned int cmd,
mutex_lock(&br_ioctl_mutex);
if (br_ioctl_hook)
- err = br_ioctl_hook(net, br, cmd, ifr, uarg);
+ err = br_ioctl_hook(net, cmd, uarg);
mutex_unlock(&br_ioctl_mutex);
return err;
@@ -1269,7 +1266,9 @@ static long sock_ioctl(struct file *file, unsigned cmd, unsigned long arg)
case SIOCSIFBR:
case SIOCBRADDBR:
case SIOCBRDELBR:
- err = br_ioctl_call(net, NULL, cmd, NULL, argp);
+ case SIOCBRADDIF:
+ case SIOCBRDELIF:
+ err = br_ioctl_call(net, cmd, argp);
break;
case SIOCGIFVLAN:
case SIOCSIFVLAN:
---8<---
Powered by blists - more mailing lists