| Message ID | 20210506212025.815380-1-pcc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kasan: fix unit tests with CONFIG_UBSAN_LOCAL_BOUNDS enabled | expand |
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!
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
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.
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
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.
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
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);
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(-)