linux-kernel - Re: [PATCH v3] 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]

Date:	Wed, 29 Aug 2012 19:37:48 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	Ananth N Mavinakayanahalli <ananth@...ibm.com>
Cc:	linux-kernel@...r.kernel.org, x86@...nel.org,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Arnaldo Carvalho de Melo <acme@...stprotocols.net>,
	Roland McGrath <roland@...hat.com>,
	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
	stan_shebs@...tor.com
Subject: Re: [PATCH v3] x86/uprobes: implement x86 specific
	arch_uprobe_*_step

On 08/22, Oleg Nesterov wrote:
>
> > Ehm. Is there anything I missed to do? Or are you speculating on
> > changes which will clash with these here?
>
> If we have task_set_blockstep(), then perhaps it mmakes sense to
> avoid user_enable_singlestep()/TIF_SINGLESTEP from the start.
> We will see.

But it is not clear when we will have task_set_blockstep.

So I am starting to think it would be better to apply your 1-2 and
change the code later. Still not correct, but better than nothing.



But. The more I think about the current code, the more I dislike it.
And I am starting to think we do not need yet another "weak arch*"
hook for single-stepping. Yes, it was me who suggested it, but this
is because I didn't want to complicate the merging of powerpc port.

However.

Ananth, Sebastian, what if we start with the patch below? Then
we can change arch/x86/kernel/uprobes.c to use the static
uprobe_*_step() helpers from the 2nd patch.

If we agree this code should be per-arch, then why do need other
hooks? This is just ugly, we already have arch_pre/post_xol.

The only problem is the pending powerpc patches, the change below
obviously breaks them. Were they already applied? If not, then
probably Ananth can do v6 on top of the patch below ;) The necessary
fixup is trivial.

What do you think?

Oleg.


--- x/kernel/events/uprobes.c
+++ x/kernel/events/uprobes.c
@@ -1440,10 +1440,8 @@ static void handle_swbp(struct pt_regs *
 		goto cleanup_ret;
 
 	utask->state = UTASK_SSTEP;
-	if (!pre_ssout(uprobe, regs, bp_vaddr)) {
-		user_enable_single_step(current);
+	if (!pre_ssout(uprobe, regs, bp_vaddr))
 		return;
-	}
 
 cleanup_ret:
 	if (utask) {
@@ -1480,7 +1478,6 @@ static void handle_singlestep(struct upr
 	put_uprobe(uprobe);
 	utask->active_uprobe = NULL;
 	utask->state = UTASK_RUNNING;
-	user_disable_single_step(current);
 	xol_free_insn_slot(current);
 
 	spin_lock_irq(&current->sighand->siglock);
--- x/arch/x86/kernel/uprobes.c
+++ x/arch/x86/kernel/uprobes.c
@@ -471,6 +471,7 @@ int arch_uprobe_pre_xol(struct arch_upro
 	regs->ip = current->utask->xol_vaddr;
 	pre_xol_rip_insn(auprobe, regs, autask);
 
+	user_enable_single_step(current);
 	return 0;
 }
 
@@ -596,6 +597,7 @@ int arch_uprobe_post_xol(struct arch_upr
 	if (auprobe->fixups & UPROBE_FIX_CALL)
 		result = adjust_ret_addr(regs->sp, correction);
 
+	user_disable_single_step(current);
 	return result;
 }
 
@@ -640,6 +642,7 @@ void arch_uprobe_abort_xol(struct arch_u
 	current->thread.trap_nr = utask->autask.saved_trap_nr;
 	handle_riprel_post_xol(auprobe, regs, NULL);
 	instruction_pointer_set(regs, utask->vaddr);
+	user_disable_single_step(current);
 }
 
 /*

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