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