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

[Andrey Ryabinin]

On 12/08/2017 11:54 PM, Kees Cook wrote:
> On Fri, Dec 8, 2017 at 7:29 AM, Dmitry Vyukov <dvyukov@xxxxxxxxxx> wrote:
>> On Fri, Dec 8, 2017 at 4:29 PM, Andrey Ryabinin <aryabinin@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx
>>> 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().