linux-kernel - Re: [PATCH v6 0/4] x86, kasan: add KASAN checks to atomic operations

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: <CACT4Y+YjtCPq+OEWfw3E1ahttef+AkT3RTMxWvYX3mq8B-FCBA@mail.gmail.com>
Date:   Mon, 26 Feb 2018 13:52:10 +0100
From:   Dmitry Vyukov <dvyukov@...gle.com>
To:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>
Cc:     Ingo Molnar <mingo@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        "H. Peter Anvin" <hpa@...or.com>,
        Andrey Ryabinin <aryabinin@...tuozzo.com>,
        kasan-dev <kasan-dev@...glegroups.com>,
        "the arch/x86 maintainers" <x86@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Will Deacon <will.deacon@....com>
Subject: Re: [PATCH v6 0/4] x86, kasan: add KASAN checks to atomic operations

On Tue, Feb 20, 2018 at 11:40 AM, Dmitry Vyukov <dvyukov@...gle.com> wrote:
> On Wed, Feb 7, 2018 at 3:17 PM, Dmitry Vyukov <dvyukov@...gle.com> wrote:
>> On Wed, Jan 31, 2018 at 5:17 PM, Will Deacon <will.deacon@....com> wrote:
>>>> > * Will Deacon <will.deacon@....com> wrote:
>>>> >> e.g. for atomic[64]_read, your asm-generic header looks like:
>>>> >>
>>>> >> #ifndef _LINUX_ATOMIC_INSTRUMENTED_H
>>>> >> #define _LINUX_ATOMIC_INSTRUMENTED_H
>>>> >>
>>>> >> #include <linux/build_bug.h>
>>>> >> #include <linux/kasan-checks.h>
>>>> >>
>>>> >> static __always_inline int __atomic_read_instrumented(const atomic_t *v)
>>>> >> {
>>>> >>       kasan_check_read(v, sizeof(*v));
>>>> >>       return atomic_read(v);
>>>> >> }
>>>> >>
>>>> >> static __always_inline s64 __atomic64_read_instrumented(const atomic64_t *v)
>>>> >> {
>>>> >>       kasan_check_read(v, sizeof(*v));
>>>> >>       return atomic64_read(v);
>>>> >> }
>>>> >>
>>>> >> #undef atomic_read
>>>> >> #undef atomic64_read
>>>> >>
>>>> >> #define atomic_read   __atomic_read_instrumented
>>>> >> #define atomic64_read __atomic64_read_instrumented
>>>> >>
>>>> >> #endif /* _LINUX_ATOMIC_INSTRUMENTED_H */
>>>> >>
>>>> >> and the arch code just includes that in asm/atomic.h once it's done with
>>>> >> its definitions.
>>>> >>
>>>> >> What do you think? Too stinky?
>>>> >
>>>> > Hm, so while this could work - I actually *like* the low level changes: they are
>>>> > straightforward, trivial, easy to read and they add the arch_ prefix that makes it
>>>> > abundantly clear that this isn't the highest level interface.
>>>> >
>>>> > The KASAN callbacks in the generic methods are also abundantly clear and very easy
>>>> > to read. I could literally verify the sanity of the series while still being only
>>>> > half awake. ;-)
>>>> >
>>>> > Also note that the arch renaming should be 'trivial', in the sense that any
>>>> > missing rename results in a clear build breakage. Plus any architecture making use
>>>> > of this new KASAN feature should probably be tested before it's enabled - and the
>>>> > renaming of the low level atomic APIs kind of forces that too.
>>>> >
>>>> > So while this approach creates some churn, this series is IMHO a marked
>>>> > improvement over the previous iterations.
>>>>
>>>>
>>>> I think I mildly leaning towards Ingo's point.
>>>> I guess people will first find the version in arch (because that's
>>>> where they used to be), but that version is actually not the one that
>>>> is called.
>>>> The renaming is mechanical and you get build errors if anything is
>>>> wrong. It's macros that caused hard to debug runtime crashes and
>>>> multiple revisions of this series.
>>>
>>> Sure, and it sounds like you're proposing to do the arm64 changes anyway so
>>> I'm not complaining! Just thought I'd float the alternative to see what
>>> people think.
>>
>>
>> Any other comments?
>> Ingo, will you take this to locking tree?
>
>
> Any other comments?
> Ingo, will you take this to locking tree?


Peter, Ingo, ping.