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

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: <96a21da7-1258-0ada-298f-e0388849402a@virtuozzo.com>
Date:   Wed, 19 Jul 2017 00:31:36 +0300
From:   Andrey Ryabinin <aryabinin@...tuozzo.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Dave Jones <davej@...emonkey.org.uk>,
        Alexander Potapenko <glider@...gle.com>,
        Dmitry Vyukov <dvyukov@...gle.com>, kasan-dev@...glegroups.com,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] lib/strscpy: avoid KASAN false positive

On 07/18/2017 11:26 PM, Linus Torvalds wrote:
> On Tue, Jul 18, 2017 at 1:15 PM, Andrey Ryabinin
> <aryabinin@...tuozzo.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).