linux-kernel - Re: [PATCH 0/9] x86/entry fixes

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: <20200603160722.GD2570@hirez.programming.kicks-ass.net>
Date:   Wed, 3 Jun 2020 18:07:22 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Marco Elver <elver@...gle.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        the arch/x86 maintainers <x86@...nel.org>,
        "Paul E. McKenney" <paulmck@...nel.org>,
        kasan-dev <kasan-dev@...glegroups.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Will Deacon <will@...nel.org>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Alexander Potapenko <glider@...gle.com>,
        Andrey Konovalov <andreyknvl@...gle.com>
Subject: Re: [PATCH 0/9] x86/entry fixes

On Wed, Jun 03, 2020 at 04:47:54PM +0200, Marco Elver wrote:

> This is fun: __always_inline functions inlined into
> __no_sanitize_undefined *do* get instrumented because apparently UBSan
> passes must run before the optimizer (before inlining), contrary to
> what [ATM]SAN instrumentation does. Both GCC and Clang do this.

That's just broken :-( You can keep it marked and then strip it out at
the end if it turns out it wasn't needed after all (of course I do
realize this might not be entirely as trivial as it sounds).

> Some options to fix:
> 
> 1. Add __no_sanitize_undefined to the problematic __always_inline
> functions. I don't know if a macro like '#define
> __always_inline_noinstr __always_inline __no_sanitize_undefined' is
> useful, but it's not an automatic fix either. This option isn't great,
> because it doesn't really scale.

Agreed, that's quite horrible and fragile.

> 2. If you look at the generated code for functions with
> __ubsan_handle_*, all the calls are actually guarded by a branch. So
> if we know that there is no UBSan violation in the function, AFAIK
> we're fine. 

> What are the exact requirements for 'noinstr'?

> Is it only "do not call anything I didn't tell you to call?" If that's
> the case, and there is no bug in the function ;-), then for UBSan
> we're fine.

This; any excursion out of noinstr for an unknown reason can have
unknown side effects which we might not be able to deal with at that
point.

For instance, if we cause a #PF before the current #PF has read CR2,
we're hosed. If we hit a hardware breakpoint before we're ready for it,
we're hosed (and we explicitly disallow setting breakpoints on noinstr,
but not stuff outside it).

So IFF UBSAN only calls out when things have gone wrong, as opposed to
checking if things go wrong (say, an out-of-line bounds check), then,
indeed, assuming no bug, no harm in having them.

And in that regard they're no different from all the WARN_ON() crud we
have all over the place, those are deemed safe under the assumption they
don't happen either.

> With that in mind, you could whitelist "__ubsan_handle"-prefixed
> functions in objtool. Given the __always_inline+noinstr+__ubsan_handle
> case is quite rare, it might be reasonable.

Yes, I think so. Let me go have dinner and then I'll try and do a patch
to that effect.