Re: [PATCH v3 22/22] x86/int3: Ensure that poke_int3_handler() is not sanitized

[Peter Zijlstra]

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