Re: [PATCHv4 bpf-next 2/7] uprobe: Add uretprobe syscall to speed up return probe

["Edgecombe, Rick P" <rick.p.edgecombe@xxxxxxxxx>]

On Fri, 2024-05-03 at 22:17 +0200, Jiri Olsa wrote:
> when uretprobe is created, kernel overwrites the return address on user
> stack to point to user space trampoline, so the setup is in kernel hands

I mean for uprobes in general. I'm didn't have any specific ideas in mind, but
in general when we give the kernel more abilities around shadow stack we have to
think if attackers could use it to work around shadow stack protections.

> 
> with the hack below on top of this patchset I'm no longer seeing shadow
> stack app crash on uretprobe.. I'll try to polish it and send out next
> week, any suggestions are welcome ;-)

Thanks. Some comments below.

> 
> thanks,
> jirka
> 
> 
> ---
> diff --git a/arch/x86/include/asm/shstk.h b/arch/x86/include/asm/shstk.h
> index 42fee8959df7..d374305a6851 100644
> --- a/arch/x86/include/asm/shstk.h
> +++ b/arch/x86/include/asm/shstk.h
> @@ -21,6 +21,8 @@ unsigned long shstk_alloc_thread_stack(struct task_struct
> *p, unsigned long clon
>  void shstk_free(struct task_struct *p);
>  int setup_signal_shadow_stack(struct ksignal *ksig);
>  int restore_signal_shadow_stack(void);
> +void uprobe_change_stack(unsigned long addr);
> +void uprobe_push_stack(unsigned long addr);

Maybe name them:
shstk_update_last_frame();
shstk_push_frame();


>  #else
>  static inline long shstk_prctl(struct task_struct *task, int option,
>                                unsigned long arg2) { return -EINVAL; }
> diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
> index 59e15dd8d0f8..804c446231d9 100644
> --- a/arch/x86/kernel/shstk.c
> +++ b/arch/x86/kernel/shstk.c
> @@ -577,3 +577,24 @@ long shstk_prctl(struct task_struct *task, int option,
> unsigned long arg2)
>                 return wrss_control(true);
>         return -EINVAL;
>  }
> +
> +void uprobe_change_stack(unsigned long addr)
> +{
> +       unsigned long ssp;

Probably want something like:

	if (!features_enabled(ARCH_SHSTK_SHSTK))
		return;

So this doesn't try the below if shadow stack is disabled.

> +
> +       ssp = get_user_shstk_addr();
> +       write_user_shstk_64((u64 __user *)ssp, (u64)addr);
> +}

Can we know that there was a valid return address just before this point on the
stack? Or could it be a sigframe or something?

> +
> +void uprobe_push_stack(unsigned long addr)
> +{
> +       unsigned long ssp;

	if (!features_enabled(ARCH_SHSTK_SHSTK))
		return;

> +
> +       ssp = get_user_shstk_addr();
> +       ssp -= SS_FRAME_SIZE;
> +       write_user_shstk_64((u64 __user *)ssp, (u64)addr);
> +
> +       fpregs_lock_and_load();
> +       wrmsrl(MSR_IA32_PL3_SSP, ssp);
> +       fpregs_unlock();
> +}
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 81e6ee95784d..259457838020 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -416,6 +416,7 @@ SYSCALL_DEFINE0(uretprobe)
>         regs->r11 = regs->flags;
>         regs->cx  = regs->ip;
>  
> +       uprobe_push_stack(r11_cx_ax[2]);

I'm concerned this could be used to push arbitrary frames to the shadow stack.
Couldn't an attacker do a jump to the point that calls this syscall? Maybe this
is what peterz was raising.

>         return regs->ax;
>  
>  sigill:
> @@ -1191,8 +1192,10 @@ arch_uretprobe_hijack_return_addr(unsigned long
> trampoline_vaddr, struct pt_regs
>                 return orig_ret_vaddr;
>  
>         nleft = copy_to_user((void __user *)regs->sp, &trampoline_vaddr,
> rasize);
> -       if (likely(!nleft))
> +       if (likely(!nleft)) {
> +               uprobe_change_stack(trampoline_vaddr);
>                 return orig_ret_vaddr;
> +       }
>  
>         if (nleft != rasize) {
>                 pr_err("return address clobbered: pid=%d, %%sp=%#lx,
> %%ip=%#lx\n",