Re: [PATCH 5.10 031/102] net: fix dev_ifsioc_locked() race condition
From: Pavel Machek <hidden>
Date: 2021-03-08 12:52:08
Also in:
lkml
Subsystem:
networking [general], the rest · Maintainers:
"David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Linus Torvalds
Hi!
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
I guess it may fix a race, but... does it leak kernel stack data to userland?
quoted hunk ↗ jump to hunk
+++ b/drivers/net/tap.c@@ -1093,10 +1093,9 @@ static long tap_ioctl(struct file *file, 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();
We copy whole "struct sockaddr".
+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;...
+ 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));
But we only coppied dev->addr_len bytes in. This would be very simple way to plug the leak. Signed-off-by: Pavel Machek (CIP) <redacted>
diff --git a/net/core/dev.c b/net/core/dev.c
index 75ca6c6d01d6..b67ff23a1f0d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c@@ -8714,11 +8714,9 @@ int dev_get_mac_address(struct sockaddr *sa, struct net *net, char *dev_name) 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)); + memset(sa->sa_data, 0, size); + memcpy(sa->sa_data, dev->dev_addr, + min_t(size_t, size, dev->addr_len)); sa->sa_family = dev->type; unlock:
Best regards, Pavel -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Attachments
- signature.asc [application/pgp-signature] 195 bytes