[RFC] kasan stack overflow warnings again: READ_ONCE(), typecheck()

* [RFC] kasan stack overflow warnings again: READ_ONCE(), typecheck()
@ 2017-02-14 23:03 Arnd Bergmann
  2017-02-15  9:18 ` Dmitry Vyukov
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Arnd Bergmann @ 2017-02-14 23:03 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: kasan-dev, Andrey Ryabinin, Alexander Potapenko, linux-kernel,
	Christian Borntraeger

Hi Dmitry,

I've come a little further on the stack size analysis after initially
finding that gcc-7.0.1 is much better than the horrible stack frames
we were seeing on gcc-7.0.0 (I found over 80kb in one case), I found
more that remain with the newer compiler, but also analyzed the worst
remaining ones down to two common helpers: typecheck() caused the
worst remaining problem, but only in a few files like
"drivers/net/wireless/ralink/rt2x00/rt2800lib.c:5068:1: error: the frame
size of 23768 bytes is larger than 1024 bytes". I think my fix should
be able to make it in, but I'd give it some more testing. It also seems
here that gcc asan-stack isn't too smart, as it adds checks to local
variables we never use except for comparing their addresses. This may
also impact min() and max(), which have the same check.

READ_ONCE()/WRITE_ONCE() are used for atomic_t and caused the next worst
offender (12688 byte stacks in lib/atomic64_test.c) as well as a lot
of other instances that were over 2048 byte stacks. This one is a lot
trickier as my the version from my patch is most likely not safe
for all callers, and the helper has been extended to cover a lot of
corner cases that my version destroys (it doesn't force aggregates
to be accessed as scalars for example, probably also causes sparse
warnings, and maybe doesn't even guarantee the "ONCE" semantics).

I see that Andrey also provided a READ_ONCE_NOCHECK() helper that
would also take care of this if used consistently, but it seems
wrong to use that for all atomics. Any other ideas?

     Arnd

diff --git a/arch/x86/include/asm/switch_to.h b/arch/x86/include/asm/switch_to.h
index fcc5cd387fd1..0c243dd569fe 100644
--- a/arch/x86/include/asm/switch_to.h
+++ b/arch/x86/include/asm/switch_to.h
@@ -30,7 +30,7 @@ static inline void prepare_switch_to(struct task_struct *prev,
 	 *
 	 * To minimize cache pollution, just follow the stack pointer.
 	 */
-	READ_ONCE(*(unsigned char *)next->thread.sp);
+	(void)READ_ONCE(*(unsigned char *)next->thread.sp);
 #endif
 }
 
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 01157d6e8cfe..dc677c7c22be 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -222,8 +222,8 @@ void ovl_dentry_update(struct dentry *dentry, struct dentry *upperdentry)
 
 void ovl_inode_init(struct inode *inode, struct inode *realinode, bool is_upper)
 {
-	WRITE_ONCE(inode->i_private, (unsigned long) realinode |
-		   (is_upper ? OVL_ISUPPER_MASK : 0));
+	WRITE_ONCE(inode->i_private, (void *)((unsigned long) realinode |
+		   (is_upper ? OVL_ISUPPER_MASK : 0)));
 }
 
 void ovl_inode_update(struct inode *inode, struct inode *upperinode)
@@ -231,7 +231,7 @@ void ovl_inode_update(struct inode *inode, struct inode *upperinode)
 	WARN_ON(!upperinode);
 	WARN_ON(!inode_unhashed(inode));
 	WRITE_ONCE(inode->i_private,
-		   (unsigned long) upperinode | OVL_ISUPPER_MASK);
+		   (void *)((unsigned long) upperinode | OVL_ISUPPER_MASK));
 	if (!S_ISDIR(upperinode->i_mode))
 		__insert_inode_hash(inode, (unsigned long) upperinode);
 }
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 416b17e03016..b8018eddd757 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -310,14 +310,17 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
  */
 
 #define __READ_ONCE(x, check)						\
-({									\
-	union { typeof(x) __val; char __c[1]; } __u;			\
+	__builtin_choose_expr(sizeof(x) == 1, *(volatile typeof(&(x)))&(x), \
+	__builtin_choose_expr(sizeof(x) == 2, *(volatile typeof(&(x)))&(x), \
+	__builtin_choose_expr(sizeof(x) == 4, *(volatile typeof(&(x)))&(x), \
+	__builtin_choose_expr(sizeof(x) == sizeof(long), *(volatile typeof(&(x)))&(x), \
+	({union { typeof(x) __val; char __c[1]; } __u;			\
 	if (check)							\
 		__read_once_size(&(x), __u.__c, sizeof(x));		\
 	else								\
 		__read_once_size_nocheck(&(x), __u.__c, sizeof(x));	\
 	__u.__val;							\
-})
+	})))))
 #define READ_ONCE(x) __READ_ONCE(x, 1)
 
 /*
@@ -326,13 +329,19 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
  */
 #define READ_ONCE_NOCHECK(x) __READ_ONCE(x, 0)
 
-#define WRITE_ONCE(x, val) \
-({							\
-	union { typeof(x) __val; char __c[1]; } __u =	\
-		{ .__val = (__force typeof(x)) (val) }; \
-	__write_once_size(&(x), __u.__c, sizeof(x));	\
-	__u.__val;					\
-})
+#define WRITE_ONCE(x, val)								\
+(											\
+	__builtin_choose_expr(sizeof(x) == 1, *(volatile typeof(&(x)))&(x) = (val),	\
+	__builtin_choose_expr(sizeof(x) == 2, *(volatile typeof(&(x)))&(x) = (val),	\
+	__builtin_choose_expr(sizeof(x) == 4, *(volatile typeof(&(x)))&(x) = (val),	\
+	__builtin_choose_expr(sizeof(x) == sizeof(long), *(volatile typeof(&(x)))&(x) = (val),	\
+	({										\
+		typeof(x) __val = (val);						\
+		barrier();								\
+		__builtin_memcpy((void *)&x, (void *)&__val, sizeof(__val));			\
+		barrier();								\
+	})))))			\
+)
 
 #endif /* __KERNEL__ */
 
diff --git a/include/linux/typecheck.h b/include/linux/typecheck.h
index eb5b74a575be..adb1579fa5f0 100644
--- a/include/linux/typecheck.h
+++ b/include/linux/typecheck.h
@@ -5,12 +5,7 @@
  * Check at compile time that something is of a particular type.
  * Always evaluates to 1 so you may use it easily in comparisons.
  */
-#define typecheck(type,x) \
-({	type __dummy; \
-	typeof(x) __dummy2; \
-	(void)(&__dummy == &__dummy2); \
-	1; \
-})
+#define typecheck(type,x) ({(void)((typeof(type) *)NULL == (typeof(x) *)NULL); 1;})
 
 /*
  * Check at compile time that 'function' is a certain type, or is a pointer

^ permalink raw reply related	[flat|nested] 9+ messages in thread