* [PATCH bpf-next] uprobes: Harden uretprobe syscall trampoline check
@ 2025-02-09 22:05 Jiri Olsa
2025-02-10 12:07 ` Oleg Nesterov
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Jiri Olsa @ 2025-02-09 22:05 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
Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
arch/x86/kernel/uprobes.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 5a952c5ea66b..109d6641a1b3 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 == -1)
+ 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] 5+ messages in thread
* Re: [PATCH bpf-next] uprobes: Harden uretprobe syscall trampoline check
2025-02-09 22:05 [PATCH bpf-next] uprobes: Harden uretprobe syscall trampoline check Jiri Olsa
@ 2025-02-10 12:07 ` Oleg Nesterov
2025-02-10 17:26 ` Andrii Nakryiko
2025-02-10 23:00 ` Kees Cook
2 siblings, 0 replies; 5+ messages in thread
From: Oleg Nesterov @ 2025-02-10 12:07 UTC (permalink / raw)
To: Jiri Olsa
Cc: Steven Rostedt, Masami Hiramatsu, 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 02/09, Jiri Olsa wrote:
>
> [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
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> arch/x86/kernel/uprobes.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next] uprobes: Harden uretprobe syscall trampoline check
2025-02-09 22:05 [PATCH bpf-next] uprobes: Harden uretprobe syscall trampoline check Jiri Olsa
2025-02-10 12:07 ` Oleg Nesterov
@ 2025-02-10 17:26 ` Andrii Nakryiko
2025-02-11 8:35 ` Jiri Olsa
2025-02-10 23:00 ` Kees Cook
2 siblings, 1 reply; 5+ messages in thread
From: Andrii Nakryiko @ 2025-02-10 17:26 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 Sun, Feb 9, 2025 at 2:05 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.
>
> [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
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> arch/x86/kernel/uprobes.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 5a952c5ea66b..109d6641a1b3 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 == -1)
slight nit: mixing -1 and unsigned long looks sloppy. Maybe let's add
something like
#define UPROBE_NO_TRAMPOLINE_VADDR ((unsigned long)-1)
and return that from uprobe_get_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 [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next] uprobes: Harden uretprobe syscall trampoline check
2025-02-09 22:05 [PATCH bpf-next] uprobes: Harden uretprobe syscall trampoline check Jiri Olsa
2025-02-10 12:07 ` Oleg Nesterov
2025-02-10 17:26 ` Andrii Nakryiko
@ 2025-02-10 23:00 ` Kees Cook
2 siblings, 0 replies; 5+ messages in thread
From: Kees Cook @ 2025-02-10 23:00 UTC (permalink / raw)
To: Jiri Olsa
Cc: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov, Peter Zijlstra,
Andrii Nakryiko, 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 Sun, Feb 09, 2025 at 11:05:15PM +0100, Jiri Olsa 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
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Thanks for this!
Reviewed-by: Kees Cook <kees@kernel.org>
--
Kees Cook
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next] uprobes: Harden uretprobe syscall trampoline check
2025-02-10 17:26 ` Andrii Nakryiko
@ 2025-02-11 8:35 ` Jiri Olsa
0 siblings, 0 replies; 5+ messages in thread
From: Jiri Olsa @ 2025-02-11 8:35 UTC (permalink / raw)
To: Andrii Nakryiko
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 Mon, Feb 10, 2025 at 09:26:53AM -0800, Andrii Nakryiko wrote:
> On Sun, Feb 9, 2025 at 2:05 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.
> >
> > [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
> > Reported-by: Jann Horn <jannh@google.com>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > arch/x86/kernel/uprobes.c | 14 +++++++++-----
> > 1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> > index 5a952c5ea66b..109d6641a1b3 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 == -1)
>
> slight nit: mixing -1 and unsigned long looks sloppy. Maybe let's add
> something like
>
> #define UPROBE_NO_TRAMPOLINE_VADDR ((unsigned long)-1)
>
> and return that from uprobe_get_trampoline_vaddr()?
ok, will add that
thanks,
jirka
>
> > + 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 [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-02-11 8:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-09 22:05 [PATCH bpf-next] uprobes: Harden uretprobe syscall trampoline check Jiri Olsa
2025-02-10 12:07 ` Oleg Nesterov
2025-02-10 17:26 ` Andrii Nakryiko
2025-02-11 8:35 ` Jiri Olsa
2025-02-10 23:00 ` Kees Cook
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).