tun: remove of user-controlled memory allocation
Subsystems (3)
RFC PATCH 1/1 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?
--- drivers/net/tun.c | 30 ++++++++++++++---------------- 1 files changed, 14 insertions(+), 16 deletions(-)
drivers/net/tun.c +14 -16
@@ -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; }
Signature · 1 line
1.7.3.2.91.g446ac
David Miller
maintainerFrom: "Michael S. Tsirkin" <[email protected]>
Date: Mon, 1 Nov 2010 10:27:49 +0200
Quoted 7 lines
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.
Quoted 5 lines
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 <[email protected]>
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.