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

Al Viro
2016-04-01T19:21:09Z

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?

> 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); \

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.