Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1369408 > unrolled thread
| Started by | Takashi Iwai <tiwai@suse.de> |
|---|---|
| First post | 2016-04-01 17:10 +0200 |
| Last post | 2016-04-01 22:20 +0200 |
| Articles | 5 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH v2] iov_iter: Fix out-of-bound access in iov_iter_advance() Takashi Iwai <tiwai@suse.de> - 2016-04-01 17:10 +0200
Re: [PATCH v2] iov_iter: Fix out-of-bound access in iov_iter_advance() Al Viro <viro@ZenIV.linux.org.uk> - 2016-04-01 19:40 +0200
Re: [PATCH v2] iov_iter: Fix out-of-bound access in iov_iter_advance() Takashi Iwai <tiwai@suse.de> - 2016-04-01 20:40 +0200
Re: [PATCH v2] iov_iter: Fix out-of-bound access in iov_iter_advance() Al Viro <viro@ZenIV.linux.org.uk> - 2016-04-01 21:30 +0200
Re: [PATCH v2] iov_iter: Fix out-of-bound access in iov_iter_advance() Takashi Iwai <tiwai@suse.de> - 2016-04-01 22:20 +0200
| From | Takashi Iwai <tiwai@suse.de> |
|---|---|
| Date | 2016-04-01 17:10 +0200 |
| Subject | [PATCH v2] iov_iter: Fix out-of-bound access in iov_iter_advance() |
| Message-ID | <rj8RZ-6cF-29@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2016-04-01 19:40 +0200 |
| Subject | Re: [PATCH v2] iov_iter: Fix out-of-bound access in iov_iter_advance() |
| Message-ID | <rjbd9-7LP-11@gated-at.bofh.it> |
| In reply to | #1369408 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Takashi Iwai <tiwai@suse.de> |
|---|---|
| Date | 2016-04-01 20:40 +0200 |
| Message-ID | <rjc9c-8p1-23@gated-at.bofh.it> |
| In reply to | #1369499 |
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;
}
[toc] | [prev] | [next] | [standalone]
| From | Al Viro <viro@ZenIV.linux.org.uk> |
|---|---|
| Date | 2016-04-01 21:30 +0200 |
| Subject | Re: [PATCH v2] iov_iter: Fix out-of-bound access in iov_iter_advance() |
| Message-ID | <rjcVA-wc-7@gated-at.bofh.it> |
| In reply to | #1369518 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Takashi Iwai <tiwai@suse.de> |
|---|---|
| Date | 2016-04-01 22:20 +0200 |
| Message-ID | <rjdHY-15p-1@gated-at.bofh.it> |
| In reply to | #1369551 |
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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web