[PATCH] kasan: fix unit tests with CONFIG_UBSAN_LOCAL_BOUNDS enabled

[PATCH] kasan: fix unit tests with CONFIG_UBSAN_LOCAL_BOUNDS enabled

[Peter Collingbourne]

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];
 
 	/* Only generic mode instruments globals. */
 	KASAN_TEST_NEEDS_CONFIG_ON(test, CONFIG_KASAN_GENERIC);
@@ -703,8 +703,8 @@ static void ksize_uaf(struct kunit *test)
 static void kasan_stack_oob(struct kunit *test)
 {
 	char stack_array[10];
-	volatile int i = OOB_TAG_OFF;
-	char *p = &stack_array[ARRAY_SIZE(stack_array) + i];
+	char *volatile array = stack_array;
+	char *p = &array[ARRAY_SIZE(stack_array) + OOB_TAG_OFF];
 
 	KASAN_TEST_NEEDS_CONFIG_ON(test, CONFIG_KASAN_STACK);
 
@@ -715,7 +715,8 @@ static void kasan_alloca_oob_left(struct kunit *test)
 {
 	volatile int i = 10;
 	char alloca_array[i];
-	char *p = alloca_array - 1;
+	char *volatile array = alloca_array;
+	char *p = array - 1;
 
 	/* Only generic mode instruments dynamic allocas. */
 	KASAN_TEST_NEEDS_CONFIG_ON(test, CONFIG_KASAN_GENERIC);
@@ -728,7 +729,8 @@ static void kasan_alloca_oob_right(struct kunit *test)
 {
 	volatile int i = 10;
 	char alloca_array[i];
-	char *p = alloca_array + i;
+	char *volatile array = alloca_array;
+	char *p = array + i;
 
 	/* Only generic mode instruments dynamic allocas. */
 	KASAN_TEST_NEEDS_CONFIG_ON(test, CONFIG_KASAN_GENERIC);
-- 
2.31.1.607.g51e8a6a459-goog

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

[Andrey Konovalov]

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.

>
>         /* Only generic mode instruments globals. */
>         KASAN_TEST_NEEDS_CONFIG_ON(test, CONFIG_KASAN_GENERIC);
> @@ -703,8 +703,8 @@ static void ksize_uaf(struct kunit *test)
>  static void kasan_stack_oob(struct kunit *test)
>  {
>         char stack_array[10];
> -       volatile int i = OOB_TAG_OFF;
> -       char *p = &stack_array[ARRAY_SIZE(stack_array) + i];
> +       char *volatile array = stack_array;
> +       char *p = &array[ARRAY_SIZE(stack_array) + OOB_TAG_OFF];
>
>         KASAN_TEST_NEEDS_CONFIG_ON(test, CONFIG_KASAN_STACK);
>
> @@ -715,7 +715,8 @@ static void kasan_alloca_oob_left(struct kunit *test)
>  {
>         volatile int i = 10;
>         char alloca_array[i];
> -       char *p = alloca_array - 1;
> +       char *volatile array = alloca_array;
> +       char *p = array - 1;
>
>         /* Only generic mode instruments dynamic allocas. */
>         KASAN_TEST_NEEDS_CONFIG_ON(test, CONFIG_KASAN_GENERIC);
> @@ -728,7 +729,8 @@ static void kasan_alloca_oob_right(struct kunit *test)
>  {
>         volatile int i = 10;
>         char alloca_array[i];
> -       char *p = alloca_array + i;
> +       char *volatile array = alloca_array;
> +       char *p = array + i;
>
>         /* Only generic mode instruments dynamic allocas. */
>         KASAN_TEST_NEEDS_CONFIG_ON(test, CONFIG_KASAN_GENERIC);
> --
> 2.31.1.607.g51e8a6a459-goog
>

Acked-by: Andrey Konovalov <andreyknvl@gmail.com>

Thanks, Peter!

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

[Peter Collingbourne]

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.

Peter

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

[Andrey Konovalov]

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.

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

[Peter Collingbourne]

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

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

[Andrew Morton]

On Thu,  6 May 2021 14:20:25 -0700 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.

Huh.  Is this use of volatile the official way of suppressing the
generation of the checking code or is it just something which happened
to work?  I'm wondering if this workaround should be formalized in some
fashion (presumably a wrapper) rather than mysteriously and
unexplainedly open-coding it like this.

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

[Peter Collingbourne]

On Sat, May 8, 2021 at 5:30 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Thu,  6 May 2021 14:20:25 -0700 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.
>
> Huh.  Is this use of volatile the official way of suppressing the
> generation of the checking code or is it just something which happened
> to work?  I'm wondering if this workaround should be formalized in some
> fashion (presumably a wrapper) rather than mysteriously and
> unexplainedly open-coding it like this.

I would consider it the official way in the sense that the compiler
must assume that the pointer that it loads from the address of "array"
has an arbitrary value due to the volatile qualifier, and the array
bounds stuff follows from that. Actually I don't think the compiler is
powerful enough yet to look through the store and load of "array", but
if it were, I think that would be the right way to suppress the
analysis.

Is the comment that I added in v2 not enough here?

Peter