| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 14 Apr 2016 14:37:12 +0200
From: Takashi Iwai <tiwai@...e.de>
To: Al Viro <viro@...IV.linux.org.uk>
Cc: Jiri Slaby <jslaby@...e.cz>,
Andrew Morton <akpm@...ux-foundation.org>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] iov_iter: Fix out-of-bound access in iov_iter_advance()
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
Powered by blists - more mailing lists