On Fri, Apr 01, 2016 at 08:39:19PM +0200, Takashi Iwai wrote: > > /* Get packet from user space buffer */ > static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, > void *msg_control, struct iov_iter *from, > int noblock) > { > .... > struct virtio_net_hdr gso = { 0 }; > ....
Here len must be equal to iov_iter_count(from). > if (tun->flags & IFF_VNET_HDR) { > if (len < tun->vnet_hdr_sz) > return -EINVAL; ... and be at least tun->vnet_hdr_sz > len -= tun->vnet_hdr_sz; > > n = copy_from_iter(&gso, sizeof(gso), from); > if (n != sizeof(gso)) > return -EFAULT; We'd consumed sizeof(gso) > if ((gso.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) && > tun16_to_cpu(tun, gso.csum_start) + tun16_to_cpu(tun, > gso.csum_offset) + 2 > tun16_to_cpu(tun, gso.hdr_len)) > gso.hdr_len = cpu_to_tun16(tun, tun16_to_cpu(tun, > gso.csum_start) + tun16_to_cpu(tun, gso.csum_offset) + 2); > > if (tun16_to_cpu(tun, gso.hdr_len) > len) > return -EINVAL; > ==> iov_iter_advance(from, tun->vnet_hdr_sz - sizeof(gso)); ... and skipped tun->vnet_hdr_sz - sizeof(gso). How the hell can that overrun the end of iterator? Is ->vnet_hdr_sz less that struct virtio_net_hdr somehow? > So, tun_get_user() calls copy_from_iter(), and the iterator is > advanced, and call iov_iter_advance() from that point for the rest > size. And this size can be zero or greater. We can put simply a zero > check there, and actually Jiri suggested it at first. > Hm, so do you mean that it's invalid to call this function with > size=0? Or shouldn't we return the actually advanced size? Currently > the function assumes the size suffices implicitly. Zero is certainly valid. But note that if _that_ is what you are concerned about, the warning is not serious. Look: #define iterate_iovec(i, n, __v, __p, skip, STEP) { \ n is 0 size_t left; \ size_t wanted = n; \ __p = i->iov; \ __v.iov_len = min(n, __p->iov_len - skip); \ min(0, some unsigned crap) => 0. if (likely(__v.iov_len)) { \ not taken __v.iov_base = __p->iov_base + skip; \ left = (STEP); \ __v.iov_len -= left; \ skip += __v.iov_len; \ n -= __v.iov_len; \ } else { \ left = 0; \ } \ while (unlikely(!left && n)) { \ never executed __p++; \ __v.iov_len = min(n, __p->iov_len); \ if (unlikely(!__v.iov_len)) \ continue; \ __v.iov_base = __p->iov_base; \ left = (STEP); \ __v.iov_len -= left; \ skip = __v.iov_len; \ n -= __v.iov_len; \ } \ n = wanted - n; \ 0 is stored in n again, no-op } with similar working for kvec and bvec cases. IF the warning is actually about zero-length case, it's a red herring. Yes, in theory the array of iovec/kvec/bvec might reach the end of a page, with the next one not being mapped at all. In that case we would oops there, and I'm fine with adding if (!n) return; there. However, I'm _not_ OK with the first part - there we would be papering over a real bug in the caller.