linux-kernel - Re: [RFC] kasan stack overflow warnings again: READ

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: <36862803-f365-6ab4-198e-e858f41ee914@virtuozzo.com>
Date:   Wed, 15 Feb 2017 17:06:53 +0300
From:   Andrey Ryabinin <aryabinin@...tuozzo.com>
To:     Arnd Bergmann <arnd@...db.de>, Dmitry Vyukov <dvyukov@...gle.com>
CC:     "kasan-dev@...glegroups.com" <kasan-dev@...glegroups.com>,
        Alexander Potapenko <glider@...gle.com>,
        <linux-kernel@...r.kernel.org>,
        Christian Borntraeger <borntraeger@...ibm.com>
Subject: Re: [RFC] kasan stack overflow warnings again: READ_ONCE(),
 typecheck()



On 02/15/2017 02:03 AM, Arnd Bergmann wrote:
> 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,

I think the following patch on top of yours should fix that for READ_ONCE().
WRITE_ONCE() probably can be fixed in a similar way, but I didn't try it.

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 10bca12..5d9dd63 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -301,10 +301,10 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
  */
 
 #define __READ_ONCE(x, check)						\
-	__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), \
+	__builtin_choose_expr(sizeof(x) == 1, (typeof(x))(__u64)*(volatile __u8 *)&(x), \
+	__builtin_choose_expr(sizeof(x) == 2, (typeof(x))(__u64)*(volatile __u16 *)&(x), \
+	__builtin_choose_expr(sizeof(x) == 4, (typeof(x))(__u64)*(volatile __u32 *)&(x), \
+	__builtin_choose_expr(sizeof(x) == 8, (typeof(x))(__u64)*(volatile __u64 *)&(x), \
 	({union { typeof(x) __val; char __c[1]; } __u;			\
 	if (check)							\
 		__read_once_size(&(x), __u.__c, sizeof(x));		\


> 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/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;			\

This should be under "if (check)" otherwise it breaks READ_ONCE_NOCHECK()

>  	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)
>