On Wed, Feb 19, 2020 at 05:06:03PM +0100, Dmitry Vyukov wrote: > On Wed, Feb 19, 2020 at 4:14 PM Peter Zijlstra <peterz@xxxxxxxxxxxxx> 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().
