On Mon, Mar 08, 2021 at 01:50:57PM +0100, Pavel Machek wrote:
> 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) <pavel@xxxxxxx>
>
> 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:
>
Please submit this change properly to the networking developers, they
are not going to pick anything up this way.
greg k-h
