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

[Takashi Iwai]

On Fri, 01 Apr 2016 19:39:20 +0200,
Al Viro wrote:
> 
> On Fri, Apr 01, 2016 at 05:02:04PM +0200, Takashi Iwai wrote:
> > Currently, iov_iter_advance() just calls iterate_and_advance() macro
> > as is, even if size=0 is passed.  Usually it is OK to pass size=0 to
> > the macro.  However, when the iov_iter has been already advanced to
> > the end of the array, it may lead to an out-of-bound access, since the
> > macro always reads the length of the vector at first.  This bug is
> > actually seen via KASAN with net tun driver, for example.
> 
> FWIW, I think it's better dealt with in callers - almost all such cases
> are signs of bugs in the calling code and quietly hiding them is not
> going to fix the underlying bugs.

Yes, that's another way.  I thought that making this function a bit
more robust would be good, though, as all other callers of the same
macro have the same size checks.

> 
> >    [<ffffffff815f7267>] ? kasan_report_error+0x507/0x540
> >    [<ffffffff8157359f>] ? __might_fault+0x3f/0x50
> >    [<ffffffff815f73d3>] ? __asan_report_load8_noabort+0x43/0x50
> >    [<ffffffff81a30660>] ? iov_iter_advance+0x510/0x540
> >    [<ffffffff81a30660>] ? iov_iter_advance+0x510/0x540
> >    [<ffffffffa0e08c15>] ? tun_get_user+0x745/0x21a0 [tun]
> 
> So tun_get_user() has a problem.

It's in the following code:

/* 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 };
....
	if (tun->flags & IFF_VNET_HDR) {
		if (len < tun->vnet_hdr_sz)
			return -EINVAL;
		len -= tun->vnet_hdr_sz;

		n = copy_from_iter(&gso, sizeof(gso), from);
		if (n != sizeof(gso))
			return -EFAULT;

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

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.


> > This patch adds the proper check of the size to iov_iter_advance(),
> > like all other functions calling iterate_and_advance() macro.
> 
> NAK.  If anything, turn that check into WARN_ON() to make sure it isn't
> missed.

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.

> And tun_get_user() does seem to have a problem - I would like to see
> a reproducer, but it looks like some in the code that decides whether
> to use zerocopy mechanism and I'm not at all sure that this change
> (i.e. silently limit the amount we are advancing for) would end up
> doing the right thing.

The reproducer code is below.  It's just open the tun device, sets it
up, and sends small bytes.  It doesn't crash, but KASAN could detect
the error.


thanks,

Takashi

---
#include <err.h>
#include <fcntl.h>
#include <stdio.h>
#include <unistd.h>

#include <sys/ioctl.h>
#include <sys/socket.h>
#include <sys/stat.h>
#include <sys/types.h>

#include <linux/if.h>
#include <linux/if_tun.h>

int main()
{
	struct ifreq ifreq = {
		.ifr_ifrn.ifrn_name = "kill",
		.ifr_ifru.ifru_flags = IFF_NO_PI | IFF_VNET_HDR | IFF_TUN,
	};
	char blank[10] = {};
	int r0;

	r0 = open("/dev/net/tun", O_RDWR);
	if (r0 < 0)
		err(1, "open");

	if (ioctl(r0, TUNSETIFF, &ifreq) < 0)
		err(1, "ioctl");

	if (write(r0, blank, sizeof(blank)) < 0)
		err(1, "write");

	close(r0);

	return 0;
}