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

[Oleg Nesterov <oleg@redhat.com>]

On 03/20, Jiri Olsa wrote:
>
> On Wed, Mar 20, 2024 at 10:44:30AM -0700, Andrii Nakryiko wrote:
> > On Wed, Mar 20, 2024 at 8:30 AM Oleg Nesterov <oleg@redhat.com> wrote:
> > >
> > > > 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)
>
> I think we should keep the same behaviour as it was for the trap,
> so I think we should restore all registers and allow consumer to change it

OK, agreed. Then something like the code below.

Oleg.

> > >         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.
> > >
>