Re: [PATCH v2] iov_iter: Fix out-of-bound access in iov_iter_advance()

[Takashi Iwai]

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