linux-kernel - Re: [PATCH v2] iov_iter: Fix out-of-bound access in iov_iter

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 1 Apr 2016 18:39:20 +0100
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Takashi Iwai <tiwai@...e.de>
Cc:	Jiri Slaby <jslaby@...e.cz>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] iov_iter: Fix out-of-bound access in
 iov_iter_advance()

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.