[PATCH] lib/test_ubsan: Silence compile-time array bounds warnings

[PATCH] lib/test_ubsan: Silence compile-time array bounds warnings

[Kees Cook]

The UBSAN tests intentionally operate beyond array bounds, so silence
the warning visible with a -Warray-bounds build:

lib/test_ubsan.c: In function 'test_ubsan_object_size_mismatch':
lib/test_ubsan.c:109:16: error: array subscript 'long long int[0]' is partly outside array bounds of 'volatile int[1]' [-Werror=array-bounds]
  109 |         val2 = *ptr;
      |                ^~~~
lib/test_ubsan.c:104:22: note: while referencing 'val'
  104 |         volatile int val __aligned(8) = 4;
      |                      ^~~

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 lib/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/Makefile b/lib/Makefile
index 08959b10bac9..2742a54a4275 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -70,6 +70,7 @@ obj-$(CONFIG_KASAN_MODULE_TEST) += test_kasan_module.o
 CFLAGS_test_kasan_module.o += -fno-builtin
 obj-$(CONFIG_TEST_UBSAN) += test_ubsan.o
 CFLAGS_test_ubsan.o += $(call cc-disable-warning, vla)
+CFLAGS_test_ubsan.o += $(call cc-disable-warning, array-bounds)
 UBSAN_SANITIZE_test_ubsan.o := y
 obj-$(CONFIG_TEST_KSTRTOX) += test-kstrtox.o
 obj-$(CONFIG_TEST_LIST_SORT) += test_list_sort.o
-- 
2.30.2

Re: [PATCH] lib/test_ubsan: Silence compile-time array bounds warnings

[Marco Elver]

On Fri, 3 Dec 2021 at 10:30, Kees Cook <keescook@chromium.org> wrote:
> The UBSAN tests intentionally operate beyond array bounds, so silence
> the warning visible with a -Warray-bounds build:
>
> lib/test_ubsan.c: In function 'test_ubsan_object_size_mismatch':
> lib/test_ubsan.c:109:16: error: array subscript 'long long int[0]' is partly outside array bounds of 'volatile int[1]' [-Werror=array-bounds]
>   109 |         val2 = *ptr;
>       |                ^~~~
> lib/test_ubsan.c:104:22: note: while referencing 'val'
>   104 |         volatile int val __aligned(8) = 4;
>       |                      ^~~
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  lib/Makefile | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/lib/Makefile b/lib/Makefile
> index 08959b10bac9..2742a54a4275 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -70,6 +70,7 @@ obj-$(CONFIG_KASAN_MODULE_TEST) += test_kasan_module.o
>  CFLAGS_test_kasan_module.o += -fno-builtin
>  obj-$(CONFIG_TEST_UBSAN) += test_ubsan.o
>  CFLAGS_test_ubsan.o += $(call cc-disable-warning, vla)
> +CFLAGS_test_ubsan.o += $(call cc-disable-warning, array-bounds)
>  UBSAN_SANITIZE_test_ubsan.o := y
>  obj-$(CONFIG_TEST_KSTRTOX) += test-kstrtox.o
>  obj-$(CONFIG_TEST_LIST_SORT) += test_list_sort.o

Are there other warnings or only the one for the fsanitize=object-size
test? I think this is fine if there are other warnings.

But, if it's only about the fsanitize=object-size test, I'm going to
propose something more drastic. :-)

I had wanted to wait a bit and dig a little deeper, but I just posted
part of my analysis here:
https://bugzilla.kernel.org/show_bug.cgi?id=214861#c4

My proposal is to remove UBSAN_OBJECT_SIZE and its related tests. The
bugzilla bug goes into the details, but the TLDR is:
1. fsanitize=object-size is incomplete,
2. it should have been a compiler warning,
3. for everything else there is KASAN which detects real OOB,
4. for GCC we already disable UBSAN_OBJECT_SIZE.

Thanks,
-- Marco

Re: [PATCH] lib/test_ubsan: Silence compile-time array bounds warnings

[Kees Cook]

On December 3, 2021 2:49:53 AM PST, Marco Elver <elver@google.com> wrote:
>On Fri, 3 Dec 2021 at 10:30, Kees Cook <keescook@chromium.org> wrote:
>> The UBSAN tests intentionally operate beyond array bounds, so silence
>> the warning visible with a -Warray-bounds build:
>>
>> lib/test_ubsan.c: In function 'test_ubsan_object_size_mismatch':
>> lib/test_ubsan.c:109:16: error: array subscript 'long long int[0]' is partly outside array bounds of 'volatile int[1]' [-Werror=array-bounds]
>>   109 |         val2 = *ptr;
>>       |                ^~~~
>> lib/test_ubsan.c:104:22: note: while referencing 'val'
>>   104 |         volatile int val __aligned(8) = 4;
>>       |                      ^~~
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>>  lib/Makefile | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/lib/Makefile b/lib/Makefile
>> index 08959b10bac9..2742a54a4275 100644
>> --- a/lib/Makefile
>> +++ b/lib/Makefile
>> @@ -70,6 +70,7 @@ obj-$(CONFIG_KASAN_MODULE_TEST) += test_kasan_module.o
>>  CFLAGS_test_kasan_module.o += -fno-builtin
>>  obj-$(CONFIG_TEST_UBSAN) += test_ubsan.o
>>  CFLAGS_test_ubsan.o += $(call cc-disable-warning, vla)
>> +CFLAGS_test_ubsan.o += $(call cc-disable-warning, array-bounds)
>>  UBSAN_SANITIZE_test_ubsan.o := y
>>  obj-$(CONFIG_TEST_KSTRTOX) += test-kstrtox.o
>>  obj-$(CONFIG_TEST_LIST_SORT) += test_list_sort.o
>
>Are there other warnings or only the one for the fsanitize=object-size
>test? I think this is fine if there are other warnings.

