Messages in this thread Patch in this message | | | Date | Mon, 1 Nov 2010 10:27:49 +0200 | From | "Michael S. Tsirkin" <> | 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@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
| |