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@xxxxxx> wrote:
>
> On Tue, May 19, 2020 at 3:30 PM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Tue, May 19, 2020 at 8:00 AM Qian Cai <cai@xxxxxx> wrote:
> > >
> > > On Mon, May 18, 2020 at 8:25 PM Andrii Nakryiko
> > > <andrii.nakryiko@xxxxxxxxx> wrote:
> > > >
> > > > On Mon, May 18, 2020 at 5:09 PM Qian Cai <cai@xxxxxx> wrote:
> > > > >
> > > > > On Mon, May 18, 2020 at 7:55 PM Andrii Nakryiko
> > > > > <andrii.nakryiko@xxxxxxxxx> wrote:
> > > > > >
> > > > > > On Sun, May 17, 2020 at 7:45 PM Qian Cai <cai@xxxxxx> 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?