linux-kernel - Re: [PATCH 1/3] uprobes: install_breakpoint() should fail if is_swbp

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: <1338398931.28384.7.camel@twins>
Date:	Wed, 30 May 2012 19:28:51 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Ingo Molnar <mingo@...e.hu>,
	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	Anton Arapov <anton@...hat.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] uprobes: install_breakpoint() should fail if
 is_swbp_insn() == T

On Wed, 2012-05-30 at 18:58 +0200, Oleg Nesterov wrote:
> install_breakpoint() returns -EEXIST if is_swbp_insn(orig_insn) == T,
> the caller treats this code as success.
> 
> This is doubly wrong. The successful return should set UPROBE_COPY_INSN,
> but the real problem is that it shouldn't succeed. If the probed insn is
> int3 the application should get SIGTRAP, this won't happen with uprobe.
> 
> Probably we can fix this, we can add the UPROBE_SHARED_BP flag and teach
> handle_swbp/set_orig_insn to handle this case correctly. But this needs
> some complications and we have other insns which can't be probed, lets
> make a simple fix for now.
> 
> I think this needs a cleanup. UPROBE_COPY_INSN should die, copy_insn()
> should be called by alloc_uprobe(). arch_uprobe_analyze_insn() depends
> on ->mm (ia32_compat) but it is called only once.
> 
> Signed-off-by: Oleg Nesterov <oleg@...hat.com>
> ---
>  kernel/events/uprobes.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 8c5e043..1593b43 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -704,7 +704,7 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
>  			return ret;
>  
>  		if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
> -			return -EEXIST;
> +			return -ENOTSUPP;
>  
>  		ret = arch_uprobe_analyze_insn(&uprobe->arch, mm);
>  		if (ret)

IIRC this -EEXIST existed because the vma iteration it does is racy and
one can encounter the same vma twice or so. See the special -EEXIST
handling in register_for_each_vma().

Changing it like this would break stuff. 
--
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/