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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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
<[email protected]> 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 <[email protected]>
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.
On Thu, Feb 1, 2018 at 12:14 PM, Andrey Ryabinin
<[email protected]> 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