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

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

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>