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: <20120530180409.GO15587@linux.vnet.ibm.com>
Date:	Wed, 30 May 2012 23:34:09 +0530
From:	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	Oleg Nesterov <oleg@...hat.com>, Ingo Molnar <mingo@...e.hu>,
	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

> 
> So its the -EEXIST from set_swbp() that I was thinking about. I think I
> was also wrong on the reason that that can happen. register's vma
> iteration is very careful not to have the same vma twice, but it can
> race against mmap() because of the uprobe_hash() vs uprobe_mmap_hash()
> madness, right?


right. 

> 
> Something like so?
> 
> ---
>  kernel/events/uprobes.c |   16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 985be4d..b4e749e 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -45,6 +45,19 @@ static DEFINE_SPINLOCK(uprobes_treelock);	/* serialize rbtree access */
>  
>  #define UPROBES_HASH_SZ	13
>  
> +/*
> + * We need separate register/unregister and mmap/munmap lock hashes because of
> + * mmap_sem nesting.
> + *
> + * {,un}egister_uprobe() needs to install probes on (potentially) all processes
> + * and thus need to acquire multiple mmap_sems (consecutively, not
> + * concurrently), whereas uprobe_m{,un}map() is called while holding mmap_sem
> + * for the particular process doing the mmap.
> + *
> + * This all means that register_uprobe() can race with uprobe_mmap() and we
> + * can try and install a probe where one is already installed.
> + */

Nit: {,un}egister_uprobe should have been uprobe_{,un}register  at
couple of places.

> +
>  /* serialize (un)register */
>  static struct mutex uprobes_mutex[UPROBES_HASH_SZ];
>  
> @@ -356,6 +369,9 @@ int __weak set_swbp(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned
>  {
>  	int result;
>  
> +	/*
> +	 * See the comment near uprobes_hash().
> +	 */
>  	result = is_swbp_at_addr(mm, vaddr);
>  	if (result == 1)
>  		return -EEXIST;
> 

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