linux-kernel - Re: [PATCH 4/7] uprobes: Fix handle_swbp() vs unregister() + register() race

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20121002184210.GA24199@redhat.com>
Date:	Tue, 2 Oct 2012 20:42:10 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Ingo Molnar <mingo@...e.hu>, Peter Zijlstra <peterz@...radead.org>,
	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
Cc:	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	Anton Arapov <anton@...hat.com>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/7] uprobes: Fix handle_swbp() vs unregister() +
	register() race

On 09/30, Oleg Nesterov wrote:
>
> +	if (uprobe && unlikely(!(uprobe->flags & UPROBE_COPY_INSN))) {
> +		uprobe = NULL;
> +		*is_swbp = 0;
> +	}

OOPS. this obvioulsy needs put_uprobe(uprobe). I updated this patch
in my tree.

-------------------------------------------------------------------
[PATCH] uprobes: Fix handle_swbp() vs unregister() + register() race

Strictly speaking this race was added by me in 56bb4cf6. However
I think that this bug is just another indication that we should
move copy_insn/uprobe_analyze_insn code from install_breakpoint()
to uprobe_register(), there are a lot of other reasons for that.
Until then, add a hack to close the race.

A task can hit uprobe U1, but before it calls find_uprobe() this
uprobe can be unregistered *AND* another uprobe U2 can be added to
uprobes_tree at the same inode/offset. In this case handle_swbp()
will use the not-fully-initialized U2, in particular its arch.insn
for xol.

Add the additional !UPROBE_COPY_INSN check into find_active_uprobe,
if this flag is not set we simply restart as if the new uprobe was
not inserted yet. This is not very nice, we need barriers, but we
will remove this hack when we change uprobe_register().

Note: with or without this patch install_breakpoint() can race with
itself, yet another reson to kill UPROBE_COPY_INSN altogether. And
even the usage of uprobe->flags is not safe. See the next patches.

Signed-off-by: Oleg Nesterov <oleg@...hat.com>
---
 kernel/events/uprobes.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 6058231..5c77137 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -596,6 +596,7 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 		BUG_ON((uprobe->offset & ~PAGE_MASK) +
 				UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);

+		smp_wmb(); /* pairs with rmb() in find_active_uprobe() */
 		uprobe->flags |= UPROBE_COPY_INSN;
 	}

@@ -1391,6 +1392,17 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
 	if (!uprobe && test_and_clear_bit(MMF_RECALC_UPROBES, &mm->flags))
 		mmf_recalc_uprobes(mm);
 	up_read(&mm->mmap_sem);
+	/*
+	 * TODO: move copy_insn/etc into _register and remove this hack.
+	 * After we hit the bp, _unregister + _register can install the
+	 * new and not-yet-analyzed uprobe at the same address, restart.
+	 */
+	smp_rmb(); /* pairs with wmb() in install_breakpoint() */
+	if (uprobe && unlikely(!(uprobe->flags & UPROBE_COPY_INSN))) {
+		put_uprobe(uprobe);
+		uprobe = NULL;
+		*is_swbp = 0;
+	}

 	return uprobe;
 }
-- 
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/