| Message ID | 20211203093000.3714620-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | lib/test_ubsan: Silence compile-time array bounds warnings | expand |
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
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.
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
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.
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
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(+)