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]

Message-ID: <20201026163100.GR2594@hirez.programming.kicks-ass.net>
Date:   Mon, 26 Oct 2020 17:31:00 +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 09:14:13AM -0700, Kyle Huey wrote:
> > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> > index 3c70fb34028b..0e7641ac19a8 100644
> > --- a/arch/x86/kernel/traps.c
> > +++ b/arch/x86/kernel/traps.c
> > @@ -799,6 +799,13 @@ static __always_inline unsigned long debug_read_clear_dr6(void)
> >          */
> >         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
> 
> I don't think the `& DR_STEP` should be necessary, that bit should be
> set by the hardware, I believe.

Yeah, the idea is to only 'copy' the DR_STEP bit, not any others.

> Also if a program sets TF on itself in EFLAGS and generates a trap it
> should still have DR_STEP set in the ptrace-visible dr6, which this
> won't do.

Why? The state was not requested by ptrace(). And the signal should have
it's si_code set to TRACE_TRACE.

> Is there a reason not to always copy dr6 into virtual_dr6 here,
> regardless of TIF_SINGLESTEP/etc?

Why should we expose DR6 bits that are the result of kernel internal
actions?

Suppose someone sets an in-kernel breakpoint (perf can do that for
example), the #DB happens and we write whatever into virtual_dr6.

Even if you have a userspace breakpoint not through ptrace(), then why
should we expose that in dr6 ?


In that respect, I think the current virtual_dr6 = 0 is placed wrong, it
should only be in exc_debug_user(). The only 'problem' then is that we
seem to be able to loose BTF, but perhaps that is already an extant bug.

Consider:

 - perf: setup in-kernel #DB
 - tracer: ptrace(PTRACE_SINGLEBLOCK)
 - tracee: #DB on perf breakpoint, looses BTF
 - tracee .. never triggers actual blockstep

Hmm ? Should we re-set BTF when TIF_BLOCKSTEP && !user_mode(regs) ?