linux-kernel - Re: hw_breakpoint: Fix Oops at destroying hw

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:	Thu, 03 Mar 2016 20:23:38 +1100
From:	Michael Ellerman <mpe@...erman.id.au>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com>,
	linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
	acme@...nel.org, mingo@...hat.com, paulus@...ba.org
Subject: Re: hw_breakpoint: Fix Oops at destroying hw_breakpoint event on
 powerpc

On Wed, 2016-03-02 at 12:59 +0100, Peter Zijlstra wrote:

> On Wed, Mar 02, 2016 at 10:53:24PM +1100, Michael Ellerman wrote:

> > Peterz, acme, do you guys want to take this? Or should I?
>
> I'm not too happy its touching event->ctx at all. It really should not
> be doing that.

Hmm OK.

It's been using ctx->task since it was merged in 2010. In fact that commit also
added arch_unregister_hw_breakpoint(), and we're still the only user of that.

The prima facie reason it's using ctx is to get at task->thread to clear
last_hit_ubp.

It looks like other arches avoid needing to do something similar by storing the
break point in a per-cpu array. Which I guess is what you meant in your other
mail ("Why do you keep per task state anyway?").

I can't think of a reason why we can't also store it per-cpu, but I could be
wrong, I don't know the code well and I haven't thought about it for very long.

Do you mind if I merge the following fix for now as a band-aid, and we'll try
and fix it up properly in the next few weeks (but maybe not in time for 4.5
final).

cheers

diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index 05e804cdecaa..aec9a1b1d25b 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -109,8 +109,9 @@ void arch_unregister_hw_breakpoint(struct perf_event *bp)
 	 * If the breakpoint is unregistered between a hw_breakpoint_handler()
 	 * and the single_step_dabr_instruction(), then cleanup the breakpoint
 	 * restoration variables to prevent dangling pointers.
+	 * FIXME, this should not be using bp->ctx at all! Sayeth peterz.
 	 */
-	if (bp->ctx && bp->ctx->task)
+	if (bp->ctx && bp->ctx->task && bp->ctx->task != ((void *)-1L))
 		bp->ctx->task->thread.last_hit_ubp = NULL;
 }