Re: [PATCHv2 bpf-next 1/2] bpf: Fail uprobe multi link with negative offset

[Jiri Olsa <olsajiri@gmail.com>]

On Mon, Dec 18, 2023 at 09:56:38AM -0800, Andrii Nakryiko wrote:
> On Sun, Dec 17, 2023 at 1:55 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Currently the __uprobe_register will return 0 (success) when called with
> > negative offset. The reason is that the call to register_for_each_vma and
> > then build_map_info won't return error for negative offset. They just won't
> > do anything - no matching vma is found so there's no registered breakpoint
> > for the uprobe.
> >
> > I don't think we can change the behaviour of __uprobe_register and fail
> > for negative uprobe offset, because apps might depend on that already.
> >
> > But I think we can still make the change and check for it on bpf multi
> > link syscall level.
> >
> > Also moving the __get_user call and check for the offsets to the top of
> > loop, to fail early without extra __get_user calls for ref_ctr_offset
> > and cookie arrays.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  kernel/trace/bpf_trace.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 97c0c49c40a0..492d60e9c480 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -3391,15 +3391,19 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> >                 goto error_free;
> >
> >         for (i = 0; i < cnt; i++) {
> > -               if (ucookies && __get_user(uprobes[i].cookie, ucookies + i)) {
> > +               if (__get_user(uprobes[i].offset, uoffsets + i)) {
> >                         err = -EFAULT;
> >                         goto error_free;
> >                 }
> > +               if (uprobes[i].offset < 0) {
> > +                       err = -EINVAL;
> > +                       goto error_free;
> > +               }
> 
> I applied this because it does fix the problem, but the whole
> reshuffle of offsets in front of cookies is pointless, because of the
> common for() loop. You are saving one or two __get_user() calls before
> failing.
> 
> If we really want to do validation first, reading offsets should be in
> its own for loop, then uref_ctr_offsets in its own, and then cookies
> in its own loop as well. That way we read and validate the entire
> array before reading another array. Please consider a follow up, if
> you think it's important enough.

ok, thanks

jirka

> 
> 
> >                 if (uref_ctr_offsets && __get_user(uprobes[i].ref_ctr_offset, uref_ctr_offsets + i)) {
> >                         err = -EFAULT;
> >                         goto error_free;
> >                 }
> > -               if (__get_user(uprobes[i].offset, uoffsets + i)) {
> > +               if (ucookies && __get_user(uprobes[i].cookie, ucookies + i)) {
> >                         err = -EFAULT;
> >                         goto error_free;
> >                 }
> > --
> > 2.43.0
> >