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

[Andrey Ignatov <rdna@fb.com>]

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.

> 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