[bpf-next] uprobes: Harden uretprobe syscall trampoline check

Commit Message

Jiri Olsa Feb. 9, 2025, 10:05 p.m. UTC

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 <[e-mail]>
Cc: Eyal Birger <[e-mail]>
Cc: [e-mail]
Reported-by: Jann Horn <[e-mail]>
Signed-off-by: Jiri Olsa <[e-mail]>
---
 arch/x86/kernel/uprobes.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Comments

Oleg Nesterov Feb. 10, 2025, 12:07 p.m. UTC | #1

On 02/09, Jiri Olsa wrote:
>
> [1] https://lore.kernel.org/bpf/202502081235.5A6F352985@keescook/T/#m9d416df341b8fbc11737dacbcd29f0054413cbbf
> Cc: Kees Cook <[e-mail]>
> Cc: Eyal Birger <[e-mail]>
> Cc: [e-mail]
> Reported-by: Jann Horn <[e-mail]>
> Signed-off-by: Jiri Olsa <[e-mail]>
> ---
>  arch/x86/kernel/uprobes.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)

Reviewed-by: Oleg Nesterov <[e-mail]>

Andrii Nakryiko Feb. 10, 2025, 5:26 p.m. UTC | #2

On Sun, Feb 9, 2025 at 2:05 PM Jiri Olsa <[e-mail]> 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 <[e-mail]>
> Cc: Eyal Birger <[e-mail]>
> Cc: [e-mail]
> Reported-by: Jann Horn <[e-mail]>
> Signed-off-by: Jiri Olsa <[e-mail]>
> ---
>  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
>

Kees Cook Feb. 10, 2025, 11 p.m. UTC | #3

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 <[e-mail]>
> Cc: Eyal Birger <[e-mail]>
> Cc: [e-mail]
> Reported-by: Jann Horn <[e-mail]>
> Signed-off-by: Jiri Olsa <[e-mail]>

Thanks for this!

Reviewed-by: Kees Cook <[e-mail]>

Jiri Olsa Feb. 11, 2025, 8:35 a.m. UTC | #4

On Mon, Feb 10, 2025 at 09:26:53AM -0800, Andrii Nakryiko wrote:
> On Sun, Feb 9, 2025 at 2:05 PM Jiri Olsa <[e-mail]> 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 <[e-mail]>
> > Cc: Eyal Birger <[e-mail]>
> > Cc: [e-mail]
> > Reported-by: Jann Horn <[e-mail]>
> > Signed-off-by: Jiri Olsa <[e-mail]>
> > ---
> >  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
> >

Patch

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));