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

[Ilya Leoshkevich <iii@linux.ibm.com>]

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

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.

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