On Wed, Mar 20, 2024 at 8:30 AM Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
>
> 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
yes to first, so ideally we fix registers to "correct" values
(especially sp), but no to the last two (at least as far as BPF is
concerned)
>
> 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.
>
