[PATCHv3 perf/core] uprobes: Harden uretprobe syscall trampoline check

Jiri Olsa posted 1 patch 10 months, 1 week ago
arch/x86/kernel/uprobes.c | 14 +++++++++-----
include/linux/uprobes.h   |  2 ++
kernel/events/uprobes.c   |  2 +-
3 files changed, 12 insertions(+), 6 deletions(-)
[PATCHv3 perf/core] uprobes: Harden uretprobe syscall trampoline check
Posted by Jiri Olsa 10 months, 1 week ago
Jann reported [1] possible issue when trampoline_check_ip returns
address near the bottom of the address space that is allowed to
call into the syscall if uretprobes are not set up.

Though the mmap minimum address restrictions will typically prevent
creating mappings there, let's make sure uretprobe syscall checks
for that.

[1] https://lore.kernel.org/bpf/202502081235.5A6F352985@keescook/T/#m9d416df341b8fbc11737dacbcd29f0054413cbbf
Cc: Kees Cook <kees@kernel.org>
Cc: Eyal Birger <eyal.birger@gmail.com>
Cc: stable@vger.kernel.org
Fixes: ff474a78cef5 ("uprobe: Add uretprobe syscall to speed up return probe")
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Reported-by: Jann Horn <jannh@google.com>
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Kees Cook <kees@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
v3 changes:
 - used ~0UL instead of -1 [Alexei]
 - used UPROBE_NO_TRAMPOLINE_VADDR in uprobe_get_trampoline_vaddr [Masami]
 - added unlikely [Andrii]
 - I kept the review/ack tags, because I think the change is basically
   the same, please scream otherwise

 arch/x86/kernel/uprobes.c | 14 +++++++++-----
 include/linux/uprobes.h   |  2 ++
 kernel/events/uprobes.c   |  2 +-
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 5a952c5ea66b..9194695662b2 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -357,19 +357,23 @@ void *arch_uprobe_trampoline(unsigned long *psize)
 	return &insn;
 }
 
