All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] kasan: fix unit tests with CONFIG_UBSAN_LOCAL_BOUNDS enabled
@ 2021-05-06 21:20 ` Peter Collingbourne
  0 siblings, 0 replies; 13+ messages in thread
From: Peter Collingbourne @ 2021-05-06 21:20 UTC (permalink / raw)
  To: Andrey Konovalov, Alexander Potapenko
  Cc: Peter Collingbourne, George Popescu, Elena Petrova,
	Evgenii Stepanov, Andrew Morton, linux-mm, stable

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH] kasan: fix unit tests with CONFIG_UBSAN_LOCAL_BOUNDS enabled
@ 2021-05-06 21:20 ` Peter Collingbourne
  0 siblings, 0 replies; 13+ messages in thread
From: Peter Collingbourne @ 2021-05-06 21:20 UTC (permalink / raw)
  To: Andrey Konovalov, Alexander Potapenko
  Cc: Peter Collingbourne, George Popescu, Elena Petrova,
	Evgenii Stepanov, Andrew Morton, linux-mm, stable

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



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] kasan: fix unit tests with CONFIG_UBSAN_LOCAL_BOUNDS enabled
  2021-05-06 21:20 ` Peter Collingbourne
@ 2021-05-06 22:12   ` Andrey Konovalov
  -1 siblings, 0 replies; 13+ messages in thread
From: Andrey Konovalov @ 2021-05-06 22:12 UTC (permalink / raw)
  To: Peter Collingbourne
  Cc: Alexander Potapenko, George Popescu, Elena Petrova,
	Evgenii Stepanov, Andrew Morton, Linux Memory Management List,
	stable

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!

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] kasan: fix unit tests with CONFIG_UBSAN_LOCAL_BOUNDS enabled
@ 2021-05-06 22:12   ` Andrey Konovalov
  0 siblings, 0 replies; 13+ messages in thread
From: Andrey Konovalov @ 2021-05-06 22:12 UTC (permalink / raw)
  To: Peter Collingbourne
  Cc: Alexander Potapenko, George Popescu, Elena Petrova,
	Evgenii Stepanov, Andrew Morton, Linux Memory Management List,
	stable

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!


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] kasan: fix unit tests with CONFIG_UBSAN_LOCAL_BOUNDS enabled
  2021-05-06 22:12   ` Andrey Konovalov
@ 2021-05-06 23:47     ` Peter Collingbourne
  -1 siblings, 0 replies; 13+ messages in thread
From: Peter Collingbourne @ 2021-05-06 23:47 UTC (permalink / raw)
  To: Andrey Konovalov
  Cc: Alexander Potapenko, George Popescu, Elena Petrova,
	Evgenii Stepanov, Andrew Morton, Linux Memory Management List,
	stable

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] kasan: fix unit tests with CONFIG_UBSAN_LOCAL_BOUNDS enabled
@ 2021-05-06 23:47     ` Peter Collingbourne
  0 siblings, 0 replies; 13+ messages in thread
From: Peter Collingbourne @ 2021-05-06 23:47 UTC (permalink / raw)
  To: Andrey Konovalov
  Cc: Alexander Potapenko, George Popescu, Elena Petrova,
	Evgenii Stepanov, Andrew Morton, Linux Memory Management List,
	stable

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] kasan: fix unit tests with CONFIG_UBSAN_LOCAL_BOUNDS enabled
  2021-05-06 23:47     ` Peter Collingbourne
@ 2021-05-06 23:57       ` Andrey Konovalov
  -1 siblings, 0 replies; 13+ messages in thread
