net: fix dev_ifsioc_locked() race condition

commit 3b23a32a63219f51a5298bc55a65ecee866e79d0 upstream.

dev_ifsioc_locked() is called with only RCU read lock, so when
there is a parallel writer changing the mac address, it could
get a partially updated mac address, as shown below:

Thread 1			Thread 2
// eth_commit_mac_addr_change()
memcpy(dev->dev_addr, addr->sa_data, ETH_ALEN);
				// dev_ifsioc_locked()
				memcpy(ifr->ifr_hwaddr.sa_data,
					dev->dev_addr,...);

Close this race condition by guarding them with a RW semaphore,
like netdev_get_name(). We can not use seqlock here as it does not
allow blocking. The writers already take RTNL anyway, so this does
not affect the slow path. To avoid bothering existing
dev_set_mac_address() callers in drivers, introduce a new wrapper
just for user-facing callers on ioctl and rtnetlink paths.

Note, bonding also changes slave mac addresses but that requires
a separate patch due to the complexity of bonding code.

Fixes: 3710becf8a58 ("net: RCU locking for simple ioctl()")
Reported-by: "Gong, Sishuai" <sishuai@purdue.edu>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Cong Wang <cong.wang@bytedance.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/drivers/net/tap.c b/drivers/net/tap.c
index 3ae70c7e6860ca4653ed698fca9bdd2ad8f2c037..f285422a8071734e01711fc40ff5997e52df07f5 100644 (file)
--- a/drivers/net/tap.c
+++ b/drivers/net/tap.c
@@ -1095,10 +1095,9 @@ static long tap_ioctl(struct file *file, unsigned int cmd,
                        return -ENOLINK;
                }
                ret = 0;
-               u = tap->dev->type;
+               dev_get_mac_address(&sa, dev_net(tap->dev), tap->dev->name);
                if (copy_to_user(&ifr->ifr_name, tap->dev->name, IFNAMSIZ) ||
-                   copy_to_user(&ifr->ifr_hwaddr.sa_data, tap->dev->dev_addr, ETH_ALEN) ||
-                   put_user(u, &ifr->ifr_hwaddr.sa_family))
+                   copy_to_user(&ifr->ifr_hwaddr, &sa, sizeof(sa)))
                        ret = -EFAULT;
                tap_put_tap_dev(tap);
                rtnl_unlock();
@@ -1113,7 +1112,7 @@ static long tap_ioctl(struct file *file, unsigned int cmd,
                        rtnl_unlock();
                        return -ENOLINK;
                }
-               ret = dev_set_mac_address(tap->dev, &sa, NULL);
+               ret = dev_set_mac_address_user(tap->dev, &sa, NULL);
                tap_put_tap_dev(tap);
                rtnl_unlock();
                return ret;

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 84e779f93f0a4da943b594d78d9e6fb51f1cf74f..fe6ec22bf3d5180ca18d816fe8d13a5ef7fff35a 100644 (file)
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -3224,7 +3224,7 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
        case SIOCGIFHWADDR:
                /* Get hw address */
                memcpy(ifr.ifr_hwaddr.sa_data, tun->dev->dev_addr, ETH_ALEN);
-               ifr.ifr_hwaddr.sa_family = tun->dev->type;
+               dev_get_mac_address(&ifr.ifr_hwaddr, net, tun->dev->name);
                if (copy_to_user(argp, &ifr, ifreq_len))
                        ret = -EFAULT;
                break;
@@ -3234,7 +3234,7 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
                tun_debug(KERN_DEBUG, tun, "set hw address: %pM\n",
                          ifr.ifr_hwaddr.sa_data);
 
-               ret = dev_set_mac_address(tun->dev, &ifr.ifr_hwaddr, NULL);
+               ret = dev_set_mac_address_user(tun->dev, &ifr.ifr_hwaddr, NULL);
                break;
 
        case TUNGETSNDBUF:

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 3e0692fd6282c52d1721faa935499ae8cad904ff..db1b9623977c81ca802f3192aabbe5dae5b274a7 100644 (file)
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3679,6 +3679,9 @@ int dev_pre_changeaddr_notify(struct net_device *dev, const char *addr,
                              struct netlink_ext_ack *extack);
 int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
                        struct netlink_ext_ack *extack);
+int dev_set_mac_address_user(struct net_device *dev, struct sockaddr *sa,
+                            struct netlink_ext_ack *extack);
+int dev_get_mac_address(struct sockaddr *sa, struct net *net, char *dev_name);
 int dev_change_carrier(struct net_device *, bool new_carrier);
 int dev_get_phys_port_id(struct net_device *dev,
                         struct netdev_phys_item_id *ppid);

