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

[Andrey Ryabinin]

On 12/07/2017 09:26 PM, Kees Cook wrote:
> On Thu, Dec 7, 2017 at 3:33 AM, Eryu Guan <eguan@xxxxxxxxxx> wrote:
>> strscpy() tries to copy sizeof(unsigned long) bytes a time from src
>> to dest when possible, and stops the loop when 'max' is less than
>> sizeof(unsigned long). But it doesn't check if (src+res) goes beyond
>> src buffer and does out-of-bound access to the underlying memory.
>>
>> KASAN reported global-out-of-bound bug when reading seccomp
>> actions_logged file in procfs:
>>
>>   cat /proc/sys/kernel/seccomp/actions_logged
>>
>> Because seccomp_names_from_actions_logged() is copying short strings
>> (less than sizeof(unsigned long)) to buffer 'names'. e.g.
>>
>>   ret = strscpy(names, " ", size);
> 
> This is a false positive:
> https://marc.info/?l=linux-kernel&m=150768944030805&w=2
> 
> Given that we keep getting these reports (this is the third), I wonder
> if can adjust the seccomp code to work around the bug in KASAN...
> 

We should fix this in strscpy() somehow. We just need to decide how to do this.
Last time we haven't came to agreement.

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).