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

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

[Michael S. Tsirkin]

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

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

[David Miller]

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Mon, 1 Nov 2010 10:27:49 +0200

> 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@redhat.com>

I think the key issue in situations like this is simply to make
sure that reasonable things that worked before, still do afterwards.

And I think your patch does that, so it's fine as far as I can tell.