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