UBSAN: array-index-out-of-bounds in kernel/bpf/arraymap.c:177

UBSAN: array-index-out-of-bounds in kernel/bpf/arraymap.c:177

[Qian Cai]

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]

Re: UBSAN: array-index-out-of-bounds in kernel/bpf/arraymap.c:177

[Andrii Nakryiko]

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?

Re: UBSAN: array-index-out-of-bounds in kernel/bpf/arraymap.c:177

[Qian Cai]

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.

Re: UBSAN: array-index-out-of-bounds in kernel/bpf/arraymap.c:177

[Andrii Nakryiko]

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.

Re: UBSAN: array-index-out-of-bounds in kernel/bpf/arraymap.c:177

[Yonghong Song]

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);
   };

Re: UBSAN: array-index-out-of-bounds in kernel/bpf/arraymap.c:177

[Andrii Nakryiko]

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]

>    };

Re: UBSAN: array-index-out-of-bounds in kernel/bpf/arraymap.c:177

[Yonghong Song]

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.

> 
>>     };

Re: UBSAN: array-index-out-of-bounds in kernel/bpf/arraymap.c:177

[Qian Cai]

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;
 }

Re: UBSAN: array-index-out-of-bounds in kernel/bpf/arraymap.c:177

[Andrii Nakryiko]

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;
>  }

Re: UBSAN: array-index-out-of-bounds in kernel/bpf/arraymap.c:177

[Qian Cai]

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.

Re: UBSAN: array-index-out-of-bounds in kernel/bpf/arraymap.c:177

[Andrii Nakryiko]

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?

Re: UBSAN: array-index-out-of-bounds in kernel/bpf/arraymap.c:177

[Qian Cai]

> 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