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

[Oleg Nesterov <oleg@xxxxxxxxxx>]

On 03/20, Oleg Nesterov wrote:
>
> On 03/20, Jiri Olsa wrote:
> >
> > are you ok if I squash the patches together
>
> Yes, thanks, I am fine.
>
> But lets discuss this change a bit more. So, with this poc we have the
> (intentionally) oversimplified
>
> 	SYSCALL_DEFINE0(uretprobe)
> 	{
> 		struct pt_regs *regs = task_pt_regs(current);
> 		unsigned long __user *ax_and_ret = (unsigned long __user *)regs->sp + 2;
> 		unsigned long ip, err;
>
> 		ip = regs->ip;
> 		regs->orig_ax = -1;
> 		err = get_user(regs->ax, ax_and_ret);
> 		WARN_ON_ONCE(err);
>
> 		uprobe_handle_trampoline(regs);
>
> 		err = put_user(regs->ip, ax_and_ret);
> 		WARN_ON_ONCE(err);
> 		regs->ip = ip;
>
> 		return regs->ax;
> 	}
>
> I have no idea what uprobe consumers / bpf programs can do, so let me ask:
>
> 	- uprobe_consumer's will see the "wrong" values of regs->cx/r11/sp
> 	  Is it OK? If not - easy to fix.
>
> 	- can uprobe_consumer change regs->cx/r11 ? If yes - easy to fix.
>
> 	- can uprobe_consumer change regs->sp ? If yes - easy to fix too,
> 	  but needs a separate check/code.

IOW. If answer is "yes" to all the questions above, then we probably need
something like

	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;

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

		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.