diff --git a/net/core/dev.c b/net/core/dev.c
index d90e8bd87df8335eefb4d106e1698bbe9de02b90..e732faade5dca9f3ba6255b4ee99d5df7bc8c925 100644 (file)
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -8144,6 +8144,48 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
 }
 EXPORT_SYMBOL(dev_set_mac_address);
 
+static DECLARE_RWSEM(dev_addr_sem);
+
+int dev_set_mac_address_user(struct net_device *dev, struct sockaddr *sa,
+                            struct netlink_ext_ack *extack)
+{
+       int ret;
+
+       down_write(&dev_addr_sem);
+       ret = dev_set_mac_address(dev, sa, extack);
+       up_write(&dev_addr_sem);
+       return ret;
+}
+EXPORT_SYMBOL(dev_set_mac_address_user);
+
+int dev_get_mac_address(struct sockaddr *sa, struct net *net, char *dev_name)
+{
+       size_t size = sizeof(sa->sa_data);
+       struct net_device *dev;
+       int ret = 0;
+
+       down_read(&dev_addr_sem);
+       rcu_read_lock();
+
+       dev = dev_get_by_name_rcu(net, dev_name);
+       if (!dev) {
+               ret = -ENODEV;
+               goto unlock;
+       }
+       if (!dev->addr_len)
+               memset(sa->sa_data, 0, size);
+       else
+               memcpy(sa->sa_data, dev->dev_addr,
+                      min_t(size_t, size, dev->addr_len));
+       sa->sa_family = dev->type;
+
+unlock:
+       rcu_read_unlock();
+       up_read(&dev_addr_sem);
+       return ret;
+}
+EXPORT_SYMBOL(dev_get_mac_address);
+
 /**
  *     dev_change_carrier - Change device carrier
  *     @dev: device

diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 5163d900bb4fb1057e273ed667e9457f05b33c93..69fb9219d51dd8aab222095e583598c7b586301b 100644 (file)
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -122,17 +122,6 @@ static int dev_ifsioc_locked(struct net *net, struct ifreq *ifr, unsigned int cm
                ifr->ifr_mtu = dev->mtu;
                return 0;
 
-       case SIOCGIFHWADDR:
-               if (!dev->addr_len)
-                       memset(ifr->ifr_hwaddr.sa_data, 0,
-                              sizeof(ifr->ifr_hwaddr.sa_data));
-               else
-                       memcpy(ifr->ifr_hwaddr.sa_data, dev->dev_addr,
-                              min(sizeof(ifr->ifr_hwaddr.sa_data),
-                                  (size_t)dev->addr_len));
-               ifr->ifr_hwaddr.sa_family = dev->type;
-               return 0;
-
        case SIOCGIFSLAVE:
                err = -EINVAL;
                break;
@@ -246,7 +235,7 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, unsigned int cmd)
        case SIOCSIFHWADDR:
                if (dev->addr_len > sizeof(struct sockaddr))
                        return -EINVAL;
-               return dev_set_mac_address(dev, &ifr->ifr_hwaddr, NULL);
+               return dev_set_mac_address_user(dev, &ifr->ifr_hwaddr, NULL);
 
        case SIOCSIFHWBROADCAST:
                if (ifr->ifr_hwaddr.sa_family != dev->type)
@@ -396,6 +385,12 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr, bool *need_c
         */
 
        switch (cmd) {
+       case SIOCGIFHWADDR:
+               dev_load(net, ifr->ifr_name);
+               ret = dev_get_mac_address(&ifr->ifr_hwaddr, net, ifr->ifr_name);
+               if (colon)
+                       *colon = ':';
+               return ret;
        /*
         *      These ioctl calls:
         *      - can be done by all.
@@ -405,7 +400,6 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr, bool *need_c
        case SIOCGIFFLAGS:
        case SIOCGIFMETRIC:
        case SIOCGIFMTU:
-       case SIOCGIFHWADDR:
        case SIOCGIFSLAVE:
        case SIOCGIFMAP:
        case SIOCGIFINDEX:

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index b0c06a063776589d5b79f46a5687ddc2ee03fe25..73c09b5864d703577852c2ab366b3241d834efdb 100644 (file)
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2471,7 +2471,7 @@ static int do_setlink(const struct sk_buff *skb,
                sa->sa_family = dev->type;
                memcpy(sa->sa_data, nla_data(tb[IFLA_ADDRESS]),
                       dev->addr_len);
-               err = dev_set_mac_address(dev, sa, extack);
+               err = dev_set_mac_address_user(dev, sa, extack);
                kfree(sa);
                if (err)
                        goto errout;