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]

Date:   Wed, 15 Feb 2017 23:19:12 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     Christian Borntraeger <borntraeger@...ibm.com>
Cc:     Dmitry Vyukov <dvyukov@...gle.com>,
        "kasan-dev@...glegroups.com" <kasan-dev@...glegroups.com>,
        Andrey Ryabinin <aryabinin@...tuozzo.com>,
        Alexander Potapenko <glider@...gle.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [RFC] kasan stack overflow warnings again: READ_ONCE(), typecheck()

On Wed, Feb 15, 2017 at 5:20 PM, Christian Borntraeger
<borntraeger@...ibm.com> wrote:
> On 02/15/2017 05:18 PM, Christian Borntraeger wrote:
>> On 02/15/2017 12:03 AM, Arnd Bergmann wrote:
>>>
>>> -#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),  \
>>
>> Have you run sparse on those changes?
>> IIRC we had to add the __force to get rid of address space annotations
>> in that macro above. Cannot tell if we need something like that here.

I haven't tried that yet, and I'm not overly worried about sparse, we can always
work around sparse using a different implementation of the macro
behind an #ifdef
if we can't easily fix that.

The hard part is coming up with a version that GCC won't turn into horrible
object code when building with ASAN, and that probably requires leaving out
the local variable.

I'm still struggling to understand what commit ab3f02fc237211 ("locking/arch:
Add WRITE_ONCE() to set_mb()") was for, and whether we should worry about
unaligned destinations trapping when we do a word-sized store to a location
that possibly has a smaller alignment.

> The original warning was
>   fs/afs/inode.c:448:9: sparse: incorrect type in initializer (different address spaces)
>   fs/afs/inode.c:448:9:    expected struct afs_permits *__val
>   fs/afs/inode.c:448:9:    got void [noderef] <asn:4>*<noident>

Is this just for the __rcu annotation in rcu_assign_pointer? WRITE_ONCE
should not care about that at all, but the caller should do it here, as this is
the place where a normal pointer becomes an __rcu pointer.

     Arnd