If uprobe handler changes instruction pointer we still execute single
step) or emulate the original instruction and increment the (new) ip
with its length.

This makes the new instruction pointer bogus and application will
likely crash on illegal instruction execution.

If user decided to take execution elsewhere, it makes little sense
to execute the original instruction, so let's skip it.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/events/uprobes.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 4c965ba77f9f..dff5509cde67 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -2742,6 +2742,9 @@ static void handle_swbp(struct pt_regs *regs)
 
 	handler_chain(uprobe, regs);
 
+	if (instruction_pointer(regs) != bp_vaddr)
+		goto out;
+
 	if (arch_uprobe_skip_sstep(&uprobe->arch, regs))
 		goto out;
 
-- 
2.50.1

On 08/01, Jiri Olsa wrote:
>
> If uprobe handler changes instruction pointer we still execute single
> step) or emulate the original instruction and increment the (new) ip
> with its length.

Yes... but what if we there are multiple consumers? The 1st one changes
instruction_pointer, the next is unaware. Or it may change regs->ip too...

Oleg.

> This makes the new instruction pointer bogus and application will
> likely crash on illegal instruction execution.
> 
> If user decided to take execution elsewhere, it makes little sense
> to execute the original instruction, so let's skip it.
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  kernel/events/uprobes.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 4c965ba77f9f..dff5509cde67 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -2742,6 +2742,9 @@ static void handle_swbp(struct pt_regs *regs)
>  
>  	handler_chain(uprobe, regs);
>  
> +	if (instruction_pointer(regs) != bp_vaddr)
> +		goto out;
> +
>  	if (arch_uprobe_skip_sstep(&uprobe->arch, regs))
>  		goto out;
>  
> -- 
> 2.50.1
>

On Sat, Aug 02, 2025 at 12:34:27PM +0200, Oleg Nesterov wrote:
> On 08/01, Jiri Olsa wrote:
> >
> > If uprobe handler changes instruction pointer we still execute single
> > step) or emulate the original instruction and increment the (new) ip
> > with its length.
> 
> Yes... but what if we there are multiple consumers? The 1st one changes
> instruction_pointer, the next is unaware. Or it may change regs->ip too...

right, and I think that's already bad in current code

how about we dd flag to the consumer that ensures it's the only consumer
on the uprobe.. and we would skip original instruction execution for such
uprobe if its consumer changes the regs->ip.. I'll try to come up with the
patch

jirka

On Mon, Aug 04, 2025 at 10:12:15AM +0200, Jiri Olsa wrote:
> On Sat, Aug 02, 2025 at 12:34:27PM +0200, Oleg Nesterov wrote:
> > On 08/01, Jiri Olsa wrote:
> > >
> > > If uprobe handler changes instruction pointer we still execute single
> > > step) or emulate the original instruction and increment the (new) ip
> > > with its length.
> > 
> > Yes... but what if we there are multiple consumers? The 1st one changes
> > instruction_pointer, the next is unaware. Or it may change regs->ip too...
> 
> right, and I think that's already bad in current code
> 
> how about we dd flag to the consumer that ensures it's the only consumer
> on the uprobe.. and we would skip original instruction execution for such
> uprobe if its consumer changes the regs->ip.. I'll try to come up with the
> patch

how about something like below?

jirka


---
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 516217c39094..b2c49a2d5468 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -59,6 +59,7 @@ struct uprobe_consumer {
 	struct list_head cons_node;
 
 	__u64 id;	/* set when uprobe_consumer is registered */
+	bool is_unique; /* the only consumer on uprobe */
 };
 
 #ifdef CONFIG_UPROBES
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index f774367c8e71..b317f9fbbf5c 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1014,14 +1014,32 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset,
 	return uprobe;
 }
 
-static void consumer_add(struct uprobe *uprobe, struct uprobe_consumer *uc)
+static bool consumer_can_add(struct list_head *head, struct uprobe_consumer *uc)
+{
+	/* there's no consumer, free to add one */
+	if (list_empty(head))
+		return true;
+	/* uprobe has consumer(s), can't add unique one */
+	if (uc->is_unique)
+		return false;
+	/* uprobe has consumer(s), we can add one only if it's not unique consumer */
+	return !list_first_entry(head, struct uprobe_consumer, cons_node)->is_unique;
+}
+
+static int consumer_add(struct uprobe *uprobe, struct uprobe_consumer *uc)
 {
 	static atomic64_t id;
+	int ret = -EBUSY;
 
 	down_write(&uprobe->consumer_rwsem);
+	if (!consumer_can_add(&uprobe->consumers, uc))
+		goto unlock;
 	list_add_rcu(&uc->cons_node, &uprobe->consumers);
 	uc->id = (__u64) atomic64_inc_return(&id);
+	ret = 0;
+unlock:
 	up_write(&uprobe->consumer_rwsem);
+	return ret;
 }
 
 /*
@@ -1410,7 +1428,12 @@ struct uprobe *uprobe_register(struct inode *inode,
 		return uprobe;
 
 	down_write(&uprobe->register_rwsem);
-	consumer_add(uprobe, uc);
+	ret = consumer_add(uprobe, uc);
+	if (ret) {
+		put_uprobe(uprobe);
+		up_write(&uprobe->register_rwsem);
+		return ERR_PTR(ret);
+	}
 	ret = register_for_each_vma(uprobe, uc);
 	up_write(&uprobe->register_rwsem);
 
@@ -2522,7 +2545,7 @@ static bool ignore_ret_handler(int rc)
 	return rc == UPROBE_HANDLER_REMOVE || rc == UPROBE_HANDLER_IGNORE;
 }
 
-static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
+static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs, bool *is_unique)
 {
 	struct uprobe_consumer *uc;
 	bool has_consumers = false, remove = true;
@@ -2536,6 +2559,8 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
 		__u64 cookie = 0;
 		int rc = 0;
 
+		*is_unique |= uc->is_unique;
+
 		if (uc->handler) {
 			rc = uc->handler(uc, regs, &cookie);
 			WARN(rc < 0 || rc > 2,
@@ -2685,6 +2710,7 @@ static void handle_swbp(struct pt_regs *regs)
 {
 	struct uprobe *uprobe;
 	unsigned long bp_vaddr;
+	bool is_unique = false;
 	int is_swbp;
 
 	bp_vaddr = uprobe_get_swbp_addr(regs);
@@ -2739,7 +2765,10 @@ static void handle_swbp(struct pt_regs *regs)
 	if (arch_uprobe_ignore(&uprobe->arch, regs))
 		goto out;
 
-	handler_chain(uprobe, regs);
+	handler_chain(uprobe, regs, &is_unique);
+
+	if (is_unique && instruction_pointer(regs) != bp_vaddr)
+		goto out;
 
 	if (arch_uprobe_skip_sstep(&uprobe->arch, regs))
 		goto out;