* [PATCH v2] iov_iter: Fix out-of-bound access in iov_iter_advance() @ 2016-04-01 15:02 Takashi Iwai 2016-04-01 17:39 ` Al Viro 0 siblings, 1 reply; 7+ messages in thread From: Takashi Iwai @ 2016-04-01 15:02 UTC (permalink / raw) To: Al Viro; +Cc: Jiri Slaby, Andrew Morton, linux-kernel 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] iov_iter: Fix out-of-bound access in iov_iter_advance() 2016-04-01 15:02 [PATCH v2] iov_iter: Fix out-of-bound access in iov_iter_advance() Takashi Iwai @ 2016-04-01 17:39 ` Al Viro 2016-04-01 18:39 ` Takashi Iwai 0 siblings, 1 reply; 7+ messages in thread From: Al Viro @ 2016-04-01 17:39 UTC (permalink / raw) To: Takashi Iwai; +Cc: Jiri Slaby, Andrew Morton, linux-kernel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] iov_iter: Fix out-of-bound access in iov_iter_advance() 2016-04-01 17:39 ` Al Viro @ 2016-04-01 18:39 ` Takashi Iwai 2016-04-01 19:21 ` Al Viro 0 siblings, 1 reply; 7+ messages in thread From: Takashi Iwai @ 2016-04-01 18:39 UTC (permalink / raw) To: Al Viro; +Cc: Jiri Slaby, Andrew Morton, linux-kernel 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; } ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] iov_iter: Fix out-of-bound access in iov_iter_advance() 2016-04-01 18:39 ` Takashi Iwai @ 2016-04-01 19:21 ` Al Viro 2016-04-01 20:11 ` Takashi Iwai 0 siblings, 1 reply; 7+ messages in thread From: Al Viro @ 2016-04-01 19:21 UTC (permalink / raw) To: Takashi Iwai; +Cc: Jiri Slaby, Andrew Morton, linux-kernel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] iov_iter: Fix out-of-bound access in iov_iter_advance() 2016-04-01 19:21 ` Al Viro @ 2016-04-01 20:11 ` Takashi Iwai 2016-04-07 9:20 ` Takashi Iwai 0 siblings, 1 reply; 7+ messages in thread From: Takashi Iwai @ 2016-04-01 20:11 UTC (permalink / raw) To: Al Viro; +Cc: Jiri Slaby, Andrew Morton, linux-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] iov_iter: Fix out-of-bound access in iov_iter_advance() 2016-04-01 20:11 ` Takashi Iwai @ 2016-04-07 9:20 ` Takashi Iwai 2016-04-14 12:37 ` Takashi Iwai 0 siblings, 1 reply; 7+ messages in thread From: Takashi Iwai @ 2016-04-07 9:20 UTC (permalink / raw) To: Al Viro; +Cc: Jiri Slaby, Andrew Morton, linux-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] iov_iter: Fix out-of-bound access in iov_iter_advance() 2016-04-07 9:20 ` Takashi Iwai @ 2016-04-14 12:37 ` Takashi Iwai 0 siblings, 0 replies; 7+ messages in thread From: Takashi Iwai @ 2016-04-14 12:37 UTC (permalink / raw) To: Al Viro; +Cc: Jiri Slaby, Andrew Morton, linux-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-04-14 12:37 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-04-01 15:02 [PATCH v2] iov_iter: Fix out-of-bound access in iov_iter_advance() Takashi Iwai 2016-04-01 17:39 ` Al Viro 2016-04-01 18:39 ` Takashi Iwai 2016-04-01 19:21 ` Al Viro 2016-04-01 20:11 ` Takashi Iwai 2016-04-07 9:20 ` Takashi Iwai 2016-04-14 12:37 ` Takashi Iwai
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.