From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Zijlstra Subject: Re: [PATCH v3 22/22] x86/int3: Ensure that poke_int3_handler() is not sanitized Date: Wed, 19 Feb 2020 17:30:25 +0100 Message-ID: <20200219163025.GH18400@hirez.programming.kicks-ass.net> References: <20200219144724.800607165@infradead.org> <20200219150745.651901321@infradead.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from bombadil.infradead.org ([198.137.202.133]:33448 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726652AbgBSQa5 (ORCPT ); Wed, 19 Feb 2020 11:30:57 -0500 Content-Disposition: inline In-Reply-To: Sender: linux-arch-owner@vger.kernel.org List-ID: To: Dmitry Vyukov Cc: LKML , linux-arch , Steven Rostedt , Ingo Molnar , Joel Fernandes , Greg Kroah-Hartman , "Gustavo A. R. Silva" , Thomas Gleixner , "Paul E. McKenney" , Josh Triplett , Mathieu Desnoyers , Lai Jiangshan , Andy Lutomirski , tony.luck@intel.com, Frederic Weisbecker , Dan Carpenter , Masami Hiramatsu , Andrey Ryabinin , kasan-dev On Wed, Feb 19, 2020 at 05:06:03PM +0100, Dmitry Vyukov wrote: > On Wed, Feb 19, 2020 at 4:14 PM Peter Zijlstra wrote: > > > > In order to ensure poke_int3_handler() is completely self contained -- > > we call this while we're modifying other text, imagine the fun of > > hitting another INT3 -- ensure that everything is without sanitize > > crud. > > +kasan-dev > > Hi Peter, > > How do we hit another INT3 here? INT3 is mostly the result of either kprobes (someone sticks a kprobe in the middle of *SAN) or self modifying text stuff (jump_labels, ftrace and soon static_call). > Does the code do > out-of-bounds/use-after-free writes? > Debugging later silent memory corruption may be no less fun :) It all stinks, debugging a recursive exception is also not fun. > Not sanitizing bsearch entirely is a bit unfortunate. We won't find > any bugs in it when called from other sites too. Agreed. > It may deserve a comment at least. Tomorrow I may want to remove > __no_sanitize, just because sanitizing more is better, and no int3 > test will fail to stop me from doing that... If only I actually had a test-case for this :/ > It's quite fragile. Tomorrow poke_int3_handler handler calls more of > fewer functions, and both ways it's not detected by anything. Yes; not having tools for this is pretty annoying. In 0/n I asked Dan if smatch could do at least the normal tracing stuff, the compiler instrumentation bits are going to be far more difficult because smatch doesn't work at that level :/ (I actually have > And if we ignore all by one function, it is still not helpful, right? > Depending on failure cause/mode, using kasan_disable/enable_current > may be a better option. kasan_disable_current() could mostly work; but only covers kasan, not ubsan or kcsan. It then also relies on kasan_disable_current() itself being notrace as well as all instrumentation functions itself (which I think is currently true because of mm/kasan/Makefile stripping CC_FLAGS_FTRACE). But what stops someone from sticking a kprobe or #DB before you check that variable? By inlining everything in poke_int3_handler() (except bsearch :/) we can mark the whole function off limits to everything and call it a day. That simplicity has been the guiding principle so far. Alternatively we can provide an __always_inline variant of bsearch().