linux-kernel - Re: [PATCH perf/core 03/22] uprobes: Move ref_ctr_offset update out of uprobe_write

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: <aB02m4ZdPGJOWatx@krava>
Date: Fri, 9 May 2025 00:56:27 +0200
From: Jiri Olsa <olsajiri@...il.com>
To: Oleg Nesterov <oleg@...hat.com>
Cc: Jiri Olsa <olsajiri@...il.com>, Peter Zijlstra <peterz@...radead.org>,
	Andrii Nakryiko <andrii@...nel.org>, bpf@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
	x86@...nel.org, Song Liu <songliubraving@...com>,
	Yonghong Song <yhs@...com>,
	John Fastabend <john.fastabend@...il.com>,
	Hao Luo <haoluo@...gle.com>, Steven Rostedt <rostedt@...dmis.org>,
	Masami Hiramatsu <mhiramat@...nel.org>,
	Alan Maguire <alan.maguire@...cle.com>,
	David Laight <David.Laight@...lab.com>,
	Thomas Weißschuh <thomas@...ch.de>,
	Ingo Molnar <mingo@...nel.org>
Subject: Re: [PATCH perf/core 03/22] uprobes: Move ref_ctr_offset update out
 of uprobe_write_opcode

On Tue, May 06, 2025 at 04:01:45PM +0200, Oleg Nesterov wrote:
> I'm on PTO and traveling until May 15 without my working laptop, can't read
> the code.
> 
> Quite possibly I am wrong, but let me try to recall what this code does...
> 
> - So. uprobe_register() succeeds and changes ref_ctr from 0 to 1.
> 
> - uprobe_unregister() fails but decrements ref_ctr back to zero. Because the
>   "Revert back reference counter if instruction update failed" logic doesn't
>   apply if is_register is true.
> 
>   Since uprobe_unregister() fails, this uprobe won't be removed. IIRC, we even
>   have the warning about that.
> 
> - another uprobe_register() comes and re-uses the same uprobe. In this case
>   install_breakpoint() will do nothing, ref_ctr won't be updated (right ?)

right, because int3 is still in place and verify_opcode returns 0

> 
> - uprobe_unregister() is called again and this time it succeeds. In this case
>   ref_ctr is changed from 0 to -1. IIRC, we even have some warning for this
>   case.

AFAICS that should not happen, there's check below in __update_ref_ctr:

        if (unlikely(*ptr + d < 0)) {
                pr_warn("ref_ctr going negative. vaddr: 0x%lx, "
                        "curr val: %d, delta: %d\n", vaddr, *ptr, d);
                ret = -EINVAL;
                goto out;
        }

        *ptr += d;
        ret = 0;
        ...


but it still prevents the uprobe from 2nd register to trigger,
so I think the change you suggest makes sense


few things first..

 - how do you make uprobe_unregister fail after succesful uprobe_register? 
   I had to instrument the code to do that for me

 - I see one extra uprobe_write_opcode call during unregister (check below)
   seems it does no harm, but looks strange


current code:

   1st register:

   - uprobe_register succeeds and changes ref_ctr_offset from 0 to 1

   1st unregister:

   - first there's uprobe_perf_close -> uprobe_apply call that ends up in
     remove_breakpoint call that will decrement ref_ctr_offset to 0 and fail

   - followed by __probe_event_disable -> uprobe_unregister_nosync call
     that ends up in remove_breakpoint call that will fail to decrement
     ref_ctr_offset to -1 (and ref_ctr_offset stays 0) and fail

   - uprobe is leaked

   2nd register:

   - another uprobe_register() comes and re-uses the same uprobe. In this case
     install_breakpoint() will do nothing, ref_ctr won't be updated, stays 0
     so uprobe WILL NOT trigger

   2nd unregister:

  -  both attempts (from uprobe_perf_close and __probe_event_disable as above)
     to write original instruction will fail, because ref_ctr_offset
     update fails and uprobe_write_opcode bails out


with the attached change we will do:

   1st register:

   - uprobe_register succeeds and changes ref_ctr_offset from 0 to 1

   1st unregister:

   - first there's uprobe_perf_close -> uprobe_apply call that ends up in
     remove_breakpoint call that will decrement ref_ctr_offset to 0 and fail
     and restore ref_ctr_offset to 1

   - followed by __probe_event_disable -> uprobe_unregister_nosync call
     that ends up in remove_breakpoint call that will do the same as
     previous step, ref_ctr_offset is 1

   - uprobe is leaked

   2nd register:

   - another uprobe_register() comes and re-uses the same uprobe. In this case
     install_breakpoint() will do nothing, ref_ctr won't be updated, stays 1,
     so uprobe WILL trigger

   2nd unregister:

  -  succeeds, and ref_ctr_offset is 0


jirka


---
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 207432e92386..65bfe52ed729 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -589,8 +589,8 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct vm_area_struct *vma,
 
 out:
 	/* Revert back reference counter if instruction update failed. */
-	if (ret < 0 && is_register && ref_ctr_updated)
-		update_ref_ctr(uprobe, mm, -1);
+	if (ret < 0 && ref_ctr_updated)
+		update_ref_ctr(uprobe, mm, is_register ? -1 : 1);
 
 	/* try collapse pmd for compound page */
 	if (ret > 0)