-static unsigned long trampoline_check_ip(void)
+static unsigned long trampoline_check_ip(unsigned long tramp)
 {
-	unsigned long tramp = uprobe_get_trampoline_vaddr();
-
 	return tramp + (uretprobe_syscall_check - uretprobe_trampoline_entry);
 }
 
 SYSCALL_DEFINE0(uretprobe)
 {
 	struct pt_regs *regs = task_pt_regs(current);
-	unsigned long err, ip, sp, r11_cx_ax[3];
+	unsigned long err, ip, sp, r11_cx_ax[3], tramp;
+
+	/* If there's no trampoline, we are called from wrong place. */
+	tramp = uprobe_get_trampoline_vaddr();
+	if (unlikely(tramp == UPROBE_NO_TRAMPOLINE_VADDR))
+		goto sigill;
 
-	if (regs->ip != trampoline_check_ip())
+	/* Make sure the ip matches the only allowed sys_uretprobe caller. */
+	if (unlikely(regs->ip != trampoline_check_ip(tramp)))
 		goto sigill;
 
 	err = copy_from_user(r11_cx_ax, (void __user *)regs->sp, sizeof(r11_cx_ax));
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index a40efdda9052..2e46b69ff0a6 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -39,6 +39,8 @@ struct page;
 
 #define MAX_URETPROBE_DEPTH		64
 
+#define UPROBE_NO_TRAMPOLINE_VADDR	(~0UL)
+
 struct uprobe_consumer {
 	/*
 	 * handler() can return UPROBE_HANDLER_REMOVE to signal the need to
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 597b9e036e5f..c5d6307bc5bc 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -2156,8 +2156,8 @@ void uprobe_copy_process(struct task_struct *t, unsigned long flags)
  */
 unsigned long uprobe_get_trampoline_vaddr(void)
 {
+	unsigned long trampoline_vaddr = UPROBE_NO_TRAMPOLINE_VADDR;
 	struct xol_area *area;
-	unsigned long trampoline_vaddr = -1;
 
 	/* Pairs with xol_add_vma() smp_store_release() */
 	area = READ_ONCE(current->mm->uprobes_state.xol_area); /* ^^^ */
-- 
2.48.1
Re: [PATCHv3 perf/core] uprobes: Harden uretprobe syscall trampoline check
Posted by Masami Hiramatsu (Google) 10 months ago
On Wed, 12 Feb 2025 23:04:33 +0100
Jiri Olsa <jolsa@kernel.org> wrote:

> Jann reported [1] possible issue when trampoline_check_ip returns
> address near the bottom of the address space that is allowed to
> call into the syscall if uretprobes are not set up.
> 
> Though the mmap minimum address restrictions will typically prevent
> creating mappings there, let's make sure uretprobe syscall checks
> for that.
> 
> [1] https://lore.kernel.org/bpf/202502081235.5A6F352985@keescook/T/#m9d416df341b8fbc11737dacbcd29f0054413cbbf
> Cc: Kees Cook <kees@kernel.org>
> Cc: Eyal Birger <eyal.birger@gmail.com>
> Cc: stable@vger.kernel.org
> Fixes: ff474a78cef5 ("uprobe: Add uretprobe syscall to speed up return probe")
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> Reported-by: Jann Horn <jannh@google.com>
> Reviewed-by: Oleg Nesterov <oleg@redhat.com>
> Reviewed-by: Kees Cook <kees@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Looks good to me.

Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thank you,

> ---
> v3 changes:
>  - used ~0UL instead of -1 [Alexei]
>  - used UPROBE_NO_TRAMPOLINE_VADDR in uprobe_get_trampoline_vaddr [Masami]
>  - added unlikely [Andrii]
>  - I kept the review/ack tags, because I think the change is basically
>    the same, please scream otherwise
> 
>  arch/x86/kernel/uprobes.c | 14 +++++++++-----
>  include/linux/uprobes.h   |  2 ++
>  kernel/events/uprobes.c   |  2 +-
>  3 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 5a952c5ea66b..9194695662b2 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -357,19 +357,23 @@ void *arch_uprobe_trampoline(unsigned long *psize)
>  	return &insn;
>  }
>  
> -static unsigned long trampoline_check_ip(void)
> +static unsigned long trampoline_check_ip(unsigned long tramp)
>  {
> -	unsigned long tramp = uprobe_get_trampoline_vaddr();
> -
>  	return tramp + (uretprobe_syscall_check - uretprobe_trampoline_entry);
>  }
>  
>  SYSCALL_DEFINE0(uretprobe)
>  {
>  	struct pt_regs *regs = task_pt_regs(current);
> -	unsigned long err, ip, sp, r11_cx_ax[3];
> +	unsigned long err, ip, sp, r11_cx_ax[3], tramp;
> +
> +	/* If there's no trampoline, we are called from wrong place. */
> +	tramp = uprobe_get_trampoline_vaddr();
> +	if (unlikely(tramp == UPROBE_NO_TRAMPOLINE_VADDR))
> +		goto sigill;
>  
> -	if (regs->ip != trampoline_check_ip())
> +	/* Make sure the ip matches the only allowed sys_uretprobe caller. */
> +	if (unlikely(regs->ip != trampoline_check_ip(tramp)))
>  		goto sigill;
>  
>  	err = copy_from_user(r11_cx_ax, (void __user *)regs->sp, sizeof(r11_cx_ax));
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index a40efdda9052..2e46b69ff0a6 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -39,6 +39,8 @@ struct page;
>  
>  #define MAX_URETPROBE_DEPTH		64
>  
> +#define UPROBE_NO_TRAMPOLINE_VADDR	(~0UL)
> +
>  struct uprobe_consumer {
>  	/*
>  	 * handler() can return UPROBE_HANDLER_REMOVE to signal the need to
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 597b9e036e5f..c5d6307bc5bc 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -2156,8 +2156,8 @@ void uprobe_copy_process(struct task_struct *t, unsigned long flags)
>   */
>  unsigned long uprobe_get_trampoline_vaddr(void)
>  {
> +	unsigned long trampoline_vaddr = UPROBE_NO_TRAMPOLINE_VADDR;
>  	struct xol_area *area;
> -	unsigned long trampoline_vaddr = -1;
>  
>  	/* Pairs with xol_add_vma() smp_store_release() */
>  	area = READ_ONCE(current->mm->uprobes_state.xol_area); /* ^^^ */
> -- 
> 2.48.1
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>
Re: [PATCHv3 perf/core] uprobes: Harden uretprobe syscall trampoline check
Posted by Andy Lutomirski 10 months ago
On Wed, Feb 12, 2025 at 2:04 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Jann reported [1] possible issue when trampoline_check_ip returns
> address near the bottom of the address space that is allowed to
> call into the syscall if uretprobes are not set up.
>
> Though the mmap minimum address restrictions will typically prevent
> creating mappings there, let's make sure uretprobe syscall checks
> for that.

It would be a layering violation, but we could perhaps do better here:

> -       if (regs->ip != trampoline_check_ip())
> +       /* Make sure the ip matches the only allowed sys_uretprobe caller. */
> +       if (unlikely(regs->ip != trampoline_check_ip(tramp)))
>                 goto sigill;

Instead of SIGILL, perhaps this should do the seccomp action?  So the
logic in seccomp would be (sketchily, with some real mode1 mess):

if (is_a_real_uretprobe())
    skip seccomp;

where is_a_real_uretprobe() is only true if the nr and arch match
uretprobe *and* the address is right.

--Andy
Re: [PATCHv3 perf/core] uprobes: Harden uretprobe syscall trampoline check
Posted by Eyal Birger 10 months ago
(sorry for the HTML spam)

On Wed, Feb 12, 2025 at 5:37 PM Andy Lutomirski <luto@kernel.org> wrote:
>
> On Wed, Feb 12, 2025 at 2:04 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Jann reported [1] possible issue when trampoline_check_ip returns
> > address near the bottom of the address space that is allowed to
> > call into the syscall if uretprobes are not set up.
> >
> > Though the mmap minimum address restrictions will typically prevent
> > creating mappings there, let's make sure uretprobe syscall checks
> > for that.
>
> It would be a layering violation, but we could perhaps do better here:
>
> > -       if (regs->ip != trampoline_check_ip())
> > +       /* Make sure the ip matches the only allowed sys_uretprobe caller. */
> > +       if (unlikely(regs->ip != trampoline_check_ip(tramp)))
> >                 goto sigill;
>
> Instead of SIGILL, perhaps this should do the seccomp action?  So the
> logic in seccomp would be (sketchily, with some real mode1 mess):
>
> if (is_a_real_uretprobe())
>     skip seccomp;
>
> where is_a_real_uretprobe() is only true if the nr and arch match
> uretprobe *and* the address is right.



Why would it make sense to rely on CONFIG_SECCOMP for this check? seems
this check should be done regardless of seccomp.

Or maybe I missed something in the suggestion.

Eyal.

>
>
> --Andy
Re: [PATCHv3 perf/core] uprobes: Harden uretprobe syscall trampoline check
Posted by Jiri Olsa 10 months ago
On Wed, Feb 12, 2025 at 05:37:11PM -0800, Andy Lutomirski wrote:
> On Wed, Feb 12, 2025 at 2:04 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Jann reported [1] possible issue when trampoline_check_ip returns
> > address near the bottom of the address space that is allowed to
> > call into the syscall if uretprobes are not set up.
> >
> > Though the mmap minimum address restrictions will typically prevent
> > creating mappings there, let's make sure uretprobe syscall checks
> > for that.
> 
> It would be a layering violation, but we could perhaps do better here:
> 
> > -       if (regs->ip != trampoline_check_ip())
> > +       /* Make sure the ip matches the only allowed sys_uretprobe caller. */
> > +       if (unlikely(regs->ip != trampoline_check_ip(tramp)))
> >                 goto sigill;
> 
> Instead of SIGILL, perhaps this should do the seccomp action?  So the
> logic in seccomp would be (sketchily, with some real mode1 mess):
> 
> if (is_a_real_uretprobe())
>     skip seccomp;

IIUC you want to move the address check earlier to the seccomp path..
with the benefit that we would kill not allowed caller sooner?

jirka

> 
> where is_a_real_uretprobe() is only true if the nr and arch match
> uretprobe *and* the address is right.
> 
> --Andy
Re: [PATCHv3 perf/core] uprobes: Harden uretprobe syscall trampoline check
Posted by Andy Lutomirski 10 months ago
On Thu, Feb 13, 2025 at 1:16 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Wed, Feb 12, 2025 at 05:37:11PM -0800, Andy Lutomirski wrote:
> > On Wed, Feb 12, 2025 at 2:04 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > Jann reported [1] possible issue when trampoline_check_ip returns
> > > address near the bottom of the address space that is allowed to
> > > call into the syscall if uretprobes are not set up.
> > >
> > > Though the mmap minimum address restrictions will typically prevent
> > > creating mappings there, let's make sure uretprobe syscall checks
> > > for that.
> >
> > It would be a layering violation, but we could perhaps do better here:
> >
> > > -       if (regs->ip != trampoline_check_ip())
> > > +       /* Make sure the ip matches the only allowed sys_uretprobe caller. */
> > > +       if (unlikely(regs->ip != trampoline_check_ip(tramp)))
> > >                 goto sigill;
> >
> > Instead of SIGILL, perhaps this should do the seccomp action?  So the
> > logic in seccomp would be (sketchily, with some real mode1 mess):
> >
> > if (is_a_real_uretprobe())
> >     skip seccomp;
>
> IIUC you want to move the address check earlier to the seccomp path..
> with the benefit that we would kill not allowed caller sooner?

The benefit would be that seccomp users that want to do something
other than killing a process (returning an error code, getting
notified, etc) could retain that functionality without the new
automatic hole being poked for uretprobe() in cases where uprobes
aren't in use or where the calling address doesn't match the uprobe
trampoline.  IOW it would reduce the scope to which we're making
seccomp behave unexpectedly.

>
> jirka
>
> >
> > where is_a_real_uretprobe() is only true if the nr and arch match
> > uretprobe *and* the address is right.
> >
> > --Andy
>
Re: [PATCHv3 perf/core] uprobes: Harden uretprobe syscall trampoline check
Posted by Jiri Olsa 10 months ago
On Thu, Feb 13, 2025 at 09:58:29AM -0800, Andy Lutomirski wrote:
> On Thu, Feb 13, 2025 at 1:16 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Wed, Feb 12, 2025 at 05:37:11PM -0800, Andy Lutomirski wrote:
> > > On Wed, Feb 12, 2025 at 2:04 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > > >
> > > > Jann reported [1] possible issue when trampoline_check_ip returns
> > > > address near the bottom of the address space that is allowed to
> > > > call into the syscall if uretprobes are not set up.
> > > >
> > > > Though the mmap minimum address restrictions will typically prevent
> > > > creating mappings there, let's make sure uretprobe syscall checks
> > > > for that.
> > >
> > > It would be a layering violation, but we could perhaps do better here:
> > >
> > > > -       if (regs->ip != trampoline_check_ip())
> > > > +       /* Make sure the ip matches the only allowed sys_uretprobe caller. */
> > > > +       if (unlikely(regs->ip != trampoline_check_ip(tramp)))
> > > >                 goto sigill;
> > >
> > > Instead of SIGILL, perhaps this should do the seccomp action?  So the
> > > logic in seccomp would be (sketchily, with some real mode1 mess):
> > >
> > > if (is_a_real_uretprobe())
> > >     skip seccomp;
> >
> > IIUC you want to move the address check earlier to the seccomp path..
> > with the benefit that we would kill not allowed caller sooner?
> 
> The benefit would be that seccomp users that want to do something
> other than killing a process (returning an error code, getting
> notified, etc) could retain that functionality without the new
> automatic hole being poked for uretprobe() in cases where uprobes
> aren't in use or where the calling address doesn't match the uprobe
> trampoline.  IOW it would reduce the scope to which we're making
> seccomp behave unexpectedly.

Kees, any thoughts about this approach?

thanks,
jirka


> 
> >
> > jirka
> >
> > >
> > > where is_a_real_uretprobe() is only true if the nr and arch match
> > > uretprobe *and* the address is right.
> > >
> > > --Andy
> >
Re: [PATCHv3 perf/core] uprobes: Harden uretprobe syscall trampoline check
Posted by Jiri Olsa 9 months, 2 weeks ago
On Mon, Feb 17, 2025 at 01:09:43PM +0100, Jiri Olsa wrote:
> On Thu, Feb 13, 2025 at 09:58:29AM -0800, Andy Lutomirski wrote:
> > On Thu, Feb 13, 2025 at 1:16 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> > >
> > > On Wed, Feb 12, 2025 at 05:37:11PM -0800, Andy Lutomirski wrote:
> > > > On Wed, Feb 12, 2025 at 2:04 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > > > >
> > > > > Jann reported [1] possible issue when trampoline_check_ip returns
> > > > > address near the bottom of the address space that is allowed to
> > > > > call into the syscall if uretprobes are not set up.
> > > > >
> > > > > Though the mmap minimum address restrictions will typically prevent
> > > > > creating mappings there, let's make sure uretprobe syscall checks
> > > > > for that.
> > > >
> > > > It would be a layering violation, but we could perhaps do better here:
> > > >
> > > > > -       if (regs->ip != trampoline_check_ip())
> > > > > +       /* Make sure the ip matches the only allowed sys_uretprobe caller. */
> > > > > +       if (unlikely(regs->ip != trampoline_check_ip(tramp)))
> > > > >                 goto sigill;
> > > >
> > > > Instead of SIGILL, perhaps this should do the seccomp action?  So the
> > > > logic in seccomp would be (sketchily, with some real mode1 mess):
> > > >
> > > > if (is_a_real_uretprobe())
> > > >     skip seccomp;
> > >
> > > IIUC you want to move the address check earlier to the seccomp path..
> > > with the benefit that we would kill not allowed caller sooner?
> > 
> > The benefit would be that seccomp users that want to do something
> > other than killing a process (returning an error code, getting
> > notified, etc) could retain that functionality without the new
> > automatic hole being poked for uretprobe() in cases where uprobes
> > aren't in use or where the calling address doesn't match the uprobe
> > trampoline.  IOW it would reduce the scope to which we're making
> > seccomp behave unexpectedly.
> 
> Kees, any thoughts about this approach?

ping, any idea?

thanks,
jirka

> 
> thanks,
> jirka
> 
> 
> > 
> > >
> > > jirka
> > >
> > > >
> > > > where is_a_real_uretprobe() is only true if the nr and arch match
> > > > uretprobe *and* the address is right.
> > > >
> > > > --Andy
> > >
Re: [PATCHv3 perf/core] uprobes: Harden uretprobe syscall trampoline check
Posted by Ingo Molnar 9 months, 2 weeks ago
* Jiri Olsa <olsajiri@gmail.com> wrote:

> On Mon, Feb 17, 2025 at 01:09:43PM +0100, Jiri Olsa wrote:
> > On Thu, Feb 13, 2025 at 09:58:29AM -0800, Andy Lutomirski wrote:
> > > On Thu, Feb 13, 2025 at 1:16 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> > > >
> > > > On Wed, Feb 12, 2025 at 05:37:11PM -0800, Andy Lutomirski wrote:
> > > > > On Wed, Feb 12, 2025 at 2:04 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > > > > >
> > > > > > Jann reported [1] possible issue when trampoline_check_ip returns
> > > > > > address near the bottom of the address space that is allowed to
> > > > > > call into the syscall if uretprobes are not set up.
> > > > > >
> > > > > > Though the mmap minimum address restrictions will typically prevent
> > > > > > creating mappings there, let's make sure uretprobe syscall checks
> > > > > > for that.
> > > > >
> > > > > It would be a layering violation, but we could perhaps do better here:
> > > > >
> > > > > > -       if (regs->ip != trampoline_check_ip())
> > > > > > +       /* Make sure the ip matches the only allowed sys_uretprobe caller. */
> > > > > > +       if (unlikely(regs->ip != trampoline_check_ip(tramp)))
> > > > > >                 goto sigill;
> > > > >
> > > > > Instead of SIGILL, perhaps this should do the seccomp action?  So the
> > > > > logic in seccomp would be (sketchily, with some real mode1 mess):
> > > > >
> > > > > if (is_a_real_uretprobe())
> > > > >     skip seccomp;
> > > >
> > > > IIUC you want to move the address check earlier to the seccomp path..
> > > > with the benefit that we would kill not allowed caller sooner?
> > > 
> > > The benefit would be that seccomp users that want to do something
> > > other than killing a process (returning an error code, getting
> > > notified, etc) could retain that functionality without the new
> > > automatic hole being poked for uretprobe() in cases where uprobes
> > > aren't in use or where the calling address doesn't match the uprobe
> > > trampoline.  IOW it would reduce the scope to which we're making
> > > seccomp behave unexpectedly.
> > 
> > Kees, any thoughts about this approach?
> 
> ping, any idea?

So in any case I think the seccomp QoL tie-in suggested by Andy should 
be done in a separate patch, and I've applied the -v3 patch to 
tip:perf/core as-is.

( I've added Alexei's Acked-by too, which as I've read the v2 thread's 
  discussion was a given as long as his ~0 suggestion was implemented,
  which you did. )

Thanks,

	Ingo
[tip: perf/core] uprobes/x86: Harden uretprobe syscall trampoline check
Posted by tip-bot2 for Jiri Olsa 9 months, 2 weeks ago
The following commit has been merged into the perf/core branch of tip:

Commit-ID:     fa6192adc32f4fdfe5b74edd5b210e12afd6ecc0
Gitweb:        https://git.kernel.org/tip/fa6192adc32f4fdfe5b74edd5b210e12afd6ecc0
Author:        Jiri Olsa <jolsa@kernel.org>
AuthorDate:    Wed, 12 Feb 2025 23:04:33 +01:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Thu, 06 Mar 2025 12:22:45 +01:00

uprobes/x86: Harden uretprobe syscall trampoline check

Jann reported a possible issue when trampoline_check_ip returns
address near the bottom of the address space that is allowed to
call into the syscall if uretprobes are not set up:

   https://lore.kernel.org/bpf/202502081235.5A6F352985@keescook/T/#m9d416df341b8fbc11737dacbcd29f0054413cbbf

Though the mmap minimum address restrictions will typically prevent
creating mappings there, let's make sure uretprobe syscall checks
for that.

Fixes: ff474a78cef5 ("uprobe: Add uretprobe syscall to speed up return probe")
Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Kees Cook <kees@kernel.org>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Acked-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/r/20250212220433.3624297-1-jolsa@kernel.org
---
 arch/x86/kernel/uprobes.c | 14 +++++++++-----
 include/linux/uprobes.h   |  2 ++
 kernel/events/uprobes.c   |  2 +-
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 5a952c5..9194695 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -357,19 +357,23 @@ void *arch_uprobe_trampoline(unsigned long *psize)
 	return &insn;
 }
 
-static unsigned long trampoline_check_ip(void)
+static unsigned long trampoline_check_ip(unsigned long tramp)
 {
-	unsigned long tramp = uprobe_get_trampoline_vaddr();
-
 	return tramp + (uretprobe_syscall_check - uretprobe_trampoline_entry);
 }
 
 SYSCALL_DEFINE0(uretprobe)
 {
 	struct pt_regs *regs = task_pt_regs(current);
-	unsigned long err, ip, sp, r11_cx_ax[3];
+	unsigned long err, ip, sp, r11_cx_ax[3], tramp;
+
+	/* If there's no trampoline, we are called from wrong place. */
+	tramp = uprobe_get_trampoline_vaddr();
+	if (unlikely(tramp == UPROBE_NO_TRAMPOLINE_VADDR))
+		goto sigill;
 
-	if (regs->ip != trampoline_check_ip())
+	/* Make sure the ip matches the only allowed sys_uretprobe caller. */
+	if (unlikely(regs->ip != trampoline_check_ip(tramp)))
 		goto sigill;
 
 	err = copy_from_user(r11_cx_ax, (void __user *)regs->sp, sizeof(r11_cx_ax));
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index a40efdd..2e46b69 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -39,6 +39,8 @@ struct page;
 
 #define MAX_URETPROBE_DEPTH		64
 
+#define UPROBE_NO_TRAMPOLINE_VADDR	(~0UL)
+
 struct uprobe_consumer {
 	/*
 	 * handler() can return UPROBE_HANDLER_REMOVE to signal the need to
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 921ad91..70c84b9 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -2169,8 +2169,8 @@ void uprobe_copy_process(struct task_struct *t, unsigned long flags)
  */
 unsigned long uprobe_get_trampoline_vaddr(void)
 {
+	unsigned long trampoline_vaddr = UPROBE_NO_TRAMPOLINE_VADDR;
 	struct xol_area *area;
-	unsigned long trampoline_vaddr = -1;
 
 	/* Pairs with xol_add_vma() smp_store_release() */
 	area = READ_ONCE(current->mm->uprobes_state.xol_area); /* ^^^ */