* [PATCHv2 perf/core] uprobes: Harden uretprobe syscall trampoline check
@ 2025-02-11 11:15 Jiri Olsa
2025-02-11 16:47 ` Andrii Nakryiko
0 siblings, 1 reply; 6+ messages in thread
From: Jiri Olsa @ 2025-02-11 11:15 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov, Peter Zijlstra,
Andrii Nakryiko
Cc: Kees Cook, Eyal Birger, stable, Jann Horn, linux-kernel,
linux-trace-kernel, linux-api, x86, bpf, Thomas Gleixner,
Ingo Molnar, Andy Lutomirski, Deepak Gupta, Stephen Rothwell
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")
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>
---
v2 changes:
- adding UPROBE_NO_TRAMPOLINE_VADDR macro (Andrii)
- rebased on top of perf/core
arch/x86/kernel/uprobes.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 5a952c5ea66b..e8d3c59aa9f7 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -357,19 +357,25 @@ 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);
}
+#define UPROBE_NO_TRAMPOLINE_VADDR ((unsigned long)-1)
+
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 (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 (regs->ip != trampoline_check_ip(tramp))
goto sigill;
err = copy_from_user(r11_cx_ax, (void __user *)regs->sp, sizeof(r11_cx_ax));
--
2.48.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCHv2 perf/core] uprobes: Harden uretprobe syscall trampoline check
2025-02-11 11:15 [PATCHv2 perf/core] uprobes: Harden uretprobe syscall trampoline check Jiri Olsa
@ 2025-02-11 16:47 ` Andrii Nakryiko
2025-02-11 16:52 ` Alexei Starovoitov
0 siblings, 1 reply; 6+ messages in thread
From: Andrii Nakryiko @ 2025-02-11 16:47 UTC (permalink / raw)
To: Jiri Olsa
Cc: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov, Peter Zijlstra,
Andrii Nakryiko, Kees Cook, Eyal Birger, stable, Jann Horn,
linux-kernel, linux-trace-kernel, linux-api, x86, bpf,
Thomas Gleixner, Ingo Molnar, Andy Lutomirski, Deepak Gupta,
Stephen Rothwell
On Tue, Feb 11, 2025 at 3:16 AM 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")
> 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>
> ---
> v2 changes:
> - adding UPROBE_NO_TRAMPOLINE_VADDR macro (Andrii)
> - rebased on top of perf/core
>
> arch/x86/kernel/uprobes.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 5a952c5ea66b..e8d3c59aa9f7 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -357,19 +357,25 @@ 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);
> }
>
> +#define UPROBE_NO_TRAMPOLINE_VADDR ((unsigned long)-1)
> +
> 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 (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 (regs->ip != trampoline_check_ip(tramp))
> goto sigill;
>
LGTM. I don't know if that would make any difference, but I'd sprinkle
unlikely() around these two conditions to make sure they don't
interfere with instruction flow much
Acked-by: Andrii Nakryiko <andrii@kernel.org>
> err = copy_from_user(r11_cx_ax, (void __user *)regs->sp, sizeof(r11_cx_ax));
> --
> 2.48.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCHv2 perf/core] uprobes: Harden uretprobe syscall trampoline check
2025-02-11 16:47 ` Andrii Nakryiko
@ 2025-02-11 16:52 ` Alexei Starovoitov
2025-02-11 16:59 ` Oleg Nesterov
0 siblings, 1 reply; 6+ messages in thread
From: Alexei Starovoitov @ 2025-02-11 16:52 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Jiri Olsa, Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Peter Zijlstra, Andrii Nakryiko, Kees Cook, Eyal Birger, stable,
Jann Horn, LKML, linux-trace-kernel, Linux API, X86 ML, bpf,
Thomas Gleixner, Ingo Molnar, Andy Lutomirski, Deepak Gupta,
Stephen Rothwell
On Tue, Feb 11, 2025 at 8:48 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Feb 11, 2025 at 3:16 AM 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")
> > 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>
> > ---
> > v2 changes:
> > - adding UPROBE_NO_TRAMPOLINE_VADDR macro (Andrii)
> > - rebased on top of perf/core
> >
> > arch/x86/kernel/uprobes.c | 16 +++++++++++-----
> > 1 file changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> > index 5a952c5ea66b..e8d3c59aa9f7 100644
> > --- a/arch/x86/kernel/uprobes.c
> > +++ b/arch/x86/kernel/uprobes.c
> > @@ -357,19 +357,25 @@ 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);
> > }
> >
> > +#define UPROBE_NO_TRAMPOLINE_VADDR ((unsigned long)-1)
If you respin anyway maybe use ~0UL instead?
In the above and in
uprobe_get_trampoline_vaddr(),
since
unsigned long trampoline_vaddr = -1;
looks odd too.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCHv2 perf/core] uprobes: Harden uretprobe syscall trampoline check
2025-02-11 16:52 ` Alexei Starovoitov
@ 2025-02-11 16:59 ` Oleg Nesterov
2025-02-12 4:05 ` Masami Hiramatsu
0 siblings, 1 reply; 6+ messages in thread
From: Oleg Nesterov @ 2025-02-11 16:59 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Andrii Nakryiko, Jiri Olsa, Steven Rostedt, Masami Hiramatsu,
Peter Zijlstra, Andrii Nakryiko, Kees Cook, Eyal Birger, stable,
Jann Horn, LKML, linux-trace-kernel, Linux API, X86 ML, bpf,
Thomas Gleixner, Ingo Molnar, Andy Lutomirski, Deepak Gupta,
Stephen Rothwell
On 02/11, Alexei Starovoitov wrote:
>
> > > +#define UPROBE_NO_TRAMPOLINE_VADDR ((unsigned long)-1)
>
> If you respin anyway maybe use ~0UL instead?
> In the above and in
> uprobe_get_trampoline_vaddr(),
> since
>
> unsigned long trampoline_vaddr = -1;
... or -1ul in both cases.
I agree, UPROBE_NO_TRAMPOLINE_VADDR has a single user, looks
a bit strange...
Oleg.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCHv2 perf/core] uprobes: Harden uretprobe syscall trampoline check
2025-02-11 16:59 ` Oleg Nesterov
@ 2025-02-12 4:05 ` Masami Hiramatsu
2025-02-12 13:51 ` Jiri Olsa
0 siblings, 1 reply; 6+ messages in thread
From: Masami Hiramatsu @ 2025-02-12 4:05 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Alexei Starovoitov, Andrii Nakryiko, Jiri Olsa, Steven Rostedt,
Masami Hiramatsu, Peter Zijlstra, Andrii Nakryiko, Kees Cook,
Eyal Birger, stable, Jann Horn, LKML, linux-trace-kernel,
Linux API, X86 ML, bpf, Thomas Gleixner, Ingo Molnar,
Andy Lutomirski, Deepak Gupta, Stephen Rothwell
On Tue, 11 Feb 2025 17:59:41 +0100
Oleg Nesterov <oleg@redhat.com> wrote:
> On 02/11, Alexei Starovoitov wrote:
> >
> > > > +#define UPROBE_NO_TRAMPOLINE_VADDR ((unsigned long)-1)
> >
> > If you respin anyway maybe use ~0UL instead?
> > In the above and in
> > uprobe_get_trampoline_vaddr(),
> > since
> >
> > unsigned long trampoline_vaddr = -1;
>
> ... or -1ul in both cases.
>
> I agree, UPROBE_NO_TRAMPOLINE_VADDR has a single user, looks
> a bit strange...
I think both this function and uprobe_get_trampoline_vaddr()
should use the same macro as a token.
(and ~0UL is a bit more comfortable for me too :) )
----
unsigned long uprobe_get_trampoline_vaddr(void)
{
struct xol_area *area;
unsigned long trampoline_vaddr = -1;
----
Thank you,
>
> Oleg.
>
>
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCHv2 perf/core] uprobes: Harden uretprobe syscall trampoline check
2025-02-12 4:05 ` Masami Hiramatsu
@ 2025-02-12 13:51 ` Jiri Olsa
0 siblings, 0 replies; 6+ messages in thread
From: Jiri Olsa @ 2025-02-12 13:51 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Oleg Nesterov, Alexei Starovoitov, Andrii Nakryiko,
Steven Rostedt, Peter Zijlstra, Andrii Nakryiko, Kees Cook,
Eyal Birger, stable, Jann Horn, LKML, linux-trace-kernel,
Linux API, X86 ML, bpf, Thomas Gleixner, Ingo Molnar,
Andy Lutomirski, Deepak Gupta, Stephen Rothwell
On Wed, Feb 12, 2025 at 01:05:09PM +0900, Masami Hiramatsu wrote:
> On Tue, 11 Feb 2025 17:59:41 +0100
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > On 02/11, Alexei Starovoitov wrote:
> > >
> > > > > +#define UPROBE_NO_TRAMPOLINE_VADDR ((unsigned long)-1)
> > >
> > > If you respin anyway maybe use ~0UL instead?
> > > In the above and in
> > > uprobe_get_trampoline_vaddr(),
> > > since
> > >
> > > unsigned long trampoline_vaddr = -1;
> >
> > ... or -1ul in both cases.
> >
> > I agree, UPROBE_NO_TRAMPOLINE_VADDR has a single user, looks
> > a bit strange...
>
> I think both this function and uprobe_get_trampoline_vaddr()
> should use the same macro as a token.
> (and ~0UL is a bit more comfortable for me too :) )
>
> ----
> unsigned long uprobe_get_trampoline_vaddr(void)
> {
> struct xol_area *area;
> unsigned long trampoline_vaddr = -1;
> ----
sounds good, I'll send new version with change below if there
are no objections
thanks,
jirka
---
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 5a952c5ea66b..015b2a6bac11 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 (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 (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 b1df7d792fa1..a6bec560bdbc 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 2ca797cbe465..e8af2f75b094 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -2160,8 +2160,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); /* ^^^ */
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-02-12 13:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-11 11:15 [PATCHv2 perf/core] uprobes: Harden uretprobe syscall trampoline check Jiri Olsa
2025-02-11 16:47 ` Andrii Nakryiko
2025-02-11 16:52 ` Alexei Starovoitov
2025-02-11 16:59 ` Oleg Nesterov
2025-02-12 4:05 ` Masami Hiramatsu
2025-02-12 13:51 ` Jiri Olsa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).