Re: [PATCH 0/9] x86/entry fixes

From: Marco Elver <elver@google.com>
To: Peter Zijlstra <peterz@infradead.org>
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
Date: Wed, 3 Jun 2020 19:26:25 +0200	[thread overview]
Message-ID: <CANpmjNMCAv4JS1Go0KUoCgc5y17ROTbaEGFy=tAYosE9sOAnAg@mail.gmail.com> (raw)
In-Reply-To: <20200603160722.GD2570@hirez.programming.kicks-ass.net>

On Wed, 3 Jun 2020 at 18:07, Peter Zijlstra <peterz@infradead.org> wrote:
>
> 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).

Eventually we may get the compilers to do this. But adding even more
ifdefs and Kconfig variables to check which combinations of things
work is getting out of hand. :-/ So if we can make the below work
would be great.

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

Very good. Yes, UBSan inlines the check and the __ubsan_handle
functions are just to generate the report.

Thanks,
-- Marco