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?
> +++ 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) <pa...@denx.de>
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
signature.asc
Description: PGP signature
