linux-kernel - Re: [PATCH v2 5/8] x86/debug: Simplify #DB signal code

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200824110501.GT1362448@hirez.programming.kicks-ass.net>
Date:   Mon, 24 Aug 2020 13:05:01 +0200
From:   peterz@...radead.org
To:     Andy Lutomirski <luto@...nel.org>
Cc:     X86 ML <x86@...nel.org>, LKML <linux-kernel@...r.kernel.org>,
        Kyle Huey <me@...ehuey.com>,
        Alexandre Chartre <alexandre.chartre@...cle.com>,
        "Robert O'Callahan" <rocallahan@...il.com>,
        "Paul E. McKenney" <paulmck@...nel.org>,
        Frederic Weisbecker <frederic@...nel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Petr Mladek <pmladek@...e.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Joel Fernandes <joel@...lfernandes.org>,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Juergen Gross <jgross@...e.com>,
        Brian Gerst <brgerst@...il.com>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Daniel Thompson <daniel.thompson@...aro.org>
Subject: Re: [PATCH v2 5/8] x86/debug: Simplify #DB signal code

On Sun, Aug 23, 2020 at 04:09:42PM -0700, Andy Lutomirski wrote:
> On Fri, Aug 21, 2020 at 3:21 AM Peter Zijlstra <peterz@...radead.org> wrote:
> >
> > Get rid of the two variables, avoid computing si_code when not needed
> > and be consistent about which dr6 value is used.
> >
> 
> > -       if (tsk->thread.debugreg6 & (DR_STEP | DR_TRAP_BITS) || user_icebp)
> > -               send_sigtrap(regs, 0, si_code);
> > +       /*
> > +        * If dr6 has no reason to give us about the origin of this trap,
> > +        * then it's very likely the result of an icebp/int01 trap.
> > +        * User wants a sigtrap for that.
> > +        */
> > +       if (dr6 & (DR_STEP | DR_TRAP_BITS) || !dr6)
> > +               send_sigtrap(regs, 0, get_si_code(dr6));
> 
> The old condition was ... || (actual DR6 value) and the new condition
> was ... || (stupid notifier-modified DR6 value).  I think the old code
> was more correct.

Hurmph.. /me goes re-read the SDM.

INT1 is a trap,
instruction breakpoint is a fault

So if you have:

	INT1
1:	some-instr

and set an X breakpoint on 1, we'll loose the INT1, right?

And because INT1 is a trap, we can't easily decode the instruction
stream either :-(

Now, OTOH, I suppose you're right in that looking at DR6 early (before
we let people muck about with it) is more reliable for detecting INT1.
Esp since the hw_breakpoint crud can consume all bits.

So I'll go fix that. What a giant pain in the ass all this is.

> The right fix is to get rid of the notifier garbage.  Want to pick up
> these two changes into your series:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/entry&id=b695a5adfdd661a10eead1eebd4002d08400e7ef
> https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/entry&id=40ca016606bd39a465feaf5802a8dc3e937aaa06
> 
> And then there is no code left that modifies ->debugreg6 out from under you.

I'm not convinced. Even with those patches applied we have to use
debugreg6, and that code very much relies on clearing DR_TRAP_BITS
early, and then having ptrace_triggered() re-set bits in it.

This is so that hw_breakpoint handlers can use breakpoints in userspace
without causing send_sigtrap() on them.

So while I hate notifiers as much as the next person, I'm not sure those
patches actually help anything at all in this particular case.

We can't actually remove the notifier, we can't remove debugreg6 and
debugreg6 will still get modified.