Re: [PATCH bpf] bpf: allow narrow loads of bpf_sysctl fields with offset > 0

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Tue, Oct 29, 2019 at 8:16 AM Andrey Ignatov <rdna@fb.com> wrote:
>
> Ilya Leoshkevich <iii@linux.ibm.com> [Tue, 2019-10-29 07:20 -0700]:
> > > Am 29.10.2019 um 05:36 schrieb Andrii Nakryiko <andrii.nakryiko@gmail.com>:
> > >
> > > On Mon, Oct 28, 2019 at 1:09 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
> > >>
> > >> --- a/kernel/bpf/cgroup.c
> > >> +++ b/kernel/bpf/cgroup.c
> > >> @@ -1311,12 +1311,12 @@ static bool sysctl_is_valid_access(int off, int size, enum bpf_access_type type,
> > >>                return false;
> > >>
> > >>        switch (off) {
> > >> -       case offsetof(struct bpf_sysctl, write):
> > >> +       case bpf_ctx_range(struct bpf_sysctl, write):
> > >
> > > this will actually allow reads pas t write field (e.g., offset = 2, size = 4).
> >
> > Wouldn't
> >
> >       if (off < 0 || off + size > sizeof(struct bpf_sysctl) || off % size)
> >               return false;
> >
> > prevent all OOB read-write attempts? Especially the off % size part - I
> > think it has the effect of preventing OOB accesses for fields. In
> > particular, it would filter offset = 2, size = 4 case.
>
> Yes, it would. This code makes sure that narrow accesses are aligned so
> that offset = 2 would allow only size = 2 or size = 1.

Yes, you both are right, I missed the "off % size" check above.
Thanks. Looks good to me as well.

Acked-by: Andrii Nakryiko <andriin@fb.com>

>
> > I have also checked the other usages of bpf_ctx_range, for example,
> > bpf_skb_is_valid_access, and they don't seem to be doing anything
> > special.
>
> Yes, sysctl hook follows logic similar to that of other program types.
>
> > >>                if (type != BPF_READ)
> > >>                        return false;
> > >>                bpf_ctx_record_field_size(info, size_default);
> > >>                return bpf_ctx_narrow_access_ok(off, size, size_default);
> > >> -       case offsetof(struct bpf_sysctl, file_pos):
> > >> +       case bpf_ctx_range(struct bpf_sysctl, file_pos)
> > >
> > > this will allow read past context struct altogether. When we allow
> > > ranges, we will have to adjust allowed read size.
> >
> > Same here.
>
> --
> Andrey Ignatov