On Wed, 12 Feb 2025 23:04:33 +0100 Jiri Olsa <jo...@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 <k...@kernel.org> > Cc: Eyal Birger <eyal.bir...@gmail.com> > Cc: sta...@vger.kernel.org > Fixes: ff474a78cef5 ("uprobe: Add uretprobe syscall to speed up return probe") > Acked-by: Andrii Nakryiko <and...@kernel.org> > Reported-by: Jann Horn <ja...@google.com> > Reviewed-by: Oleg Nesterov <o...@redhat.com> > Reviewed-by: Kees Cook <k...@kernel.org> > Signed-off-by: Jiri Olsa <jo...@kernel.org> Looks good to me. Acked-by: Masami Hiramatsu (Google) <mhira...@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) <mhira...@kernel.org>
