Commit 3b23a32a authored by Cong Wang's avatar Cong Wang Committed by David S. Miller
Browse files

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: 3710becf ("net: RCU locking for simple ioctl()")
Reported-by: default avatar"Gong, Sishuai" <sishuai@purdue.edu>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: default avatarCong Wang <cong.wang@bytedance.com>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parent 7867299c

drivers/net/tap.c

+3 −4
Original line number Diff line number Diff line
@@ -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;

drivers/net/tun.c

+2 −3
Original line number Diff line number Diff line
@@ -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:

include/linux/netdevice.h

+3 −0
Original line number Diff line number Diff line
@@ -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);

net/core/dev.c

+42 −0
Original line number Diff line number Diff line
@@ -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

net/core/dev_ioctl.c

+7 −13
Original line number Diff line number Diff line
@@ -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: