Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 3 Jun 2020 18:07:22 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Marco Elver <elver@google.com>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        the arch/x86 maintainers <x86@kernel.org>,
        "Paul E. McKenney" <paulmck@kernel.org>,
        kasan-dev <kasan-dev@googlegroups.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Will Deacon <will@kernel.org>,
        Dmitry Vyukov <dvyukov@google.com>,
        Alexander Potapenko <glider@google.com>,
        Andrey Konovalov <andreyknvl@google.com>
Subject: Re: [PATCH 0/9] x86/entry fixes
Message-ID: <20200603160722.GD2570@hirez.programming.kicks-ass.net>
References: <20200603114014.152292216@infradead.org>
 <20200603120037.GA2570@hirez.programming.kicks-ass.net>
 <20200603120818.GC2627@hirez.programming.kicks-ass.net>
 <CANpmjNOxLkqh=qpHQjUC_bZ0GCjkoJ4NxF3UuNGKhJSvcjavaA@mail.gmail.com>
 <20200603121815.GC2570@hirez.programming.kicks-ass.net>
 <CANpmjNPxMo0sNmkbMHmVYn=WJJwtmYR03ZtFDyPhmiMuR1ug=w@mail.gmail.com>
 <CANpmjNPzmynV2X+e76roUmt_3oq8KDDKyLLsgn__qtAb8i0aXQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CANpmjNPzmynV2X+e76roUmt_3oq8KDDKyLLsgn__qtAb8i0aXQ@mail.gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.