net: fix dev_ifsioc_locked() race condition

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: b98453b8f3f7 ("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>

diff --git a/drivers/net/tap.c b/drivers/net/tap.c
index ff4aa35979a19aa7b07ca9e70a6e46f2127ae027..8e3a28ba6b28238d88dddd9104921c86e49fff87 100644 (file)
--- a/drivers/net/tap.c
+++ b/drivers/net/tap.c
@@ -1090,10 +1090,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();
@@ -1108,7 +1107,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 62690baa19bc8c4bf52f8b18a092f570e2125fc8..fc86da7f1628f728778d8401b59eef638e60e738 100644 (file)
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -3107,15 +3107,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 a20310ff5083b7dcb7bb1c2c6268888894086579..bfadf3b82f9c585bb3310ae6cf32e76a38a74e9f 100644 (file)
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3902,6 +3902,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 321d41a110e723e962cbf7e78141161c9b0d5532..ce6291bc2e166a4f05c6a9d25266b24c650f934f 100644 (file)
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -8937,6 +8937,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 db8a0ff86f366f0422b452bf312e2f2f712669d1..478d032f34acaa80bce0e306c5f4eff01f4dd2b4 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 c313aaf2bce1b5b1157bdcbee79eff1c066581da..0edc0b2baaa433938883bee99ae90c9fca9663bd 100644 (file)
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2660,7 +2660,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;