kunit/fortify: Replace "volatile" with OPTIMIZER_HIDE_VAR()
Kees Cook
maintainerPATCH 1/1 kunit/fortify: Replace "volatile" with OPTIMIZER_HIDE_VAR()
It does seem that using "volatile" isn't going to be sane compared to using OPTIMIZER_HIDE_VAR() going forward. Some strange interactions[1] with the sanitizers have been observed in the self-test code, so replace the logic.
---
Cc: Nathan Chancellor <[email protected]>
Cc: [email protected]
--- lib/tests/fortify_kunit.c | 139 +++++++++++++++++++++----------------- 1 file changed, 77 insertions(+), 62 deletions(-)
lib/tests/fortify_kunit.c +77 -62
@@ -411,8 +411,6 @@ struct fortify_padding {char buf[32 ];unsigned long bytes_after; }; -/* Force compiler into not being able to resolve size at compile-time. */ -static volatile int unconst;static void fortify_test_strlen (struct kunit *test) {
@@ -537,57 +535,56 @@ static void fortify_test_strncpy(struct kunit *test) {struct fortify_padding pad = { };char src[] ="Copy me fully into a small buffer and I will overflow!" ; +size_t sizeof_buf =sizeof (pad.buf); + +OPTIMIZER_HIDE_VAR (sizeof_buf);/* Destination is %NUL-filled to start with. */ KUNIT_EXPECT_EQ (test, pad.bytes_before,0 ); -KUNIT_EXPECT_EQ (test, pad.buf[sizeof (pad.buf) -1 ],'\0' ); -KUNIT_EXPECT_EQ (test, pad.buf[sizeof (pad.buf) -2 ],'\0' ); -KUNIT_EXPECT_EQ (test, pad.buf[sizeof (pad.buf) -3 ],'\0' ); +KUNIT_EXPECT_EQ (test, pad.buf[sizeof_buf -1 ],'\0' ); +KUNIT_EXPECT_EQ (test, pad.buf[sizeof_buf -2 ],'\0' ); +KUNIT_EXPECT_EQ (test, pad.buf[sizeof_buf -3 ],'\0' );KUNIT_EXPECT_EQ (test, pad.bytes_after,0 );/* Legitimate strncpy() 1 less than of max size. */ -KUNIT_ASSERT_TRUE (test,strncpy (pad.buf, src, -sizeof (pad.buf) + unconst -1 ) +KUNIT_ASSERT_TRUE (test,strncpy (pad.buf, src, sizeof_buf -1 ) == pad.buf);KUNIT_EXPECT_EQ (test, fortify_write_overflows,0 );/* Only last byte should be %NUL */ -KUNIT_EXPECT_EQ (test, pad.buf[sizeof (pad.buf) -1 ],'\0' ); -KUNIT_EXPECT_NE (test, pad.buf[sizeof (pad.buf) -2 ],'\0' ); -KUNIT_EXPECT_NE (test, pad.buf[sizeof (pad.buf) -3 ],'\0' ); +KUNIT_EXPECT_EQ (test, pad.buf[sizeof_buf -1 ],'\0' ); +KUNIT_EXPECT_NE (test, pad.buf[sizeof_buf -2 ],'\0' ); +KUNIT_EXPECT_NE (test, pad.buf[sizeof_buf -3 ],'\0' );/* Legitimate (though unterminated) max-size strncpy. */ -KUNIT_ASSERT_TRUE (test,strncpy (pad.buf, src, -sizeof (pad.buf) + unconst) +KUNIT_ASSERT_TRUE (test,strncpy (pad.buf, src, sizeof_buf) == pad.buf);KUNIT_EXPECT_EQ (test, fortify_write_overflows,0 );/* No trailing %NUL -- thanks strncpy API. */ -KUNIT_EXPECT_NE (test, pad.buf[sizeof (pad.buf) -1 ],'\0' ); -KUNIT_EXPECT_NE (test, pad.buf[sizeof (pad.buf) -2 ],'\0' ); -KUNIT_EXPECT_NE (test, pad.buf[sizeof (pad.buf) -2 ],'\0' ); +KUNIT_EXPECT_NE (test, pad.buf[sizeof_buf -1 ],'\0' ); +KUNIT_EXPECT_NE (test, pad.buf[sizeof_buf -2 ],'\0' ); +KUNIT_EXPECT_NE (test, pad.buf[sizeof_buf -2 ],'\0' );/* But we will not have gone beyond. */ KUNIT_EXPECT_EQ (test, pad.bytes_after,0 );/* Now verify that FORTIFY is working... */ -KUNIT_ASSERT_TRUE (test,strncpy (pad.buf, src, -sizeof (pad.buf) + unconst +1 ) +KUNIT_ASSERT_TRUE (test,strncpy (pad.buf, src, sizeof_buf +1 ) == pad.buf);/* Should catch the overflow. */ KUNIT_EXPECT_EQ (test, fortify_write_overflows,1 ); -KUNIT_EXPECT_NE (test, pad.buf[sizeof (pad.buf) -1 ],'\0' ); -KUNIT_EXPECT_NE (test, pad.buf[sizeof (pad.buf) -2 ],'\0' ); -KUNIT_EXPECT_NE (test, pad.buf[sizeof (pad.buf) -2 ],'\0' ); +KUNIT_EXPECT_NE (test, pad.buf[sizeof_buf -1 ],'\0' ); +KUNIT_EXPECT_NE (test, pad.buf[sizeof_buf -2 ],'\0' ); +KUNIT_EXPECT_NE (test, pad.buf[sizeof_buf -2 ],'\0' );/* And we will not have gone beyond. */ KUNIT_EXPECT_EQ (test, pad.bytes_after,0 );/* And further... */ -KUNIT_ASSERT_TRUE (test,strncpy (pad.buf, src, -sizeof (pad.buf) + unconst +2 ) +KUNIT_ASSERT_TRUE (test,strncpy (pad.buf, src, sizeof_buf +2 ) == pad.buf);/* Should catch the overflow. */ KUNIT_EXPECT_EQ (test, fortify_write_overflows,2 ); -KUNIT_EXPECT_NE (test, pad.buf[sizeof (pad.buf) -1 ],'\0' ); -KUNIT_EXPECT_NE (test, pad.buf[sizeof (pad.buf) -2 ],'\0' ); -KUNIT_EXPECT_NE (test, pad.buf[sizeof (pad.buf) -2 ],'\0' ); +KUNIT_EXPECT_NE (test, pad.buf[sizeof_buf -1 ],'\0' ); +KUNIT_EXPECT_NE (test, pad.buf[sizeof_buf -2 ],'\0' ); +KUNIT_EXPECT_NE (test, pad.buf[sizeof_buf -2 ],'\0' );/* And we will not have gone beyond. */ KUNIT_EXPECT_EQ (test, pad.bytes_after,0 ); }
@@ -596,55 +593,56 @@ static void fortify_test_strscpy(struct kunit *test) {struct fortify_padding pad = { };char src[] ="Copy me fully into a small buffer and I will overflow!" ; +size_t sizeof_buf =sizeof (pad.buf); +size_t sizeof_src =sizeof (src); + +OPTIMIZER_HIDE_VAR (sizeof_buf); +OPTIMIZER_HIDE_VAR (sizeof_src);/* Destination is %NUL-filled to start with. */ KUNIT_EXPECT_EQ (test, pad.bytes_before,0 ); -KUNIT_EXPECT_EQ (test, pad.buf[sizeof (pad.buf) -1 ],'\0' ); -KUNIT_EXPECT_EQ (test, pad.buf[sizeof (pad.buf) -2 ],'\0' ); -KUNIT_EXPECT_EQ (test, pad.buf[sizeof (pad.buf) -3 ],'\0' ); +KUNIT_EXPECT_EQ (test, pad.buf[sizeof_buf -1 ],'\0' ); +KUNIT_EXPECT_EQ (test, pad.buf[sizeof_buf -2 ],'\0' ); +KUNIT_EXPECT_EQ (test, pad.buf[sizeof_buf -3 ],'\0' );KUNIT_EXPECT_EQ (test, pad.bytes_after,0 );/* Legitimate strscpy() 1 less than of max size. */ -KUNIT_ASSERT_EQ (test,strscpy (pad.buf, src, -sizeof (pad.buf) + unconst -1 ), +KUNIT_ASSERT_EQ (test,strscpy (pad.buf, src, sizeof_buf -1 ), -E2BIG );KUNIT_EXPECT_EQ (test, fortify_write_overflows,0 );/* Keeping space for %NUL, last two bytes should be %NUL */ -KUNIT_EXPECT_EQ (test, pad.buf[sizeof (pad.buf) -1 ],'\0' ); -KUNIT_EXPECT_EQ (test, pad.buf[sizeof (pad.buf) -2 ],'\0' ); -KUNIT_EXPECT_NE (test, pad.buf[sizeof (pad.buf) -3 ],'\0' ); +KUNIT_EXPECT_EQ (test, pad.buf[sizeof_buf -1 ],'\0' ); +KUNIT_EXPECT_EQ (test, pad.buf[sizeof_buf -2 ],'\0' ); +KUNIT_EXPECT_NE (test, pad.buf[sizeof_buf -3 ],'\0' );/* Legitimate max-size strscpy. */ -KUNIT_ASSERT_EQ (test,strscpy (pad.buf, src, -sizeof (pad.buf) + unconst), +KUNIT_ASSERT_EQ (test,strscpy (pad.buf, src, sizeof_buf), -E2BIG );KUNIT_EXPECT_EQ (test, fortify_write_overflows,0 );/* A trailing %NUL will exist. */ -KUNIT_EXPECT_EQ (test, pad.buf[sizeof (pad.buf) -1 ],'\0' ); -KUNIT_EXPECT_NE (test, pad.buf[sizeof (pad.buf) -2 ],'\0' ); -KUNIT_EXPECT_NE (test, pad.buf[sizeof (pad.buf) -2 ],'\0' ); +KUNIT_EXPECT_EQ (test, pad.buf[sizeof_buf -1 ],'\0' ); +KUNIT_EXPECT_NE (test, pad.buf[sizeof_buf -2 ],'\0' ); +KUNIT_EXPECT_NE (test, pad.buf[sizeof_buf -2 ],'\0' );/* Now verify that FORTIFY is working... */ -KUNIT_ASSERT_EQ (test,strscpy (pad.buf, src, -sizeof (pad.buf) + unconst +1 ), +KUNIT_ASSERT_EQ (test,strscpy (pad.buf, src, sizeof_buf +1 ), -E2BIG );/* Should catch the overflow. */ KUNIT_EXPECT_EQ (test, fortify_write_overflows,1 ); -KUNIT_EXPECT_EQ (test, pad.buf[sizeof (pad.buf) -1 ],'\0' ); -KUNIT_EXPECT_NE (test, pad.buf[sizeof (pad.buf) -2 ],'\0' ); -KUNIT_EXPECT_NE (test, pad.buf[sizeof (pad.buf) -2 ],'\0' ); +KUNIT_EXPECT_EQ (test, pad.buf[sizeof_buf -1 ],'\0' ); +KUNIT_EXPECT_NE (test, pad.buf[sizeof_buf -2 ],'\0' ); +KUNIT_EXPECT_NE (test, pad.buf[sizeof_buf -2 ],'\0' );/* And we will not have gone beyond. */ KUNIT_EXPECT_EQ (test, pad.bytes_after,0 );/* And much further... */ -KUNIT_ASSERT_EQ (test,strscpy (pad.buf, src, -sizeof (src) *2 + unconst), +KUNIT_ASSERT_EQ (test,strscpy (pad.buf, src, sizeof_src *2 ), -E2BIG );/* Should catch the overflow. */ KUNIT_EXPECT_EQ (test, fortify_write_overflows,2 ); -KUNIT_EXPECT_EQ (test, pad.buf[sizeof (pad.buf) -1 ],'\0' ); -KUNIT_EXPECT_NE (test, pad.buf[sizeof (pad.buf) -2 ],'\0' ); -KUNIT_EXPECT_NE (test, pad.buf[sizeof (pad.buf) -2 ],'\0' ); +KUNIT_EXPECT_EQ (test, pad.buf[sizeof_buf -1 ],'\0' ); +KUNIT_EXPECT_NE (test, pad.buf[sizeof_buf -2 ],'\0' ); +KUNIT_EXPECT_NE (test, pad.buf[sizeof_buf -2 ],'\0' );/* And we will not have gone beyond. */ KUNIT_EXPECT_EQ (test, pad.bytes_after,0 ); }
@@ -784,7 +782,9 @@ static void fortify_test_strlcat(struct kunit *test)struct fortify_padding pad = { };char src[sizeof (pad.buf)] = { };int i, partial; -int len =sizeof (pad.buf) + unconst; +int len =sizeof (pad.buf); + +OPTIMIZER_HIDE_VAR (len);/* Fill 15 bytes with valid characters. */ partial =sizeof (src) /2 -1 ;
@@ -874,28 +874,32 @@ struct fortify_zero_sized {#define __fortify_test (memfunc) \ static void fortify_test_##memfunc(struct kunit *test) \ { \ - struct fortify_zero_sized zero = { }; \ + struct fortify_zero_sized empty = { }; \ struct fortify_padding pad = { }; \ char srcA[sizeof(pad.buf) + 2]; \ char srcB[sizeof(pad.buf) + 2]; \ - size_t len = sizeof(pad.buf) + unconst; \ + size_t len = sizeof(pad.buf); \ + size_t zero = 0; \ + \ + OPTIMIZER_HIDE_VAR(len); \ + OPTIMIZER_HIDE_VAR(zero); \ \ memset(srcA, 'A', sizeof(srcA)); \ KUNIT_ASSERT_EQ(test, srcA[0], 'A'); \ memset(srcB, 'B', sizeof(srcB)); \ KUNIT_ASSERT_EQ(test, srcB[0], 'B'); \ \ - memfunc(pad.buf, srcA, 0 + unconst); \ + memfunc(pad.buf, srcA, zero); \ KUNIT_EXPECT_EQ(test, pad.buf[0], '\0'); \ KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0); \ KUNIT_EXPECT_EQ(test, fortify_write_overflows, 0); \ - memfunc(pad.buf + 1, srcB, 1 + unconst); \ + memfunc(pad.buf + 1, srcB, zero + 1); \ KUNIT_EXPECT_EQ(test, pad.buf[0], '\0'); \ KUNIT_EXPECT_EQ(test, pad.buf[1], 'B'); \ KUNIT_EXPECT_EQ(test, pad.buf[2], '\0'); \ KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0); \ KUNIT_EXPECT_EQ(test, fortify_write_overflows, 0); \ - memfunc(pad.buf, srcA, 1 + unconst); \ + memfunc(pad.buf, srcA, zero + 1); \ KUNIT_EXPECT_EQ(test, pad.buf[0], 'A'); \ KUNIT_EXPECT_EQ(test, pad.buf[1], 'B'); \ KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0); \
@@ -921,10 +925,10 @@ static void fortify_test_##memfunc(struct kunit *test) \/* Reset error counter. */ \ fortify_write_overflows =0 ; \/* Copy nothing into nothing: no errors. */ \ -memfunc (zero.buf, srcB,0 + unconst); \ +memfunc (empty.buf, srcB, zero); \KUNIT_EXPECT_EQ (test, fortify_read_overflows,0 ); \KUNIT_EXPECT_EQ (test, fortify_write_overflows,0 ); \ -memfunc (zero.buf, srcB,1 + unconst); \ +memfunc (empty.buf, srcB, zero +1 ); \KUNIT_EXPECT_EQ (test, fortify_read_overflows,0 ); \KUNIT_EXPECT_EQ (test, fortify_write_overflows,1 ); \ }
@@ -936,7 +940,9 @@ static void fortify_test_memscan(struct kunit *test)char haystack[] ="Where oh where is my memory range?" ;char *mem = haystack +strlen ("Where oh where is " );char needle ='m' ; -size_t len =sizeof (haystack) + unconst; +size_t len =sizeof (haystack); + +OPTIMIZER_HIDE_VAR (len);KUNIT_ASSERT_PTR_EQ (test,memscan (haystack, needle, len), mem);
@@ -955,7 +961,9 @@ static void fortify_test_memchr(struct kunit *test)char haystack[] ="Where oh where is my memory range?" ;char *mem = haystack +strlen ("Where oh where is " );char needle ='m' ; -size_t len =sizeof (haystack) + unconst; +size_t len =sizeof (haystack); + +OPTIMIZER_HIDE_VAR (len);KUNIT_ASSERT_PTR_EQ (test,memchr (haystack, needle, len), mem);
@@ -974,7 +982,9 @@ static void fortify_test_memchr_inv(struct kunit *test)char haystack[] ="Where oh where is my memory range?" ;char *mem = haystack +1 ;char needle ='W' ; -size_t len =sizeof (haystack) + unconst; +size_t len =sizeof (haystack); + +OPTIMIZER_HIDE_VAR (len);/* Normal search is okay. */ KUNIT_ASSERT_PTR_EQ(test, memchr_inv(haystack, needle, len),
@@ -993,8 +1003,11 @@ static void fortify_test_memcmp(struct kunit *test) {char one[] ="My mind is going ..." ;char two[] ="My mind is going ... I can feel it." ; -size_t one_len =sizeof (one) + unconst -1 ; -size_t two_len =sizeof (two) + unconst -1 ; +size_t one_len =sizeof (one) -1 ; +size_t two_len =sizeof (two) -1 ; + +OPTIMIZER_HIDE_VAR (one_len); +OPTIMIZER_HIDE_VAR (two_len);/* We match the first string (ignoring the %NUL). */ KUNIT_ASSERT_EQ (test,memcmp (one, two, one_len),0 );
@@ -1015,7 +1028,9 @@ static void fortify_test_kmemdup(struct kunit *test) {char src[] ="I got Doom running on it!" ;char *copy; -size_t len =sizeof (src) + unconst; +size_t len =sizeof (src); + +OPTIMIZER_HIDE_VAR (len);/* Copy is within bounds. */ copy =kmemdup (src, len,GFP_KERNEL );
Signature · 1 line
2.34.1
Hi Kees,
Quoted 8 lines · Kees Cook
It does seem that using "volatile" isn't going to be sane compared to using
OPTIMIZER_HIDE_VAR()going forward. Some strange interactions[1] with the sanitizers have been observed in the self-test code, so replace the logic.Reported-by Nathan Chancellor <[email protected]>Signed-off-by Kees Cook <[email protected]>
...
Quoted 4 lines
lib/tests/fortify_kunit.c +0
-0
...
Quoted 4 lines
lib/tests/fortify_kunit.c +0
-0@@ -993,8 +1003,11 @@ static void fortify_test_memcmp(struct kunit *test) {char one[] ="My mind is going ..." ;char two[] ="My mind is going ... I can feel it." ;
lib/tests/fortify_kunit.c static void fortify_test_memcmp(struct kunit *test) · line 1013
Quoted 10 lines
lib/tests/fortify_kunit.c +5
-2-size_t one_len =sizeof (one) + unconst -1 ; -size_t two_len =sizeof (two) + unconst -1 ; +size_t one_len =sizeof (one) -1 ; +size_t two_len =sizeof (two) -1 ; + +OPTIMIZER_HIDE_VAR (one_len); +OPTIMIZER_HIDE_VAR (two_len);/* We match the first string (ignoring the %NUL). */ KUNIT_ASSERT_EQ (test,memcmp (one, two, one_len),0 );
I am sorry for bringing this up some time after you sent this change, as I have only now had a chance to actually sit down and understand the results of my bisect. I am still seeing a __read_overflow error when building lib/tests/fortify_kunit.o with Fedora's configuration + LTO in next-20250317, which contains this change. I do not think it is issue 2075, as I can reproduce it without UBSAN enabled altogether. This is with LLVM 20.1.0.
$ cat kernel/configs/repro.config CONFIG_FORTIFY_KUNIT_TEST=m CONFIG_FORTIFY_SOURCE=y CONFIG_KUNIT=y # CONFIG_LTO_NONE is not set CONFIG_LTO_CLANG_THIN=y
# or x86_64
$ make -skj"$(nproc)" ARCH=arm64 LLVM=1 mrproper {def,repro.}config lib/tests/fortify_kunit.o
ld.lld: error: call to __read_overflow marked "dontcall-error": detected read beyond size of object (1st parameter)
make[6]: *** [scripts/Makefile.build:203: lib/tests/fortify_kunit.o] Error 1Selectively reverting this avoids the problem, which is definitely odd... Maybe issue 2075 is related more to issue 2077 and this patch should not be entertained?
Cheers,
Nathan
lib/tests/fortify_kunit.c +4 -5
@@ -411,6 +411,8 @@ struct fortify_padding {char buf[32 ];unsigned long bytes_after; }; +/* Force compiler into not being able to resolve size at compile-time. */ +static volatile int unconst;static void fortify_test_strlen (struct kunit *test) {
@@ -1003,11 +1005,8 @@ static void fortify_test_memcmp(struct kunit *test) {char one[] ="My mind is going ..." ;char two[] ="My mind is going ... I can feel it." ; -size_t one_len =sizeof (one) -1 ; -size_t two_len =sizeof (two) -1 ; - -OPTIMIZER_HIDE_VAR (one_len); -OPTIMIZER_HIDE_VAR (two_len); +size_t one_len =sizeof (one) + unconst -1 ; +size_t two_len =sizeof (two) + unconst -1 ;/* We match the first string (ignoring the %NUL). */ KUNIT_ASSERT_EQ (test,memcmp (one, two, one_len),0 );
Kees Cook
maintainerQuoted 29 lines · Nathan Chancellor
Hi Kees,
Quoted 8 lines · Kees Cook
It does seem that using "volatile" isn't going to be sane compared to using
OPTIMIZER_HIDE_VAR()going forward. Some strange interactions[1] with the sanitizers have been observed in the self-test code, so replace the logic.Reported-by Nathan Chancellor <[email protected]>Signed-off-by Kees Cook <[email protected]>...
Quoted 4 lines
lib/tests/fortify_kunit.c +0
-0...
Quoted 12 lines
lib/tests/fortify_kunit.c +5
-2@@ -993,8 +1003,11 @@ static void fortify_test_memcmp(struct kunit *test) {char one[] ="My mind is going ..." ;char two[] ="My mind is going ... I can feel it." ; -size_t one_len =sizeof (one) + unconst -1 ; -size_t two_len =sizeof (two) + unconst -1 ; +size_t one_len =sizeof (one) -1 ; +size_t two_len =sizeof (two) -1 ; + +OPTIMIZER_HIDE_VAR (one_len); +OPTIMIZER_HIDE_VAR (two_len);
Quoted 10 lines
lib/tests/fortify_kunit.c static void fortify_test_memcmp(struct kunit *test) · line 1012
Quoted 2 lines
lib/tests/fortify_kunit.c +0
-0/* We match the first string (ignoring the %NUL). */ KUNIT_ASSERT_EQ (test,memcmp (one, two, one_len),0 );I am sorry for bringing this up some time after you sent this change, as I have only now had a chance to actually sit down and understand the results of my bisect. I am still seeing a
__read_overflowerror when buildinglib/tests/fortify_kunit.owith Fedora's configuration + LTO in next-20250317, which contains this change. I do not think it is issue 2075, as I can reproduce it without UBSAN enabled altogether. This is with LLVM 20.1.0.
Hmpf. Well, I guess I'll continue to debug these. I think I'll keep the patch that refactors volatile -- I prefer OPTIMIZER_HIDE_VAR() as it's a more direct way to indicate what I want the compiler to ignore. I've been trying to move to using it instead of my old style volatile uses.
Quoted 6 lines
$ cat kernel/configs/repro.config CONFIG_FORTIFY_KUNIT_TEST=m CONFIG_FORTIFY_SOURCE=y CONFIG_KUNIT=y # CONFIG_LTO_NONE is not set CONFIG_LTO_CLANG_THIN=y
Thanks for the config that pokes it!
Quoted 9 lines
# or x86_64 $ make -skj"$(nproc)" ARCH=arm64 LLVM=1 mrproper {def,repro.}config lib/tests/fortify_kunit.o ld.lld: error: call to __read_overflow marked "dontcall-error": detected read beyond size of object (1st parameter) make[6]: *** [scripts/Makefile.build:203: lib/tests/fortify_kunit.o] Error 1Selectively reverting this avoids the problem, which is definitely odd... Maybe issue 2075 is related more to issue 2077 and this patch should not be entertained?
Yeah, I don't yet see what is going on with these. :|
-Kees
Signature · 1 line
Kees Cook