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 1f4bdd94407a9dda66f8c4249bfac67fca9b7e76..f549d3a8e59c0380c7f2d375d88900bf422b4675 100644 (file)
--- a/drivers/net/tap.c
+++ b/drivers/net/tap.c
@@ -1093,10 +1093,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();
@@ -1111,7 +1110,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 1ac80756e5afa5caaa293ccb77629f90d221f78f..accde25a66a01b284e7e8af595b337c7440885fe 100644 (file)
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -3157,15 +3157,14 @@ 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;
 
        case SIOCSIFHWADDR:
                /* Set hw address */
-               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 9de5312edeb861f7e233f95a25fd99cf730eb6ee..8753e98a8d58a79eb4fb25b839e35d121916394d 100644 (file)
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3879,6 +3879,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 210d0fce58e177f6e2d7535dfc2b1d61531120f8..75ca6c6d01d6eda7461c639bdd91b0c0caed830d 100644 (file)
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -8686,6 +8686,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 205e92e604ef7ef04fc8379789554cfb6308b9ef..54fb18b4f55e49a910b7b5931366ccba3e2397d2 100644 (file)
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -123,17 +123,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;
@@ -274,7 +263,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)
@@ -418,6 +407,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.
@@ -427,7 +422,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 7d72236917839f50687c8abbcdefb605cd6ce430..eae8e87930cd7587c2bbbb863c0c7a15012f720d 100644 (file)
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2658,7 +2658,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;