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

[Takashi Iwai]

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...


thanks,

Takashi