linux-kernel - Re: [PATCH 2/2] ptrace/x86: dont delay perf_event_disable() till second pass in ptrace_write

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: <20130416133017.GB9189@redhat.com>
Date:	Tue, 16 Apr 2013 15:30:17 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Frederic Weisbecker <fweisbec@...il.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Alan Stern <stern@...land.harvard.edu>,
	Ingo Molnar <mingo@...e.hu>,
	Jan Kratochvil <jan.kratochvil@...hat.com>,
	Maneesh Soni <maneesh@...ux.vnet.ibm.com>,
	Prasad <prasad@...ux.vnet.ibm.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] ptrace/x86: dont delay perf_event_disable() till
	second pass in ptrace_write_dr7()

On 04/16, Frederic Weisbecker wrote:
>
> On Sun, Apr 14, 2013 at 09:12:32PM +0200, Oleg Nesterov wrote:
> > ptrace_write_dr7() skips ptrace_modify_breakpoint(disabled => true)
> > unless second_pass, this buys nothing but complicates the code and
> > means that we always do the main loop twice even if "disabled" was
> > never true.
> >
> > The comment says:
> >
> > 	Don't unregister the breakpoints right-away,
> > 	unless all register_user_hw_breakpoint()
> > 	requests have succeeded.
> >
> > I think this logic was always wrong, hw_breakpoint_del() does not
> > free the slot so perf_event_disable() can't hurt.
>
> For the record, I think it was necessary before
> 44234adcdce38f83c56e05f808ce656175b4beeb
> ("hw-breakpoints: Modify breakpoints without unregistering them") because
> modifying a breakpoint implied that the old bp was released and a new one
> was created, opening a little race window in between against concurrent
> breakpoint users.

Aah, thank, I'll update the changelog.

> Acked-by: Frederic Weisbecker <fweisbec@...il.com>

Thanks!

> >  	old_dr7 = ptrace_get_dr7(thread->ptrace_bps);
> > @@ -651,35 +643,31 @@ restore:
> >  		bool disabled = !decode_dr7(data, i, &len, &type);
> >  		struct perf_event *bp = thread->ptrace_bps[i];
> >
> > -		if (disabled) {
> > +		if (!bp) {
> > +			if (disabled)
> > +				continue;
> >  			/*
> > -			 * Don't unregister the breakpoints right-away, unless
> > -			 * all register_user_hw_breakpoint() requests have
> > -			 * succeeded. This prevents any window of opportunity
> > -			 * for debug register grabbing by other users.
> > +			 * We should have at least an inactive breakpoint at
> > +			 * this slot. It means the user is writing dr7 without
> > +			 * having written the address register first.
> >  			 */
> > -			if (!bp || !second_pass)
> > -				continue;
> > +			rc = -EINVAL;
> > +			break;
> >  		}
> >
> >  		rc = ptrace_modify_breakpoint(bp, len, type, tsk, disabled);
> >  		if (rc)
> >  			break;
>
> It would be nice to warn here:
>
>    WARN_ON_ONCE(rc && second_pass);

Well, I disagree.

To clarify, I agree with WARN_ON_ONCE(), but afaics it has nothing to
do with "second_pass",

> And these are indeed supposed
> to.

Indeed, but this is because ptrace_modify_breakpoint() should not fail.

So, what do you think if I change the main loop above

		rc = ptrace_modify_breakpoint(...)
	-	if (rc)
	+	if (WARN_ON_ONCE(rc))
			break;

instead?

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/