I will double check, but I think it's only the object-size test, which seems to confirm my suspicion that -Warray-bounds provides sufficient coverage and object-size can be removed.

I have another patch I intend to send today for the sk_buff/sk_buff_head issue, as -Warray-bounds warns for that as well.

>But, if it's only about the fsanitize=object-size test, I'm going to
>propose something more drastic. :-)

Are there any cases where object-size does a run-time check that couldn't be done at compile time? That's the only reason I could see to keep it at this point, as -Warray-bounds can do the compile time checks.

>I had wanted to wait a bit and dig a little deeper, but I just posted
>part of my analysis here:
>https://bugzilla.kernel.org/show_bug.cgi?id=214861#c4

Thanks, I'll refer to that in my sk_buff patch. It seems -Warray-bounds suffers from the same conservativism about object casts, which is frustrating on the one hand since the warning can be a false positive (cast vs access), but on the other, it does call attention to fragile arrangements which maybe could do with adjustment.

>My proposal is to remove UBSAN_OBJECT_SIZE and its related tests. The
>bugzilla bug goes into the details, but the TLDR is:
>1. fsanitize=object-size is incomplete,
>2. it should have been a compiler warning,
>3. for everything else there is KASAN which detects real OOB,
>4. for GCC we already disable UBSAN_OBJECT_SIZE.

And maybe:
5. -Warray-bounds provides the same coverage and is about to be enabled globally.



-- 
Kees Cook

Re: [PATCH] lib/test_ubsan: Silence compile-time array bounds warnings

[Marco Elver]

On Fri, 3 Dec 2021 at 17:21, Kees Cook <keescook@chromium.org> wrote:
> On December 3, 2021 2:49:53 AM PST, Marco Elver <elver@google.com> wrote:
[...]
> >Are there other warnings or only the one for the fsanitize=object-size
> >test? I think this is fine if there are other warnings.
>
> I will double check, but I think it's only the object-size test, which seems to confirm my suspicion that -Warray-bounds provides sufficient coverage and object-size can be removed.
>
> I have another patch I intend to send today for the sk_buff/sk_buff_head issue, as -Warray-bounds warns for that as well.

Nice.

Do you want to send the patch removing UBSAN_OBJECT_SIZE, or shall I
do it? Perhaps it ties in better with the rest of your patches which I
have no state of.

> >But, if it's only about the fsanitize=object-size test, I'm going to
> >propose something more drastic. :-)
>
> Are there any cases where object-size does a run-time check that couldn't be done at compile time? That's the only reason I could see to keep it at this point, as -Warray-bounds can do the compile time checks.

No, I don't think so.

I stared at the LLVM code several times now, because I still couldn't
quite believe it myself, but I think it really doesn't do any dynamic
checks. Hence, why below I say it should have been a compiler warning.
As mentioned in the bugzilla bug, there's a FIXME in the LLVM code to
do a dynamic check with the help of fsanitize=address, but that never
happened. And that doesn't make much sense anyway if fsanitize=address
(viz. KASAN for us) is already on and does checking itself.

> >I had wanted to wait a bit and dig a little deeper, but I just posted
> >part of my analysis here:
> >https://bugzilla.kernel.org/show_bug.cgi?id=214861#c4
>
> Thanks, I'll refer to that in my sk_buff patch. It seems -Warray-bounds suffers from the same conservativism about object casts, which is frustrating on the one hand since the warning can be a false positive (cast vs access), but on the other, it does call attention to fragile arrangements which maybe could do with adjustment.
>
> >My proposal is to remove UBSAN_OBJECT_SIZE and its related tests. The
> >bugzilla bug goes into the details, but the TLDR is:
> >1. fsanitize=object-size is incomplete,
> >2. it should have been a compiler warning,
> >3. for everything else there is KASAN which detects real OOB,
> >4. for GCC we already disable UBSAN_OBJECT_SIZE.
>
> And maybe:
> 5. -Warray-bounds provides the same coverage and is about to be enabled globally.

Yup, in which case the compiler warning already exists and point #2
above is moot.

Thanks,
-- Marco

Re: [PATCH] lib/test_ubsan: Silence compile-time array bounds warnings

[Kees Cook]

On Fri, Dec 03, 2021 at 05:53:15PM +0100, Marco Elver wrote:
> On Fri, 3 Dec 2021 at 17:21, Kees Cook <keescook@chromium.org> wrote:
> > On December 3, 2021 2:49:53 AM PST, Marco Elver <elver@google.com> wrote:
> [...]
> > >Are there other warnings or only the one for the fsanitize=object-size
> > >test? I think this is fine if there are other warnings.
> >
> > I will double check, but I think it's only the object-size test, which seems to confirm my suspicion that -Warray-bounds provides sufficient coverage and object-size can be removed.
> >
> > I have another patch I intend to send today for the sk_buff/sk_buff_head issue, as -Warray-bounds warns for that as well.
> 
> Nice.
> 
> Do you want to send the patch removing UBSAN_OBJECT_SIZE, or shall I
> do it? Perhaps it ties in better with the rest of your patches which I
> have no state of.

Sure; I'll tear it out. :) Thanks for doing the deep inspection on what
it is actually doing! That had been my main open question while digging
through all the -Warray-bounds warnings.

-- 
Kees Cook