On 07/18/2017 08:22 PM, Linus Torvalds wrote: > On Tue, Jul 18, 2017 at 10:15 AM, Andrey Ryabinin > <aryabi...@virtuozzo.com> wrote: >> >> + /* >> + * KASAN won't be happy about word-at-a-time >> + * optimistic reads, so let's avoid them. >> + */ >> + if (IS_ENABLED(CONFIG_KASAN)) >> + max = 0; >> + > > No, please don't. > > Two reasons: > > (a) it turns out that KASAN doesn't actually warn about this when > there aren't buggy users (because we only do word-at-a-time in the > spacified-to-be-safe region anyway). >
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)): Currently KASAN doesn't complain about strscpy() only because KASAN doesn't complain about unused code. And after 077d2ba519b2e8bf1ab("replace incorrect strscpy use in FORTIFY_SOURCE") or before the FORTIFY_SOURCE patch strscpy() is pretty much unused. Only 2 calls in some drivers, plus 3 in tile arch-code. > (b) havign automated testing that then just changes semantics and > implementation of what is tested is a bad bad bad idea. > I agree, but what choice do we have here? > So (a) says that we shouldn't need it in the first place, and (b) says > that we should avoid KASAN changing behavior unless we absolutely > *have* to. > (a) is wrong. I absolutely agree with (b) and I think that this is exactly the case where we have to do this. > In fact, I think we should *never* have that kind of "KASAN changes > semantics". If there is some particular load that is known to be > problematic for KASAN, we *still* shouldn't change semantics, we > should just mark that single load as being unchecked by KASAN. For strscpy() that would mean making the *whole* read from 'src' buffer unchecked by KASAN. Although, we can always check the 'src' afterwards. But honestly, this looks shitty: --- lib/string.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/string.c b/lib/string.c index ebbb99c775bd..5624b629bffa 100644 --- a/lib/string.c +++ b/lib/string.c @@ -23,6 +23,7 @@ #include <linux/string.h> #include <linux/ctype.h> #include <linux/kernel.h> +#include <linux/kasan-checks.h> #include <linux/export.h> #include <linux/bug.h> #include <linux/errno.h> @@ -202,12 +203,15 @@ ssize_t strscpy(char *dest, const char *src, size_t count) while (max >= sizeof(unsigned long)) { unsigned long c, data; - c = *(unsigned long *)(src+res); + c = READ_ONCE_NOCHECK(*(unsigned long *)(src+res)); if (has_zero(c, &data, &constants)) { data = prep_zero_mask(c, data, &constants); data = create_zero_mask(data); *(unsigned long *)(dest+res) = c & zero_bytemask(data); - return res + find_zero(data); + + res = res + find_zero(data); + kasan_check_read(src, res); + return res; } *(unsigned long *)(dest+res) = c; res += sizeof(unsigned long); @@ -215,6 +219,7 @@ ssize_t strscpy(char *dest, const char *src, size_t count) max -= sizeof(unsigned long); } + kasan_check_read(src, res); while (count) { char c; -- 2.13.0