linux-kernel - Re: [PATCH 6/7] uprobes: Xol should send SIGTRAP if X86_EFLAGS

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, 12 Sep 2012 17:38:57 +0530
From:	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Ingo Molnar <mingo@...e.hu>, Peter Zijlstra <peterz@...radead.org>,
	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	Anton Arapov <anton@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Roland McGrath <roland@...k.frob.com>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 6/7] uprobes: Xol should send SIGTRAP if X86_EFLAGS_TF
 was	set

* Oleg Nesterov <oleg@...hat.com> [2012-09-03 17:26:16]:

> arch_uprobe_disable_step() correctly preserves X86_EFLAGS_TF and
> returns to user-mode. But this means the application gets SIGTRAP
> only after the next insn.
> 

Agree.

> This means that UPROBE_CLEAR_TF logic is not really right. _enable
> should only record the state of X86_EFLAGS_TF, and _disable should
> check it separately from UPROBE_FIX_SETF.
> 
> Remove arch_uprobe_task->restore_flags, add ->saved_tf instead, and
> change enable/disable accordingly.
> 
> arch_uprobe_skip_sstep() logic has the same problem, change it to
> check X86_EFLAGS_TF and send SIGTRAP as well. We will cleanup this
> all after we fold enable/disable_step into pre/post_hol hooks.
> 
> Note: send_sig(SIGTRAP) is not actually right, we need send_sigtrap().

Are you pointing to the info field not being filled? or are there other
problems?

> But this needs more changes, handle_swbp() does the same and this is
> equally wrong.

send_sigtrap() is arch specific and defined for only few archs.
we would have to force. But these are not related to the patch.

> 
> Signed-off-by: Oleg Nesterov <oleg@...hat.com>


Acked-by: Srikar Dronamraju <srikar@...ux.vnet.ibm.com>

> ---
>  arch/x86/include/asm/uprobes.h |    3 +--
>  arch/x86/kernel/uprobes.c      |   19 +++++++++++++------
>  2 files changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
> index cee5862..d561ff5 100644
> --- a/arch/x86/include/asm/uprobes.h
> +++ b/arch/x86/include/asm/uprobes.h
> @@ -46,8 +46,7 @@ struct arch_uprobe_task {
>  #ifdef CONFIG_X86_64
>  	unsigned long			saved_scratch_register;
>  #endif
> -#define UPROBE_CLEAR_TF			(1 << 0)
> -	unsigned int			restore_flags;
> +	unsigned int			saved_tf;
>  };
> 
>  extern int  arch_uprobe_analyze_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long addr);
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 3b4aae6..7e993d1 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -653,7 +653,7 @@ void arch_uprobe_abort_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
>   * Skip these instructions as per the currently known x86 ISA.
>   * 0x66* { 0x90 | 0x0f 0x1f | 0x0f 0x19 | 0x87 0xc0 }
>   */
> -bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
> +static bool __skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
>  {
>  	int i;
> 
> @@ -681,16 +681,21 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
>  	return false;
>  }
> 
> +bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
> +{
> +	bool ret = __skip_sstep(auprobe, regs);

Should we add a comment here saying ptrace or some debugger wants to
trace this instruction and we singlestepped and hence we want to inform
them now and otherwise they would only know one instruction later?

> +	if (ret && (regs->flags & X86_EFLAGS_TF))
> +		send_sig(SIGTRAP, current, 0);
> +	return ret;
> +}
> +
>  void arch_uprobe_enable_step(struct arch_uprobe *auprobe)
>  {
>  	struct task_struct *task = current;
>  	struct arch_uprobe_task	*autask	= &task->utask->autask;
>  	struct pt_regs *regs = task_pt_regs(task);
> 
> -	autask->restore_flags = 0;
> -	if (!(regs->flags & X86_EFLAGS_TF) &&
> -	    !(auprobe->fixups & UPROBE_FIX_SETF))
> -		autask->restore_flags |= UPROBE_CLEAR_TF;
> +	autask->saved_tf = !!(regs->flags & X86_EFLAGS_TF);
> 
>  	regs->flags |= X86_EFLAGS_TF;
>  	if (test_tsk_thread_flag(task, TIF_BLOCKSTEP))
> @@ -707,6 +712,8 @@ void arch_uprobe_disable_step(struct arch_uprobe *auprobe)
>  	 * SIGTRAP if we do not clear TF. We need to examine the opcode to
>  	 * make it right.
>  	 */
> -	if (autask->restore_flags & UPROBE_CLEAR_TF)
> +	if (autask->saved_tf)
> +		send_sig(SIGTRAP, task, 0);
> +	else if (!(auprobe->fixups & UPROBE_FIX_SETF))
>  		regs->flags &= ~X86_EFLAGS_TF;
>  }
> -- 
> 1.5.5.1
> 

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