Re: [PATCH] lib/strscpy: avoid KASAN false positive

[Andrey Ryabinin <aryabinin@virtuozzo.com>]

On 07/18/2017 11:26 PM, Linus Torvalds wrote:
> On Tue, Jul 18, 2017 at 1:15 PM, Andrey Ryabinin
> <aryabinin@virtuozzo.com> wrote:
>>
>> No, it does warn about valid users. The report that Dave posted wasn't about wrong strscpy() usage
>> it was about reading 8-bytes from 5-bytes source string. It wasn't about buggy 'count' at all.
>> So KASAN will warn for perfectly valid code like this:
>>         char dest[16];
>>         strscpy(dest, "12345", sizeof(dest)):
> 
> Ugh, ok, yes.
> 
>> For strscpy() that would mean making the *whole* read from 'src' buffer unchecked by KASAN.
> 
> So we do have that READ_ONCE_NOCHECK(), but could we perhaps have
> something that doesn't do a NOCHECK but a partial check and is simply
> ok with "this is an optimistc longer access"
> 

This can be dont, I think.

Something like this:
static inline unsigned long read_partial_nocheck(unsigned long *x)
{
	unsigned long ret = READ_ONCE_NOCHECK(x);
	kasan_check_partial(x, sizeof(unsigned long));
	return ret;
}

Where kasan_check_partial() will warn only if the kasan shadow has a negative value.
Positive values 1-7 in the shadow would mean a partial oob access, that should be ignored.


> We have that for the dcache case too, although there the code does
> that odd kasan_unpoison_shadow() instead.
>

Yes, it marks memory as valid, so it can be accessed without warning.
We can do this the same in strscpy(), but the disadvantage of this approach is that
memory becomes accessible for everyone, not just for the code that does optimistic reads.
Thus it would be possible to miss some off-by-ones (quite usual bug for strings).