* UBSAN: array-index-out-of-bounds in kernel/bpf/arraymap.c:177 @ 2020-05-18 2:44 Qian Cai 2020-05-18 23:55 ` Andrii Nakryiko 0 siblings, 1 reply; 12+ messages in thread From: Qian Cai @ 2020-05-18 2:44 UTC (permalink / raw) To: Alexei Starovoitov, Daniel Borkmann Cc: Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, Linux Netdev List, bpf, Linux Kernel Mailing List, clang-built-linux With Clang 9.0.1, return array->value + array->elem_size * (index & array->index_mask); but array->value is, char value[0] __aligned(8); [ 506.031548][ T4134] LTP: starting bpf_prog02 [ 506.125326][ T4352] ================================================================================ [ 506.134603][ T4352] UBSAN: array-index-out-of-bounds in kernel/bpf/arraymap.c:177:22 [ 506.142521][ T4352] index 8 is out of range for type 'char [0]' [ 506.148613][ T4352] CPU: 222 PID: 4352 Comm: bpf_prog02 Tainted: G L 5.7.0-rc5-next-20200515 #2 [ 506.158632][ T4352] Hardware name: HPE Apollo 70 /C01_APACHE_MB , BIOS L50_5.13_1.11 06/18/2019 [ 506.169084][ T4352] Call trace: [ 506.172256][ T4352] dump_backtrace+0x0/0x22c [ 506.176634][ T4352] show_stack+0x28/0x34 [ 506.180666][ T4352] dump_stack+0x104/0x194 [ 506.184877][ T4352] __ubsan_handle_out_of_bounds+0xf0/0x120 [ 506.190565][ T4352] array_map_lookup_elem+0x90/0x94 [ 506.195560][ T4352] bpf_map_lookup_elem+0x48/0x60 [ 506.200383][ T4352] ___bpf_prog_run+0xe9c/0x2840 [ 506.205109][ T4352] __bpf_prog_run32+0x80/0xac [ 506.209673][ T4352] __bpf_prog_run_save_cb+0x104/0x46c [ 506.214919][ T4352] sk_filter_trim_cap+0x21c/0x2c4 [ 506.219823][ T4352] unix_dgram_sendmsg+0x45c/0x860 [ 506.224725][ T4352] sock_sendmsg+0x4c/0x74 [ 506.228935][ T4352] sock_write_iter+0x158/0x1a4 [ 506.233584][ T4352] __vfs_write+0x190/0x1d8 [ 506.237874][ T4352] vfs_write+0x13c/0x1b8 [ 506.241992][ T4352] ksys_write+0xb0/0x120 [ 506.246108][ T4352] __arm64_sys_write+0x54/0x88 [ 506.250747][ T4352] do_el0_svc+0x128/0x1dc [ 506.254957][ T4352] el0_sync_handler+0xd0/0x268 [ 506.259594][ T4352] el0_sync+0x164/0x180 [ 506.263747][ T4352] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: UBSAN: array-index-out-of-bounds in kernel/bpf/arraymap.c:177 2020-05-18 2:44 UBSAN: array-index-out-of-bounds in kernel/bpf/arraymap.c:177 Qian Cai @ 2020-05-18 23:55 ` Andrii Nakryiko 2020-05-19 0:09 ` Qian Cai 0 siblings, 1 reply; 12+ messages in thread From: Andrii Nakryiko @ 2020-05-18 23:55 UTC (permalink / raw) To: Qian Cai Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, Linux Netdev List, bpf, Linux Kernel Mailing List, clang-built-linux 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? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: UBSAN: array-index-out-of-bounds in kernel/bpf/arraymap.c:177 2020-05-18 23:55 ` Andrii Nakryiko @ 2020-05-19 0:09 ` Qian Cai 2020-05-19 0:25 ` Andrii Nakryiko 0 siblings, 1 reply; 12+ messages in thread From: Qian Cai @ 2020-05-19 0:09 UTC (permalink / raw) To: Andrii Nakryiko Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, Linux Netdev List, bpf, Linux Kernel Mailing List, clang-built-linux, Kees Cook 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: UBSAN: array-index-out-of-bounds in kernel/bpf/arraymap.c:177 2020-05-19 0:09 ` Qian Cai @ 2020-05-19 0:25 ` Andrii Nakryiko 2020-05-19 1:00 ` Yonghong Song 2020-05-19 15:00 ` Qian Cai 0 siblings, 2 replies; 12+ messages in thread From: Andrii Nakryiko @ 2020-05-19 0:25 UTC (permalink / raw) To: Qian Cai Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, Linux Netdev List, bpf, Linux Kernel Mailing List, clang-built-linux, Kees Cook 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: UBSAN: array-index-out-of-bounds in kernel/bpf/arraymap.c:177 2020-05-19 0:25 ` Andrii Nakryiko @ 2020-05-19 1:00 ` Yonghong Song 2020-05-19 1:30 ` Andrii Nakryiko 2020-05-19 15:00 ` Qian Cai 1 sibling, 1 reply; 12+ messages in thread From: Yonghong Song @ 2020-05-19 1:00 UTC (permalink / raw) To: Andrii Nakryiko, Qian Cai Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Andrii Nakryiko, John Fastabend, KP Singh, Linux Netdev List, bpf, Linux Kernel Mailing List, clang-built-linux, Kees Cook 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); }; ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: UBSAN: array-index-out-of-bounds in kernel/bpf/arraymap.c:177 2020-05-19 1:00 ` Yonghong Song @ 2020-05-19 1:30 ` Andrii Nakryiko 2020-05-19 1:36 ` Yonghong Song 0 siblings, 1 reply; 12+ messages in thread From: Andrii Nakryiko @ 2020-05-19 1:30 UTC (permalink / raw) To: Yonghong Song Cc: Qian Cai, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Andrii Nakryiko, John Fastabend, KP Singh, Linux Netdev List, bpf, Linux Kernel Mailing List, clang-built-linux, Kees Cook 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] > }; ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: UBSAN: array-index-out-of-bounds in kernel/bpf/arraymap.c:177 2020-05-19 1:30 ` Andrii Nakryiko @ 2020-05-19 1:36 ` Yonghong Song 0 siblings, 0 replies; 12+ messages in thread From: Yonghong Song @ 2020-05-19 1:36 UTC (permalink / raw) To: Andrii Nakryiko Cc: Qian Cai, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Andrii Nakryiko, John Fastabend, KP Singh, Linux Netdev List, bpf, Linux Kernel Mailing List, clang-built-linux, Kees Cook 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. > >> }; ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: UBSAN: array-index-out-of-bounds in kernel/bpf/arraymap.c:177 2020-05-19 0:25 ` Andrii Nakryiko 2020-05-19 1:00 ` Yonghong Song @ 2020-05-19 15:00 ` Qian Cai 2020-05-19 19:29 ` Andrii Nakryiko 1 sibling, 1 reply; 12+ messages in thread From: Qian Cai @ 2020-05-19 15:00 UTC (permalink / raw) To: Andrii Nakryiko Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, Linux Netdev List, bpf, Linux Kernel Mailing List, clang-built-linux, Kees Cook On Mon, May 18, 2020 at 8:25 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> 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. This works although it might makes sense to create a pair of ubsan_disable_current()/ubsan_enable_current() for it. diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c index 11584618e861..6415b089725e 100644 --- a/kernel/bpf/arraymap.c +++ b/kernel/bpf/arraymap.c @@ -170,11 +170,16 @@ static void *array_map_lookup_elem(struct bpf_map *map, void *key) { struct bpf_array *array = container_of(map, struct bpf_array, map); u32 index = *(u32 *)key; + void *elem; if (unlikely(index >= array->map.max_entries)) return NULL; - return array->value + array->elem_size * (index & array->index_mask); + current->in_ubsan++; + elem = array->value + array->elem_size * (index & array->index_mask); + current->in_ubsan--; + + return elem; } ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: UBSAN: array-index-out-of-bounds in kernel/bpf/arraymap.c:177 2020-05-19 15:00 ` Qian Cai @ 2020-05-19 19:29 ` Andrii Nakryiko 2020-05-19 20:18 ` Qian Cai 0 siblings, 1 reply; 12+ messages in thread From: Andrii Nakryiko @ 2020-05-19 19:29 UTC (permalink / raw) To: Qian Cai Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, Linux Netdev List, bpf, Linux Kernel Mailing List, clang-built-linux, Kees Cook On Tue, May 19, 2020 at 8:00 AM Qian Cai <cai@lca.pw> wrote: > > On Mon, May 18, 2020 at 8:25 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> 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. > > This works although it might makes sense to create a pair of > ubsan_disable_current()/ubsan_enable_current() for it. > > diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c > index 11584618e861..6415b089725e 100644 > --- a/kernel/bpf/arraymap.c > +++ b/kernel/bpf/arraymap.c > @@ -170,11 +170,16 @@ static void *array_map_lookup_elem(struct > bpf_map *map, void *key) > { > struct bpf_array *array = container_of(map, struct bpf_array, map); > u32 index = *(u32 *)key; > + void *elem; > > if (unlikely(index >= array->map.max_entries)) > return NULL; > > - return array->value + array->elem_size * (index & array->index_mask); > + current->in_ubsan++; > + elem = array->value + array->elem_size * (index & array->index_mask); > + current->in_ubsan--; This is an unnecessary performance hit for silencing what is clearly a false positive. I'm not sure that's the right solution here. It seems like something that's lacking on the tooling side instead. C language doesn't allow to express the intent here using flexible array approach. That doesn't mean that what we are doing here is wrong or undefined. > + > + return elem; > } ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: UBSAN: array-index-out-of-bounds in kernel/bpf/arraymap.c:177 2020-05-19 19:29 ` Andrii Nakryiko @ 2020-05-19 20:18 ` Qian Cai 2020-05-19 23:23 ` Andrii Nakryiko 0 siblings, 1 reply; 12+ messages in thread From: Qian Cai @ 2020-05-19 20:18 UTC (permalink / raw) To: Andrii Nakryiko Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, Linux Netdev List, bpf, Linux Kernel Mailing List, clang-built-linux, Kees Cook On Tue, May 19, 2020 at 3:30 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, May 19, 2020 at 8:00 AM Qian Cai <cai@lca.pw> wrote: > > > > On Mon, May 18, 2020 at 8:25 PM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> 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. > > > > This works although it might makes sense to create a pair of > > ubsan_disable_current()/ubsan_enable_current() for it. > > > > diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c > > index 11584618e861..6415b089725e 100644 > > --- a/kernel/bpf/arraymap.c > > +++ b/kernel/bpf/arraymap.c > > @@ -170,11 +170,16 @@ static void *array_map_lookup_elem(struct > > bpf_map *map, void *key) > > { > > struct bpf_array *array = container_of(map, struct bpf_array, map); > > u32 index = *(u32 *)key; > > + void *elem; > > > > if (unlikely(index >= array->map.max_entries)) > > return NULL; > > > > - return array->value + array->elem_size * (index & array->index_mask); > > + current->in_ubsan++; > > + elem = array->value + array->elem_size * (index & array->index_mask); > > + current->in_ubsan--; > > This is an unnecessary performance hit for silencing what is clearly a > false positive. I'm not sure that's the right solution here. It seems > like something that's lacking on the tooling side instead. C language > doesn't allow to express the intent here using flexible array > approach. That doesn't mean that what we are doing here is wrong or > undefined. Oh, so you worry about this ++ and -- hurt the performance? If so, how about this? ubsan_disable_current(); elem = array->value + array->elem_size * (index & array->index_mask); ubsan_enable_current(); #ifdef UBSAN ubsan_disable_current() { current->in_ubsan++; } #else ubsan_disable_current() {} #endif etc Production kernel would normally have UBSAN=n, so it is an noop. Leaving this false positive unsilenced may also waste many people's time over and over again, and increase the noisy level. Especially, it seems this is one-off (not seen other parts of kernel doing like this) and rather expensive to silence it in the UBSAN or/and compilers. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: UBSAN: array-index-out-of-bounds in kernel/bpf/arraymap.c:177 2020-05-19 20:18 ` Qian Cai @ 2020-05-19 23:23 ` Andrii Nakryiko 2020-05-20 1:55 ` Qian Cai 0 siblings, 1 reply; 12+ messages in thread From: Andrii Nakryiko @ 2020-05-19 23:23 UTC (permalink / raw) To: Qian Cai Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, Linux Netdev List, bpf, Linux Kernel Mailing List, clang-built-linux, Kees Cook On Tue, May 19, 2020 at 1:18 PM Qian Cai <cai@lca.pw> wrote: > > On Tue, May 19, 2020 at 3:30 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Tue, May 19, 2020 at 8:00 AM Qian Cai <cai@lca.pw> wrote: > > > > > > On Mon, May 18, 2020 at 8:25 PM Andrii Nakryiko > > > <andrii.nakryiko@gmail.com> 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. > > > > > > This works although it might makes sense to create a pair of > > > ubsan_disable_current()/ubsan_enable_current() for it. > > > > > > diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c > > > index 11584618e861..6415b089725e 100644 > > > --- a/kernel/bpf/arraymap.c > > > +++ b/kernel/bpf/arraymap.c > > > @@ -170,11 +170,16 @@ static void *array_map_lookup_elem(struct > > > bpf_map *map, void *key) > > > { > > > struct bpf_array *array = container_of(map, struct bpf_array, map); > > > u32 index = *(u32 *)key; > > > + void *elem; > > > > > > if (unlikely(index >= array->map.max_entries)) > > > return NULL; > > > > > > - return array->value + array->elem_size * (index & array->index_mask); > > > + current->in_ubsan++; > > > + elem = array->value + array->elem_size * (index & array->index_mask); > > > + current->in_ubsan--; > > > > This is an unnecessary performance hit for silencing what is clearly a > > false positive. I'm not sure that's the right solution here. It seems > > like something that's lacking on the tooling side instead. C language > > doesn't allow to express the intent here using flexible array > > approach. That doesn't mean that what we are doing here is wrong or > > undefined. > > Oh, so you worry about this ++ and -- hurt the performance? If so, how > about this? > > ubsan_disable_current(); > elem = array->value + array->elem_size * (index & array->index_mask); > ubsan_enable_current(); > > #ifdef UBSAN > ubsan_disable_current() > { > current->in_ubsan++; > } > #else > ubsan_disable_current() {} > #endif > > etc > > Production kernel would normally have UBSAN=n, so it is an noop. That would solve runtime performance hit, yes. > > Leaving this false positive unsilenced may also waste many people's > time over and over again, and increase the noisy level. Especially, it > seems this is one-off (not seen other parts of kernel doing like this) > and rather expensive to silence it in the UBSAN or/and compilers. I agree, it's bad to have this noise. But again, there is nothing wrong with the way it's used in BPF code base. We'd gladly use flexible array, if we could. But given we can't, I'd say the proper solution (in order of my preference) would be: - don't trigger false error, if zero-sized array is the member of union; - or have some sort of annotation at field declaration site (not a field access site). Is that possible? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: UBSAN: array-index-out-of-bounds in kernel/bpf/arraymap.c:177 2020-05-19 23:23 ` Andrii Nakryiko @ 2020-05-20 1:55 ` Qian Cai 0 siblings, 0 replies; 12+ messages in thread From: Qian Cai @ 2020-05-20 1:55 UTC (permalink / raw) To: Andrii Nakryiko Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, Linux Netdev List, bpf, Linux Kernel Mailing List, clang-built-linux, Kees Cook > On May 19, 2020, at 7:23 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > I agree, it's bad to have this noise. But again, there is nothing > wrong with the way it's used in BPF code base. We'd gladly use > flexible array, if we could. But given we can't, I'd say the proper > solution (in order of my preference) would be: > > - don't trigger false error, if zero-sized array is the member of union; > - or have some sort of annotation at field declaration site (not a > field access site). > > Is that possible? I am not a compiler expert, but with my experience with all those compiler instrumental technology like KCSAN, KASAN and UBSAN, it seems both options you prop need to modify compilers, i.e., -fsanitize=undefined ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-05-20 1:55 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-18 2:44 UBSAN: array-index-out-of-bounds in kernel/bpf/arraymap.c:177 Qian Cai 2020-05-18 23:55 ` Andrii Nakryiko 2020-05-19 0:09 ` Qian Cai 2020-05-19 0:25 ` Andrii Nakryiko 2020-05-19 1:00 ` Yonghong Song 2020-05-19 1:30 ` Andrii Nakryiko 2020-05-19 1:36 ` Yonghong Song 2020-05-19 15:00 ` Qian Cai 2020-05-19 19:29 ` Andrii Nakryiko 2020-05-19 20:18 ` Qian Cai 2020-05-19 23:23 ` Andrii Nakryiko 2020-05-20 1:55 ` Qian Cai
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).