Re: [PATCH RFC bpf-next 4/3] uprobe: ensure sys_uretprobe uses sysret

[Jiri Olsa <olsajiri@gmail.com>]

On Wed, Mar 20, 2024 at 04:28:48PM +0100, Oleg Nesterov wrote:

SNIP

> 	SYSCALL_DEFINE0(uretprobe)
> 	{
> 		struct pt_regs *regs = task_pt_regs(current);
> 		unsigned long err, ip, sp, r11_cx_ax[3];
> 
> 		err = copy_from_user(r11_cx_ax, (void __user*)regs->sp, sizeof(r11_cx_ax));
> 		WARN_ON_ONCE(err);
> 
> 		// Q1: apart from ax, do we really care?
> 		// expose the "right" values of r11/cx/ax/sp to uprobe_consumer's
> 		regs->r11 = r11_cx_ax[0];
> 		regs->cx  = r11_cx_ax[1];
> 		regs->ax  = r11_cx_ax[2];
> 		regs->sp += sizeof(r11_cx_ax);
> 		regs->orig_ax = -1;
> 
> 		ip = regs->ip;
> 		sp = regs->sp;
> 
> 		uprobe_handle_trampoline(regs);
> 
> 		// Q2: is it possible? do we care?
> 		// uprobe_consumer has changed sp, we can do nothing,
> 		// just return via iret.
> 		if (regs->sp != sp)
> 			return regs->ax;
> 		regs->sp -= sizeof(r11_cx_ax);
> 
> 		// Q3: is it possible? do we care?
> 		// for the case uprobe_consumer has changed r11/cx
> 		r11_cx_ax[0] = regs->r11;
> 		r11_cx_ax[1] = regs->cx;

I wonder we could add test for this as well, and check we return
proper register values in case the consuer changed them, will check

> 
> 		// comment to explain this hack
> 		r11_cx_ax[2] = regs->ip;
> 		regs->ip = ip;

we still need restore regs->ip in case do_syscall_64 decides to do
iret for some reason, right?

overall lgtm, thanks

jirka

> 
> 		err = copy_to_user((void __user*)regs->sp, r11_cx_ax, sizeof(r11_cx_ax));
> 		WARN_ON_ONCE(err);
> 
> 		// ensure sysret, see do_syscall_64()
> 		regs->r11 = regs->flags;
> 		regs->cx  = regs->ip;
> 
> 		return regs->ax;
> 	}
> 
> Oleg.
>