On Thu, 07 Apr 2016 11:20:02 +0200, Takashi Iwai wrote: > > On Fri, 01 Apr 2016 22:11:11 +0200, > Takashi Iwai wrote: > > > > On Fri, 01 Apr 2016 21:21:05 +0200, > > Al Viro wrote: > > > > > > 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? > > > > The bug is really well hidden, and I also didn't realize until Jiri > > spotted it. Actually, the iterator doesn't overrun. By the first > > copy_from_iter() call, iov reaches exactly at the end. Then it calls > > iov_iter_advance() with size 0. Now, what happens is... > > > > > > > > > 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); \ > > > > ... here __p->io_vlen is read, and __p (= iov) had already reached at > > the end. So this read will become out of bounce. > > > > > > > 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. > > > > The bug is about calling with zero length, yes, and triggered only at > > the end boundary. > > > > Of course, it can be fixed in the caller side. But I'm not sure which > > is better in this particular case. The call itself looks valid as an > > iterator POV, after all... > > Al, was my previous post clarifying enough? > > If you still prefer fixing in tun driver side, let me know. I'll cook > up the patch.
Any update on this? thanks, Takashi