Messages in this thread | | | Subject | Re: UBSAN: array-index-out-of-bounds in kernel/bpf/arraymap.c:177 | From | Yonghong Song <> | Date | Mon, 18 May 2020 18:36:26 -0700 |
| |
On 5/18/20 6:30 PM, Andrii Nakryiko wrote: > On Mon, May 18, 2020 at 6:00 PM Yonghong Song <yhs@fb.com> wrote: >> >> >> >> On 5/18/20 5:25 PM, Andrii Nakryiko wrote: >>> On Mon, May 18, 2020 at 5:09 PM Qian Cai <cai@lca.pw> wrote: >>>> >>>> On Mon, May 18, 2020 at 7:55 PM Andrii Nakryiko >>>> <andrii.nakryiko@gmail.com> wrote: >>>>> >>>>> On Sun, May 17, 2020 at 7:45 PM Qian Cai <cai@lca.pw> wrote: >>>>>> >>>>>> With Clang 9.0.1, >>>>>> >>>>>> return array->value + array->elem_size * (index & array->index_mask); >>>>>> >>>>>> but array->value is, >>>>>> >>>>>> char value[0] __aligned(8); >>>>> >>>>> This, and ptrs and pptrs, should be flexible arrays. But they are in a >>>>> union, and unions don't support flexible arrays. Putting each of them >>>>> into anonymous struct field also doesn't work: >>>>> >>>>> /data/users/andriin/linux/include/linux/bpf.h:820:18: error: flexible >>>>> array member in a struct with no named members >>>>> struct { void *ptrs[] __aligned(8); }; >>>>> >>>>> So it probably has to stay this way. Is there a way to silence UBSAN >>>>> for this particular case? >>>> >>>> I am not aware of any way to disable a particular function in UBSAN >>>> except for the whole file in kernel/bpf/Makefile, >>>> >>>> UBSAN_SANITIZE_arraymap.o := n >>>> >>>> If there is no better way to do it, I'll send a patch for it. >>> >>> >>> That's probably going to be too drastic, we still would want to >>> validate the rest of arraymap.c code, probably. Not sure, maybe >>> someone else has better ideas. >> >> Maybe something like below? >> >> struct bpf_array { >> struct bpf_map map; >> u32 elem_size; >> u32 index_mask; >> struct bpf_array_aux *aux; >> union { >> char value; >> void *ptrs; >> void __percpu *pptrs; >> } u[] __aligned(8); > > That will require wider code changes, and would look quite unnatural: > > array->u[whatever].pptrs > > instead of current > > array->pptrs[whatever]
Right. There will be a tradeoff between to make it work vs. some code ugliness :-). BTW, I don't have a strong preference on how to solve this particular issue.
> >> };
| |