linux-kernel - Re: [PATCH] lib/string: avoid reading beyond src buffer in strscpy

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]

Message-ID: <46584b52-f2f2-a602-1ae6-cfa0e321324a@virtuozzo.com>
Date:   Mon, 11 Dec 2017 19:44:16 +0300
From:   Andrey Ryabinin <aryabinin@...tuozzo.com>
To:     Kees Cook <keescook@...omium.org>,
        Dmitry Vyukov <dvyukov@...gle.com>
Cc:     Eryu Guan <eguan@...hat.com>, LKML <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Chris Metcalf <cmetcalf@...hip.com>,
        Alexander Potapenko <glider@...gle.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH] lib/string: avoid reading beyond src buffer in strscpy

On 12/08/2017 11:54 PM, Kees Cook wrote:
> On Fri, Dec 8, 2017 at 7:29 AM, Dmitry Vyukov <dvyukov@...gle.com> wrote:
>> On Fri, Dec 8, 2017 at 4:29 PM, Andrey Ryabinin <aryabinin@...tuozzo.com> wrote:
>>>
>>> So, possible solutions are:
>>>
>>> 1) Simply disable word-at-a-time optimization in strscpy(). I seriously doubt
>>> that this optimization have noticeable performance impact in real workloads.
>>>
>>> 2) Apply patch https://lkml.kernel.org/r/20170718171523.32208-1-aryabinin@virtuozzo.com
>>> It's a simple, minimally intrusive way to fix the bug.
>>> However, the patch changes semantics/implementation of the strscpy() which is bad idea.
>>> It basically means that we use one implementation of the strscpy() but test with KASAN another implementation.
>>> For that reason patch wasn't liked by Linus.
>>>
>>> 3) Introduce read_once_partial_check() function and use it in strscpy().
>>> read_once_partial_check() supposed to read one word, but KASAN will check only the first byte of the access.
>>> Dmitry didn't like this. I'm also not fan of this solution, because we may not notice some real bugs, e.g.:
>>>
>>>         char dst[8];
>>>         char *src;
>>>
>>>         src = kmalloc(5, GFP_KERNEL);
>>>         memset(src, 0xff, 5);
>>>         strscpy(dst, src, 8);
>>>
>>> stscpy() will copy from 6 up to 8 bytes (exact size depends on what is stored in src[5-7])
>>> from non-null terminated src. With read_once_partial_check() used in strscpy() KASAN will
>>> not report such bug.
>>>
>>>
>>> So, what it's gonna be? Let's finally decide something and deal with the problem.
>>> My vote belongs to 1) or 2).
>>
>>
>> My vote is for 1) if we agree that the optimization is not worth it,
>> otherwise for 2).
> 
> Who would be the best person to measure the difference for 1)?
> 

I suppose that depends on which one strscpy() caller you'd want to test.
Briefly looking at all current users, it doesn't look like they process huge amounts
of data through strscpy(), thus we shouldn't suffer from a slight
performance degradation of strscpy().