[PATCH 1/5] compiler.h, kasan: Avoid duplicating __read_once_size_nocheck()

[PATCH 1/5] compiler.h, kasan: Avoid duplicating __read_once_size_nocheck()

[Andrey Ryabinin]

Instead of having two identical __read_once_size_nocheck() functions
with different attributes, consolidate all the difference in new macro
__no_kasan_or_inline and use it. No functional changes.

Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
 include/linux/compiler.h | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 97847f2f86cf..b8414ecf9ba1 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -190,23 +190,21 @@ void __read_once_size(const volatile void *p, void *res, int size)
 
 #ifdef CONFIG_KASAN
 /*
- * This function is not 'inline' because __no_sanitize_address confilcts
+ * We can't declare function 'inline' because __no_sanitize_address confilcts
  * with inlining. Attempt to inline it may cause a build failure.
  * 	https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
  * '__maybe_unused' allows us to avoid defined-but-not-used warnings.
  */
-static __no_sanitize_address __maybe_unused
-void __read_once_size_nocheck(const volatile void *p, void *res, int size)
-{
-	__READ_ONCE_SIZE;
-}
+# define __no_kasan_or_inline __no_sanitize_address __maybe_unused
 #else
-static __always_inline
+# define __no_kasan_or_inline __always_inline
+#endif
+
+static __no_kasan_or_inline
 void __read_once_size_nocheck(const volatile void *p, void *res, int size)
 {
 	__READ_ONCE_SIZE;
 }
-#endif
 
 static __always_inline void __write_once_size(volatile void *p, void *res, int size)
 {
-- 
2.13.6

[PATCH 2/5] compiler.h: Add read_word_at_a_time() function.

[Andrey Ryabinin]

Sometimes we know that it's safe to do potentially out-of-bounds access
because we know it won't cross a page boundary. Still, KASAN will
report this as a bug.

Add read_word_at_a_time() function which is supposed to be used in such
cases. In read_word_at_a_time() KASAN performs relaxed check - only the
first byte of access is validated.

Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
 include/linux/compiler.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index b8414ecf9ba1..0eb5bb0902e9 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -243,6 +243,7 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
  * required ordering.
  */
 #include <asm/barrier.h>
+#include <linux/kasan-checks.h>
 
 #define __READ_ONCE(x, check)						\
 ({									\
@@ -262,6 +263,13 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
  */
 #define READ_ONCE_NOCHECK(x) __READ_ONCE(x, 0)
 
+static __no_kasan_or_inline
+unsigned long read_word_at_a_time(const void *addr)
+{
+	kasan_check_read(addr, 1);
+	return *(unsigned long *)addr;
+}
+
 #define WRITE_ONCE(x, val) \
 ({							\
 	union { typeof(x) __val; char __c[1]; } __u =	\
-- 
2.13.6

[PATCH 3/5] lib/strscpy: Shut up KASAN false-positives in strscpy()

[Andrey Ryabinin]

strscpy() performs the word-at-a-time optimistic reads. So it may
may access the memory past the end of the object, which is perfectly fine
since strscpy() doesn't use that (past-the-end) data and makes sure the
optimistic read won't cross a page boundary.

Use new read_word_at_a_time() to shut up the KASAN.

Note that this potentially could hide some bugs. In example bellow,
stscpy() will copy more than we should (1-3 extra uninitialized bytes):

        char dst[8];
        char *src;

        src = kmalloc(5, GFP_KERNEL);
        memset(src, 0xff, 5);
        strscpy(dst, src, 8);

Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
 lib/string.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/string.c b/lib/string.c
index 64a9e33f1daa..2c0900a5d51a 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -203,7 +203,7 @@ 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_word_at_a_time(src+res);
 		if (has_zero(c, &data, &constants)) {
 			data = prep_zero_mask(c, data, &constants);
 			data = create_zero_mask(data);
-- 
2.13.6

[PATCH 4/5] fs/dcache: Use read_word_at_a_time() in dentry_string_cmp()

[Andrey Ryabinin]

dentry_string_cmp() performs the word-at-a-time reads from 'cs' and may
read slightly more than it was requested in kmallac(). Normally this
would make KASAN to report out-of-bounds access, but this was workarounded
by commit df4c0e36f1b1 ("fs: dcache: manually unpoison dname after
allocation to shut up kasan's reports").

This workaround is not perfect, since it allows out-of-bounds access
to dentry's name for all the code, not just in dentry_string_cmp().

So it would be better to use read_word_at_a_time() instead and
revert commit df4c0e36f1b1.

Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
 fs/dcache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index c99812fd54b3..195999c8d1b4 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -193,7 +193,7 @@ static inline int dentry_string_cmp(const unsigned char *cs, const unsigned char
 	unsigned long a,b,mask;
 
 	for (;;) {
-		a = *(unsigned long *)cs;
+		a = read_word_at_a_time(cs);
 		b = load_unaligned_zeropad(ct);
 		if (tcount < sizeof(unsigned long))
 			break;
-- 
2.13.6

[PATCH 5/5] fs: dcache: Revert "manually unpoison dname after allocation to shut up kasan's reports"

[Andrey Ryabinin]

This reverts commit df4c0e36f1b1782b0611a77c52cc240e5c4752dd.
It's no longer needed since dentry_string_cmp() now uses read_word_at_a_time()
to avoid kasan's reports.

Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
 fs/dcache.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 195999c8d1b4..b3dc1870caa8 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -37,8 +37,6 @@
 #include <linux/prefetch.h>
 #include <linux/ratelimit.h>
 #include <linux/list_lru.h>
-#include <linux/kasan.h>
-
 #include "internal.h"
 #include "mount.h"
 
@@ -1629,9 +1627,6 @@ struct dentry *__d_alloc(struct super_block *sb, const struct qstr *name)
 		}
 		atomic_set(&p->u.count, 1);
 		dname = p->name;
-		if (IS_ENABLED(CONFIG_DCACHE_WORD_ACCESS))
-			kasan_unpoison_shadow(dname,
-				round_up(name->len + 1,	sizeof(unsigned long)));
 	} else  {
 		dname = dentry->d_iname;
 	}	
-- 
2.13.6

Re: [PATCH 1/5] compiler.h, kasan: Avoid duplicating __read_once_size_nocheck()

[Linus Torvalds]

Ack for the whole series.

Will this go through Andrew's mm tree or what? I can pick it up directly too.

               Linus

On Thu, Feb 1, 2018 at 10:00 AM, Andrey Ryabinin
<aryabinin@virtuozzo.com> wrote:
> Instead of having two identical __read_once_size_nocheck() functions
> with different attributes, consolidate all the difference in new macro
> __no_kasan_or_inline and use it. No functional changes.
>
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>

Re: [PATCH 1/5] compiler.h, kasan: Avoid duplicating __read_once_size_nocheck()

[Andrey Ryabinin]

On 02/01/2018 09:28 PM, Linus Torvalds wrote:
> Ack for the whole series.
> 
> Will this go through Andrew's mm tree or what? I can pick it up directly too.
> 

Whatever you'd prefer.

Re: [PATCH 1/5] compiler.h, kasan: Avoid duplicating __read_once_size_nocheck()

[Linus Torvalds]

On Thu, Feb 1, 2018 at 12:14 PM, Andrey Ryabinin
<aryabinin@virtuozzo.com> wrote:
> On 02/01/2018 09:28 PM, Linus Torvalds wrote:
>>
>> Will this go through Andrew's mm tree or what? I can pick it up directly too.
>
> Whatever you'd prefer.

Ok, I just took it directly as-is,

             Linus