linux-kernel - Re: [PATCH 5.10 031/102] net: fix dev_ifsioc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210308125057.GA19538@duo.ucw.cz>
Date:   Mon, 8 Mar 2021 13:50:57 +0100
From:   Pavel Machek <pavel@...x.de>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     linux-kernel@...r.kernel.org, stable@...r.kernel.org,
        "Gong, Sishuai" <sishuai@...due.edu>,
        Eric Dumazet <eric.dumazet@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Cong Wang <cong.wang@...edance.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH 5.10 031/102] net: fix dev_ifsioc_locked() race condition

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

Download attachment "signature.asc" of type "application/pgp-signature" (196 bytes)