lkml.org 
[lkml]   [2010]   [Nov]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    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

    \
     
     \ /
      Last update: 2010-11-01 09:31    [W:0.021 / U:91.164 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site