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

[Al Viro <viro@ZenIV.linux.org.uk>]

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.