On Tue, Oct 29, 2019 at 8:16 AM Andrey Ignatov <rdna@xxxxxx> wrote:
>
> Ilya Leoshkevich <iii@xxxxxxxxxxxxx> [Tue, 2019-10-29 07:20 -0700]:
> > > Am 29.10.2019 um 05:36 schrieb Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx>:
> > >
> > > On Mon, Oct 28, 2019 at 1:09 PM Ilya Leoshkevich <iii@xxxxxxxxxxxxx> 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@xxxxxx>
>
> > 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