From: Andrey Konovalov @ 2021-05-06 23:57 UTC (permalink / raw)
  To: Peter Collingbourne
  Cc: Alexander Potapenko, George Popescu, Elena Petrova,
	Evgenii Stepanov, Andrew Morton, Linux Memory Management List,
	stable

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] kasan: fix unit tests with CONFIG_UBSAN_LOCAL_BOUNDS enabled
@ 2021-05-06 23:57       ` Andrey Konovalov
  0 siblings, 0 replies; 13+ messages in thread
From: Andrey Konovalov @ 2021-05-06 23:57 UTC (permalink / raw)
  To: Peter Collingbourne
  Cc: Alexander Potapenko, George Popescu, Elena Petrova,
	Evgenii Stepanov, Andrew Morton, Linux Memory Management List,
	stable

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.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] kasan: fix unit tests with CONFIG_UBSAN_LOCAL_BOUNDS enabled
  2021-05-06 23:57       ` Andrey Konovalov
@ 2021-05-07  2:59         ` Peter Collingbourne
  -1 siblings, 0 replies; 13+ messages in thread
From: Peter Collingbourne @ 2021-05-07  2:59 UTC (permalink / raw)
  To: Andrey Konovalov
  Cc: Alexander Potapenko, George Popescu, Elena Petrova,
	Evgenii Stepanov, Andrew Morton, Linux Memory Management List,
	stable

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] kasan: fix unit tests with CONFIG_UBSAN_LOCAL_BOUNDS enabled
@ 2021-05-07  2:59         ` Peter Collingbourne
  0 siblings, 0 replies; 13+ messages in thread
From: Peter Collingbourne @ 2021-05-07  2:59 UTC (permalink / raw)
  To: Andrey Konovalov
  Cc: Alexander Potapenko, George Popescu, Elena Petrova,
	Evgenii Stepanov, Andrew Morton, Linux Memory Management List,
	stable

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] kasan: fix unit tests with CONFIG_UBSAN_LOCAL_BOUNDS enabled
  2021-05-06 21:20 ` Peter Collingbourne
  (?)
  (?)
@ 2021-05-09  0:30 ` Andrew Morton
  2021-05-10 17:16     ` Peter Collingbourne
  -1 siblings, 1 reply; 13+ messages in thread
From: Andrew Morton @ 2021-05-09  0:30 UTC (permalink / raw)
  To: Peter Collingbourne
  Cc: Andrey Konovalov, Alexander Potapenko, George Popescu,
	Elena Petrova, Evgenii Stepanov, linux-mm, stable

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.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] kasan: fix unit tests with CONFIG_UBSAN_LOCAL_BOUNDS enabled
  2021-05-09  0:30 ` Andrew Morton
@ 2021-05-10 17:16     ` Peter Collingbourne
  0 siblings, 0 replies; 13+ messages in thread
From: Peter Collingbourne @ 2021-05-10 17:16 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Andrey Konovalov, Alexander Potapenko, George Popescu,
	Elena Petrova, Evgenii Stepanov, Linux Memory Management List,
	stable

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] kasan: fix unit tests with CONFIG_UBSAN_LOCAL_BOUNDS enabled
@ 2021-05-10 17:16     ` Peter Collingbourne
  0 siblings, 0 replies; 13+ messages in thread
From: Peter Collingbourne @ 2021-05-10 17:16 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Andrey Konovalov, Alexander Potapenko, George Popescu,
	Elena Petrova, Evgenii Stepanov, Linux Memory Management List,
	stable

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-05-10 17:16 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-06 21:20 [PATCH] kasan: fix unit tests with CONFIG_UBSAN_LOCAL_BOUNDS enabled Peter Collingbourne
2021-05-06 21:20 ` Peter Collingbourne
2021-05-06 22:12 ` Andrey Konovalov
2021-05-06 22:12   ` Andrey Konovalov
2021-05-06 23:47   ` Peter Collingbourne
2021-05-06 23:47     ` Peter Collingbourne
2021-05-06 23:57     ` Andrey Konovalov
2021-05-06 23:57       ` Andrey Konovalov
2021-05-07  2:59       ` Peter Collingbourne
2021-05-07  2:59         ` Peter Collingbourne
2021-05-09  0:30 ` Andrew Morton
2021-05-10 17:16   ` Peter Collingbourne
2021-05-10 17:16     ` Peter Collingbourne

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.