linux-kernel - Re: [REGRESSION] x86/debug: After PTRACE_SINGLESTEP DR

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]

Date:   Mon, 26 Oct 2020 18:17:49 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Kyle Huey <me@...ehuey.com>
Cc:     open list <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Robert O'Callahan <rocallahan@...il.com>,
        Alexandre Chartre <alexandre.chartre@...cle.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>,
        Joel Fernandes <joel@...lfernandes.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Juergen Gross <jgross@...e.com>,
        Brian Gerst <brgerst@...il.com>,
        Andy Lutomirski <luto@...nel.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Daniel Thompson <daniel.thompson@...aro.org>
Subject: Re: [REGRESSION] x86/debug: After PTRACE_SINGLESTEP DR_STEP is no
 longer reported in dr6

On Mon, Oct 26, 2020 at 10:12:30AM -0700, Kyle Huey wrote:
> On Mon, Oct 26, 2020 at 9:55 AM Peter Zijlstra <peterz@...radead.org> wrote:
> > @@ -935,6 +936,26 @@ static __always_inline void exc_debug_user(struct pt_regs *regs,
> >         irqentry_enter_from_user_mode(regs);
> >         instrumentation_begin();
> >
> > +       /*
> > +        * Clear the virtual DR6 value, ptrace routines will set bits here for
> > +        * things we want signals for.
> > +        */
> > +       current->thread.virtual_dr6 = 0;
> > +
> > +       /*
> > +        * If PTRACE requested SINGLE(BLOCK)STEP, make sure to reflect that in
> > +        * the ptrace visible DR6 copy.
> > +        */
> > +       if (test_thread_flag(TIF_BLOCKSTEP) || test_thread_flag(TIF_SINGLESTEP))
> > +               current->thread.virtual_dr6 |= (dr6 & DR_STEP);
> > +
> > +       /*
> > +        * The SDM says "The processor clears the BTF flag when it
> > +        * generates a debug exception."  Clear TIF_BLOCKSTEP to keep
> > +        * TIF_BLOCKSTEP in sync with the hardware BTF flag.
> > +        */
> > +       clear_thread_flag(TIF_BLOCKSTEP);
> > +
> >         /*
> >          * 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.
> 
> This looks good to me (at least the non BTF parts), and I'll test it
> shortly, but especially now that clearing virtual_dr6 is moved to
> exc_debug_user I still don't see why it's not ok to copy the entire
> dr6 value into virtual_dr6 unconditionally.  Any extraneous dr6 state
> from an in-kernel #DB would have been picked up and cleared already
> when we entered exc_debug_kernel.

There is !ptrace user breakpoints as well. Why should we want potential
random bits in dr6 ?

Suppose perf and ptrace set a user breakpoint on the exact same
instruction. The #DB fires and has two DR_TRAP# bits set. perf consumes
one and ptrace consumes one.

Only the ptrace one should be visible to ptrace, the perf one doesn't
affect the userspace execution at all and shouldn't be visible.