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

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

[Takashi Iwai]

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.

  BUG: KASAN: stack-out-of-bounds in iov_iter_advance+0x510/0x540 at addr ffff88003d5efd40
  Read of size 8 by task syz-executor/22356
  page:ffffea0000f57bc0 count:0 mapcount:0 mapping:          (null) index:0x0
  flags: 0x1fffff80000000()
  page dumped because: kasan: bad access detected
  CPU: 0 PID: 22356 Comm: syz-executor Tainted: G        W   E      4.4.6-0-default #1
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.8.1-0-g4adadbd-20151112_172657-sheep25 04/01/2014
   0000000000000000 ffff88003d5ef9d0 ffffffff819f42c1 ffff88003d5efa68
   ffff88003d5efd40 0000000000000000 ffff88003d5efd38 ffff88003d5efa58
   ffffffff815f7267 000000000000000a ffff88003d5efad8 0000000000000296
  Call Trace:
   [<ffffffff819f42c1>] ? dump_stack+0xb3/0x112
   [<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]
   [<ffffffff812791f0>] ? debug_check_no_locks_freed+0x290/0x290
   [<ffffffffa0e084d0>] ? tun_select_queue+0x370/0x370 [tun]
   [<ffffffff81329559>] ? futex_wake+0x149/0x420
   [<ffffffff812c9027>] ? debug_lockdep_rcu_enabled+0x77/0x90
   [<ffffffffa0e03895>] ? __tun_get+0x5/0x220 [tun]
   [<ffffffffa0e039b1>] ? __tun_get+0x121/0x220 [tun]
   [<ffffffffa0e0a88a>] ? tun_chr_write_iter+0xda/0x190 [tun]
   [<ffffffff8164841a>] ? __vfs_write+0x30a/0x480
   [<ffffffff81648110>] ? vfs_iter_write+0x320/0x320
   [<ffffffff812c9027>] ? debug_lockdep_rcu_enabled+0x77/0x90
   [<ffffffff818c1448>] ? common_file_perm+0x158/0x7a0
   [<ffffffff818c1cb7>] ? apparmor_file_permission+0x27/0x30
   [<ffffffff81648fa5>] ? rw_verify_area+0x105/0x2f0
   [<ffffffff8164960c>] ? vfs_write+0x16c/0x4a0
   [<ffffffff8164c29a>] ? SyS_write+0x11a/0x230

This patch adds the proper check of the size to iov_iter_advance(),
like all other functions calling iterate_and_advance() macro.

Reported-by: Jiri Slaby <jslaby@suse.cz>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---

We can put these checks in iterate_and_advance(), too.  I chose this
patch since it's smaller, and doing in the macro will be a bit ugly.
Let me know if you prefer another option. 

v1->v2: Fix the bogus return value

 lib/iov_iter.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 5fecddc32b1b..2545c31fa0de 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -508,6 +508,10 @@ EXPORT_SYMBOL(iov_iter_copy_from_user_atomic);
 
 void iov_iter_advance(struct iov_iter *i, size_t size)
 {
+	if (unlikely(size > i->count))
+		size = i->count;
+	if (unlikely(!size))
+		return;
 	iterate_and_advance(i, size, v, 0, 0, 0)
 }
 EXPORT_SYMBOL(iov_iter_advance);
-- 
2.7.4

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

[Al Viro]

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.

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

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

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.

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

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

[Al Viro]

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.

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

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

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

[Takashi Iwai]

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