netdev - [PATCH RFC] tun: remove of user-controlled memory allocation

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-next>] [day] [month] [year] [list]

Message-ID: <20101101082749.GA25860@redhat.com>
Date:	Mon, 1 Nov 2010 10:27:49 +0200
From:	"Michael S. Tsirkin" <mst@...hat.com>
To:	unlisted-recipients:; (no To-header on input)
Cc:	"David S. Miller" <davem@...emloft.net>,
	"Michael S. Tsirkin" <mst@...hat.com>,
	Herbert Xu <herbert@...dor.hengli.com.au>,
	Eric Dumazet <eric.dumazet@...il.com>,
	Joe Perches <joe@...ches.com>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: [PATCH RFC] tun: remove of user-controlled memory allocation

Untested, this is just an RFC.

tun does a kmalloc where userspace controls the length. This will
produce warnings in kernel log when the length is too large, or might
block for a long while. A simple fix is to avoid the allocatiuon
altogether, and copy from user in a loop.

However, with this patch an illegal address passed to the ioctl might
leave the filter disabled.  Is this something we care about?  If
yes we could recover by creating a copy of the filter.  Thoughts?

Signed-off-by: Michael S. Tsirkin <mst@...hat.com>
---
 drivers/net/tun.c |   30 ++++++++++++++----------------
 1 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 55f3a3e..ea36888 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -220,28 +220,23 @@ static unsigned int addr_hash_test(const u32 *mask, const u8 *addr)
 
 static int update_filter(struct tap_filter *filter, void __user *arg)
 {
-	struct { u8 u[ETH_ALEN]; } *addr;
+	struct { u8 u[ETH_ALEN]; } __user *addr;
 	struct tun_filter uf;
-	int err, alen, n, nexact;
+	int err = -EFAULT, n, nexact;
 
 	if (copy_from_user(&uf, arg, sizeof(uf)))
-		return -EFAULT;
+		goto done;
 
 	if (!uf.count) {
 		/* Disabled */
 		filter->count = 0;
-		return 0;
+		err = 0;
+		goto done;
 	}
 
-	alen = ETH_ALEN * uf.count;
-	addr = kmalloc(alen, GFP_KERNEL);
-	if (!addr)
-		return -ENOMEM;
-
-	if (copy_from_user(addr, arg + sizeof(uf), alen)) {
-		err = -EFAULT;
+	addr = arg + sizeof(uf);
+	if (!access_ok(VERIFY_READ, addr, ETH_ALEN * uf.count))
 		goto done;
-	}
 
 	/* The filter is updated without holding any locks. Which is
 	 * perfectly safe. We disable it first and in the worst
@@ -251,7 +246,8 @@ static int update_filter(struct tap_filter *filter, void __user *arg)
 
 	/* Use first set of addresses as an exact filter */
 	for (n = 0; n < uf.count && n < FLT_EXACT_COUNT; n++)
-		memcpy(filter->addr[n], addr[n].u, ETH_ALEN);
+		if (__copy_from_user(filter->addr[n], addr[n].u, ETH_ALEN))
+			goto done;
 
 	nexact = n;
 
@@ -259,11 +255,14 @@ static int update_filter(struct tap_filter *filter, void __user *arg)
 	 * unicast will leave the filter disabled. */
 	memset(filter->mask, 0, sizeof(filter->mask));
 	for (; n < uf.count; n++) {
-		if (!is_multicast_ether_addr(addr[n].u)) {
+		u8 u[ETH_ALEN];
+		if (__copy_from_user(u, addr[n].u, ETH_ALEN))
+			goto done;
+		if (!is_multicast_ether_addr(u)) {
 			err = 0; /* no filter */
 			goto done;
 		}
-		addr_hash_set(filter->mask, addr[n].u);
+		addr_hash_set(filter->mask, u);
 	}
 
 	/* For ALLMULTI just set the mask to all ones.
@@ -279,7 +278,6 @@ static int update_filter(struct tap_filter *filter, void __user *arg)
 	err = nexact;
 
 done:
-	kfree(addr);
 	return err;
 }
 
-- 
1.7.3.2.91.g446ac
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html