Re: [PATCH] kasan: fix unit tests with CONFIG_UBSAN_LOCAL_BOUNDS enabled

[Peter Collingbourne <pcc@google.com>]

On Thu, May 6, 2021 at 4:58 PM Andrey Konovalov <andreyknvl@gmail.com> wrote:
>
> On Fri, May 7, 2021 at 1:47 AM Peter Collingbourne <pcc@google.com> wrote:
> >
> > On Thu, May 6, 2021 at 3:12 PM Andrey Konovalov <andreyknvl@gmail.com> wrote:
> > >
> > > On Thu, May 6, 2021 at 11:20 PM Peter Collingbourne <pcc@google.com> wrote:
> > > >
> > > > These tests deliberately access these arrays out of bounds,
> > > > which will cause the dynamic local bounds checks inserted by
> > > > CONFIG_UBSAN_LOCAL_BOUNDS to fail and panic the kernel. To avoid this
> > > > problem, access the arrays via volatile pointers, which will prevent
> > > > the compiler from being able to determine the array bounds.
> > > >
> > > > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > > > Cc: stable@vger.kernel.org
> > > > Link: https://linux-review.googlesource.com/id/I90b1713fbfa1bf68ff895aef099ea77b98a7c3b9
> > > > ---
> > > >  lib/test_kasan.c | 14 ++++++++------
> > > >  1 file changed, 8 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> > > > index dc05cfc2d12f..2a078e8e7b8e 100644
> > > > --- a/lib/test_kasan.c
> > > > +++ b/lib/test_kasan.c
> > > > @@ -654,8 +654,8 @@ static char global_array[10];
> > > >
> > > >  static void kasan_global_oob(struct kunit *test)
> > > >  {
> > > > -       volatile int i = 3;
> > > > -       char *p = &global_array[ARRAY_SIZE(global_array) + i];
> > > > +       char *volatile array = global_array;
> > > > +       char *p = &array[ARRAY_SIZE(global_array) + 3];
> > >
> > > Nit: in the kernel, "volatile" usually comes before the pointer type.
> >
> > That would refer to a different type. "volatile char *" is a pointer
> > to volatile char, while "char *volatile" is a volatile pointer to
> > char. The latter is what we want here, because we want to prevent the
> > compiler from inferring things about the pointer itself (i.e. its
> > array bounds), not the data that it refers to.
>
> I see. This is unusual. I'd say this needs to be explicitly explained
> in the commit message, as well as in a comment in the code.

Done in v2.

Peter