From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755356AbcDGJUJ (ORCPT ); Thu, 7 Apr 2016 05:20:09 -0400 Received: from mx2.suse.de ([195.135.220.15]:56009 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750785AbcDGJUF (ORCPT ); Thu, 7 Apr 2016 05:20:05 -0400 Date: Thu, 07 Apr 2016 11:20:02 +0200 Message-ID: From: Takashi Iwai To: Al Viro Cc: Jiri Slaby , Andrew Morton , linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] iov_iter: Fix out-of-bound access in iov_iter_advance() In-Reply-To: References: <1459522924-17720-1-git-send-email-tiwai@suse.de> <20160401173919.GC17997@ZenIV.linux.org.uk> <20160401192105.GD17997@ZenIV.linux.org.uk> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/24.5 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. thanks, Takashi