linux-kernel - Re: [PATCH 2/2] x86/uprobes: implement x86 specific arch_uprobe_*

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: <20120801122616.GA32705@redhat.com>
Date:	Wed, 1 Aug 2012 14:26:16 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:	linux-kernel@...r.kernel.org, ananth@...ibm.com,
	a.p.zijlstra@...llo.nl, mingo@...hat.com,
	srikar@...ux.vnet.ibm.com, roland@...k.frob.com
Subject: Re: [PATCH 2/2] x86/uprobes: implement x86 specific
	arch_uprobe_*_step

On 07/31, Sebastian Andrzej Siewior wrote:
>
> On 07/31/2012 07:51 PM, Oleg Nesterov wrote:
>> However, honestly I do not like it. I think we should change this
>> step-by-step, that is why I suggested to use TIF_SINGLESTEP and
>> user_enable_single_step() like your initial patch did. With this
>> patch at least the debugger doesn't lose the control over the tracee
>> if it steps over the probed insn, and this is the main (and known ;)
>> problem to me.
>
> I thought you did not like the nesting with TIF_SIGNLESTEP and the
> _FORCE and suggested to handle the complete state within uprobe.

Yes, but I suggested to do this in a separate patch.

In particular, because even if we do not care about DEBUGCTLMSR_BTF
after _disable, _enable has to clear it. See below.

>> Every change needs the discussion. For example, _enable should
>> clear DEBUGCTLMSR_BTF, this is obvious. But it is not clear to
>> me if _disable should restore it. What if the probed insn was
>> "jmp"? We need the additional complications to handle this case
>> really correctly, and for what? OK, gdb can get the extra SIGTRAP
>> from the tracee, but this is fine. And uprobes can confuse gdb
>> in many ways.
>
> I don't know if it is worth to have correct behavior here or rather go
> for the easy way which is either always do the wakeup or delay until
> the next jump.

This is debatable, personally I think it is fine to lose DEBUGCTLMSR_BTF.
Otherwise we need much more complications, not sure if it worth a trouble.
And an extra SIGTRAP for gdb is certainly better than the lost SIGTRAP.

However, once again, at least we should clear DEBUGCTLMSR_BTF
before the step. And I strongly believe we should not copy-and-paste
this code from step.c. This means we need something like the patch
below, before we teach uprobes to play with _TF directly, imho.

And btw, this is offtopic, but the usage of update_debugctlmsr()
doesn't look right to me (I can be easily wrong though). I'll write
another email.

Oleg.

--- x/arch/x86/kernel/step.c
+++ x/arch/x86/kernel/step.c
@@ -157,6 +157,21 @@ static int enable_single_step(struct tas
 	return 1;
 }
 
+void enable_block_step(struct task_struct *child, bool on)
+{
+	unsigned long debugctl = get_debugctlmsr();
+
+	if (on) {
+		set_tsk_thread_flag(child, TIF_BLOCKSTEP);
+		debugctl |= DEBUGCTLMSR_BTF;
+	} else {
+		clear_tsk_thread_flag(child, TIF_BLOCKSTEP);
+		debugctl &= ~DEBUGCTLMSR_BTF;
+	}
+
+	update_debugctlmsr(debugctl);
+}
+
 /*
  * Enable single or block step.
  */
@@ -169,19 +184,10 @@ static void enable_step(struct task_stru
 	 * So no one should try to use debugger block stepping in a program
 	 * that uses user-mode single stepping itself.
 	 */
-	if (enable_single_step(child) && block) {
-		unsigned long debugctl = get_debugctlmsr();
-
-		debugctl |= DEBUGCTLMSR_BTF;
-		update_debugctlmsr(debugctl);
-		set_tsk_thread_flag(child, TIF_BLOCKSTEP);
-	} else if (test_tsk_thread_flag(child, TIF_BLOCKSTEP)) {
-		unsigned long debugctl = get_debugctlmsr();
-
-		debugctl &= ~DEBUGCTLMSR_BTF;
-		update_debugctlmsr(debugctl);
-		clear_tsk_thread_flag(child, TIF_BLOCKSTEP);
-	}
+	if (enable_single_step(child) && block)
+		enable_block_step(true);
+	else if (test_tsk_thread_flag(child, TIF_BLOCKSTEP))
+		enable_block_step(false);
 }
 
 void user_enable_single_step(struct task_struct *child)
@@ -199,13 +205,8 @@ void user_disable_single_step(struct tas
 	/*
 	 * Make sure block stepping (BTF) is disabled.
 	 */
-	if (test_tsk_thread_flag(child, TIF_BLOCKSTEP)) {
-		unsigned long debugctl = get_debugctlmsr();
-
-		debugctl &= ~DEBUGCTLMSR_BTF;
-		update_debugctlmsr(debugctl);
-		clear_tsk_thread_flag(child, TIF_BLOCKSTEP);
-	}
+	if (test_tsk_thread_flag(child, TIF_BLOCKSTEP))
+		enable_block_step(false);
 
 	/* Always clear TIF_SINGLESTEP... */
 	clear_tsk_thread_flag(child, TIF_SINGLESTEP);

--
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/