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

[Jiri Olsa <olsajiri@gmail.com>]

On Thu, Mar 21, 2024 at 01:14:56PM +0100, Oleg Nesterov wrote:
> On 03/21, Jiri Olsa wrote:
> >
> > On Thu, Mar 21, 2024 at 11:17:51AM +0100, Oleg Nesterov wrote:
> > > On 03/21, Jiri Olsa wrote:
> > > >
> > > > 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?
> > >
> > > I don't understand... could you spell?
> >
> > I was wondering why to restore regs->ip for sysret path, but do_syscall_64
> > can decide to do iret return (for which we need proper regs->ip) even if we
> > prepare cx/r11 registers for sysexit
> 
> Still don't understand... Yes, we prepare cx/r11 to avoid iret if possible.
> But (apart from performance) we do not care if do_syscall_64() picks iret.
> Either way
> 
> 			regs->ip = ip;
> 
> above ensures that usermode returns to uretprobe_syscall_entry right after
> the syscall insn. 

hm, I think above ensures that do_syscall_64 will skip the 'regs->cx != regs->ip'
check.. and after the sysret returns to rcx register value and ignores regs->ip

but in any case we need to set it

> ... Then popq %r11/cx will restore r11/cx even if they were
> changed by uprobe_consumer's. And then "retq" will return to the address
> "returned" by handle_trampoline(regs) because we do
> 
> 			// comment to explain this hack
> 			r11_cx_ax[2] = regs->ip;
> 
> after handle_trampoline(). This all doesn't depend on iret-or-sysret.
> 
> OK, I am sure you understand this, so I guess I misunderstood your concerns.

thanks for the patience ;-)

jirka