[5.10,031/102] net: fix dev_ifsioc_locked() race condition

From: Cong Wang <cong.wang@bytedance.com>

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
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: 3710becf8a58 ("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>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/net/tap.c         |    7 +++----
 drivers/net/tun.c         |    5 ++---
 include/linux/netdevice.h |    3 +++
 net/core/dev.c            |   42 ++++++++++++++++++++++++++++++++++++++++++
 net/core/dev_ioctl.c      |   20 +++++++-------------
 net/core/rtnetlink.c      |    2 +-
 6 files changed, 58 insertions(+), 21 deletions(-)

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@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

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@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:

> 


Please submit this change properly to the networking developers, they
are not going to pick anything up this way.

greg k-h