| Message ID | 20230406000212.3442647-3-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fortify: Add KUnit tests for runtime overflows | expand |
Hi Kees,
kernel test robot noticed the following build warnings:
[auto build test WARNING on kees/for-next/hardening]
[also build test WARNING on kees/for-next/pstore kees/for-next/kspp akpm-mm/mm-everything linus/master v6.3-rc5 next-20230405]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Kees-Cook/kunit-tool-Enable-CONFIG_FORTIFY_SOURCE-under-UML/20230406-081014
base: https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/hardening
patch link: https://lore.kernel.org/r/20230406000212.3442647-3-keescook%40chromium.org
patch subject: [PATCH 3/9] string: Add Kunit tests for strcat() family
config: riscv-randconfig-r042-20230403 (https://download.01.org/0day-ci/archive/20230406/202304061243.Bx5SK1xq-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/1cdc1d7eeb09497f84367bfc2919d5fcc380e454
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Kees-Cook/kunit-tool-Enable-CONFIG_FORTIFY_SOURCE-under-UML/20230406-081014
git checkout 1cdc1d7eeb09497f84367bfc2919d5fcc380e454
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=riscv olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304061243.Bx5SK1xq-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from include/linux/err.h:5,
from include/kunit/assert.h:12,
from include/kunit/test.h:12,
from lib/strcat_kunit.c:8:
lib/strcat_kunit.c: In function 'strncat_test':
>> lib/strcat_kunit.c:52:33: warning: 'strncat' output truncated copying 2 bytes from a string of length 4 [-Wstringop-truncation]
52 | KUNIT_EXPECT_TRUE(test, strncat(dest, "ABCD", 2) == dest);
| ^~~~~~~~~~~~~~~~~~~~~~~~
include/linux/compiler.h:77:45: note: in definition of macro 'likely'
77 | # define likely(x) __builtin_expect(!!(x), 1)
| ^
include/kunit/test.h:545:9: note: in expansion of macro 'KUNIT_UNARY_ASSERTION'
545 | KUNIT_UNARY_ASSERTION(test, \
| ^~~~~~~~~~~~~~~~~~~~~
include/kunit/test.h:736:9: note: in expansion of macro 'KUNIT_TRUE_MSG_ASSERTION'
736 | KUNIT_TRUE_MSG_ASSERTION(test, \
| ^~~~~~~~~~~~~~~~~~~~~~~~
include/kunit/test.h:733:9: note: in expansion of macro 'KUNIT_EXPECT_TRUE_MSG'
733 | KUNIT_EXPECT_TRUE_MSG(test, condition, NULL)
| ^~~~~~~~~~~~~~~~~~~~~
lib/strcat_kunit.c:52:9: note: in expansion of macro 'KUNIT_EXPECT_TRUE'
52 | KUNIT_EXPECT_TRUE(test, strncat(dest, "ABCD", 2) == dest);
| ^~~~~~~~~~~~~~~~~
vim +/strncat +52 lib/strcat_kunit.c
29
30 static void strncat_test(struct kunit *test)
31 {
32 char dest[8];
33
34 /* Destination is terminated. */
35 memset(dest, 0, sizeof(dest));
36 KUNIT_EXPECT_EQ(test, strlen(dest), 0);
37 /* Empty copy of size 0 does nothing. */
38 KUNIT_EXPECT_TRUE(test, strncat(dest, "", 0) == dest);
39 KUNIT_EXPECT_STREQ(test, dest, "");
40 /* Empty copy of size 1 does nothing too. */
41 KUNIT_EXPECT_TRUE(test, strncat(dest, "", 1) == dest);
42 KUNIT_EXPECT_STREQ(test, dest, "");
43 /* Copy of max 0 characters should do nothing. */
44 KUNIT_EXPECT_TRUE(test, strncat(dest, "asdf", 0) == dest);
45 KUNIT_EXPECT_STREQ(test, dest, "");
46
47 /* 4 characters copied in, even if max is 8. */
48 KUNIT_EXPECT_TRUE(test, strncat(dest, "four\000123", 8) == dest);
49 KUNIT_EXPECT_STREQ(test, dest, "four");
50 KUNIT_EXPECT_EQ(test, dest[5], '\0');
51 /* 2 characters copied in okay, 2 ignored. */
> 52 KUNIT_EXPECT_TRUE(test, strncat(dest, "ABCD", 2) == dest);
53 KUNIT_EXPECT_STREQ(test, dest, "fourAB");
54 }
55
> +static void strncat_test(struct kunit *test) > +{ > + char dest[8]; > + > + /* Destination is terminated. */ > + memset(dest, 0, sizeof(dest)); > + KUNIT_EXPECT_EQ(test, strlen(dest), 0); > + /* Empty copy of size 0 does nothing. */ > + KUNIT_EXPECT_TRUE(test, strncat(dest, "", 0) == dest); > + KUNIT_EXPECT_STREQ(test, dest, ""); > + /* Empty copy of size 1 does nothing too. */ > + KUNIT_EXPECT_TRUE(test, strncat(dest, "", 1) == dest); > + KUNIT_EXPECT_STREQ(test, dest, ""); > + /* Copy of max 0 characters should do nothing. */ > + KUNIT_EXPECT_TRUE(test, strncat(dest, "asdf", 0) == dest); > + KUNIT_EXPECT_STREQ(test, dest, ""); > + > + /* 4 characters copied in, even if max is 8. */ > + KUNIT_EXPECT_TRUE(test, strncat(dest, "four\000123", 8) == dest); > + KUNIT_EXPECT_STREQ(test, dest, "four"); > + KUNIT_EXPECT_EQ(test, dest[5], '\0'); Maybe also add a test case for strncat(dest, "four", 4) that checks that the fourth byte of dest is not 0?
On Thu, Apr 06, 2023 at 11:11:09AM +0200, Alexander Potapenko wrote: > > +static void strncat_test(struct kunit *test) > > +{ > > + char dest[8]; > > + > > + /* Destination is terminated. */ > > + memset(dest, 0, sizeof(dest)); > > + KUNIT_EXPECT_EQ(test, strlen(dest), 0); > > + /* Empty copy of size 0 does nothing. */ > > + KUNIT_EXPECT_TRUE(test, strncat(dest, "", 0) == dest); > > + KUNIT_EXPECT_STREQ(test, dest, ""); > > + /* Empty copy of size 1 does nothing too. */ > > + KUNIT_EXPECT_TRUE(test, strncat(dest, "", 1) == dest); > > + KUNIT_EXPECT_STREQ(test, dest, ""); > > + /* Copy of max 0 characters should do nothing. */ > > + KUNIT_EXPECT_TRUE(test, strncat(dest, "asdf", 0) == dest); > > + KUNIT_EXPECT_STREQ(test, dest, ""); > > + > > + /* 4 characters copied in, even if max is 8. */ > > + KUNIT_EXPECT_TRUE(test, strncat(dest, "four\000123", 8) == dest); > > + KUNIT_EXPECT_STREQ(test, dest, "four"); > > + KUNIT_EXPECT_EQ(test, dest[5], '\0'); > > Maybe also add a test case for strncat(dest, "four", 4) that checks > that the fourth byte of dest is not 0? I think I don't understand what state you want to test for? The line above (STREQ is checking dest is "four". Maybe I should check for dest[6] being 0 as well as dest[5]. But if that's not what you mean, I'm not sure. Is it something here: char dest[16]; memset(dest, 0, sizeof(dest)); // dest == "" strncat(dest, "four", 4); // dest == "four" strncat(dest, "four", 4); // dest == "fourfour" strncat's "n" is how much to reach from source -- dest will always be terminated.
On Fri, Apr 7, 2023 at 1:07 AM Kees Cook <keescook@chromium.org> wrote: > > On Thu, Apr 06, 2023 at 11:11:09AM +0200, Alexander Potapenko wrote: > > > +static void strncat_test(struct kunit *test) > > > +{ > > > + char dest[8]; > > > + > > > + /* Destination is terminated. */ > > > + memset(dest, 0, sizeof(dest)); > > > + KUNIT_EXPECT_EQ(test, strlen(dest), 0); > > > + /* Empty copy of size 0 does nothing. */ > > > + KUNIT_EXPECT_TRUE(test, strncat(dest, "", 0) == dest); > > > + KUNIT_EXPECT_STREQ(test, dest, ""); > > > + /* Empty copy of size 1 does nothing too. */ > > > + KUNIT_EXPECT_TRUE(test, strncat(dest, "", 1) == dest); > > > + KUNIT_EXPECT_STREQ(test, dest, ""); > > > + /* Copy of max 0 characters should do nothing. */ > > > + KUNIT_EXPECT_TRUE(test, strncat(dest, "asdf", 0) == dest); > > > + KUNIT_EXPECT_STREQ(test, dest, ""); > > > + > > > + /* 4 characters copied in, even if max is 8. */ > > > + KUNIT_EXPECT_TRUE(test, strncat(dest, "four\000123", 8) == dest); > > > + KUNIT_EXPECT_STREQ(test, dest, "four"); > > > + KUNIT_EXPECT_EQ(test, dest[5], '\0'); > > > > Maybe also add a test case for strncat(dest, "four", 4) that checks > > that the fourth byte of dest is not 0? > > I think I don't understand what state you want to test for? The line > above (STREQ is checking dest is "four". Maybe I should check for > dest[6] being 0 as well as dest[5]. But if that's not what you mean, I'm > not sure. Is it something here: Sorry, not sure why I wrote "strncat" here, I was thinking about strncpy, which is handled in a different patch. Please disregard. > char dest[16]; > memset(dest, 0, sizeof(dest)); > // dest == "" > strncat(dest, "four", 4); > // dest == "four" > strncat(dest, "four", 4); > // dest == "fourfour" > > strncat's "n" is how much to reach from source -- dest will always be > terminated. > > -- > Kees Cook
diff --git a/MAINTAINERS b/MAINTAINERS index ec57c42ed544..86c0012b5130 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8021,6 +8021,7 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/har F: include/linux/fortify-string.h F: lib/fortify_kunit.c F: lib/memcpy_kunit.c +F: lib/strcat_kunit.c F: lib/strscpy_kunit.c F: lib/test_fortify/* F: scripts/test_fortify.sh diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index d48a5f4b471e..86157aa5e979 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -2631,6 +2631,11 @@ config HW_BREAKPOINT_KUNIT_TEST If unsure, say N. +config STRCAT_KUNIT_TEST + tristate "Test strcat() family of functions at runtime" if !KUNIT_ALL_TESTS + depends on KUNIT + default KUNIT_ALL_TESTS + config STRSCPY_KUNIT_TEST tristate "Test strscpy*() family of functions at runtime" if !KUNIT_ALL_TESTS depends on KUNIT diff --git a/lib/Makefile b/lib/Makefile index baf2821f7a00..6582d8fe1a77 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -389,6 +389,7 @@ obj-$(CONFIG_STACKINIT_KUNIT_TEST) += stackinit_kunit.o CFLAGS_fortify_kunit.o += $(call cc-disable-warning, unsequenced) CFLAGS_fortify_kunit.o += $(DISABLE_STRUCTLEAK_PLUGIN) obj-$(CONFIG_FORTIFY_KUNIT_TEST) += fortify_kunit.o +obj-$(CONFIG_STRCAT_KUNIT_TEST) += strcat_kunit.o obj-$(CONFIG_STRSCPY_KUNIT_TEST) += strscpy_kunit.o obj-$(CONFIG_SIPHASH_KUNIT_TEST) += siphash_kunit.o diff --git a/lib/strcat_kunit.c b/lib/strcat_kunit.c new file mode 100644 index 000000000000..b6428c3a557f --- /dev/null +++ b/lib/strcat_kunit.c @@ -0,0 +1,100 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Kernel module for testing 'strcat' family of functions. + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <kunit/test.h> +#include <linux/string.h> + +static void strcat_test(struct kunit *test) +{ + char dest[8]; + + /* Destination is terminated. */ + memset(dest, 0, sizeof(dest)); + KUNIT_EXPECT_EQ(test, strlen(dest), 0); + /* Empty copy does nothing. */ + KUNIT_EXPECT_TRUE(test, strcat(dest, "") == dest); + KUNIT_EXPECT_STREQ(test, dest, ""); + /* 4 characters copied in, stops at %NUL. */ + KUNIT_EXPECT_TRUE(test, strcat(dest, "four\000123") == dest); + KUNIT_EXPECT_STREQ(test, dest, "four"); + KUNIT_EXPECT_EQ(test, dest[5], '\0'); + /* 2 more characters copied in okay. */ + KUNIT_EXPECT_TRUE(test, strcat(dest, "AB") == dest); + KUNIT_EXPECT_STREQ(test, dest, "fourAB"); +} + +static void strncat_test(struct kunit *test) +{ + char dest[8]; + + /* Destination is terminated. */ + memset(dest, 0, sizeof(dest)); + KUNIT_EXPECT_EQ(test, strlen(dest), 0); + /* Empty copy of size 0 does nothing. */ + KUNIT_EXPECT_TRUE(test, strncat(dest, "", 0) == dest); + KUNIT_EXPECT_STREQ(test, dest, ""); + /* Empty copy of size 1 does nothing too. */ + KUNIT_EXPECT_TRUE(test, strncat(dest, "", 1) == dest); + KUNIT_EXPECT_STREQ(test, dest, ""); + /* Copy of max 0 characters should do nothing. */ + KUNIT_EXPECT_TRUE(test, strncat(dest, "asdf", 0) == dest); + KUNIT_EXPECT_STREQ(test, dest, ""); + + /* 4 characters copied in, even if max is 8. */ + KUNIT_EXPECT_TRUE(test, strncat(dest, "four\000123", 8) == dest); + KUNIT_EXPECT_STREQ(test, dest, "four"); + KUNIT_EXPECT_EQ(test, dest[5], '\0'); + /* 2 characters copied in okay, 2 ignored. */ + KUNIT_EXPECT_TRUE(test, strncat(dest, "ABCD", 2) == dest); + KUNIT_EXPECT_STREQ(test, dest, "fourAB"); +} + +static void strlcat_test(struct kunit *test) +{ + char dest[8] = ""; + + /* Destination is terminated. */ + KUNIT_EXPECT_EQ(test, strlen(dest), 0); + /* Empty copy is size 0. */ + KUNIT_EXPECT_EQ(test, strlcat(dest, "", sizeof(dest)), 0); + KUNIT_EXPECT_STREQ(test, dest, ""); + /* Size 1 should keep buffer terminated, report size of source only. */ + KUNIT_EXPECT_EQ(test, strlcat(dest, "four", 1), 4); + KUNIT_EXPECT_STREQ(test, dest, ""); + + /* 4 characters copied in. */ + KUNIT_EXPECT_EQ(test, strlcat(dest, "four", sizeof(dest)), 4); + KUNIT_EXPECT_STREQ(test, dest, "four"); + /* 2 characters copied in okay, gets to 6 total. */ + KUNIT_EXPECT_EQ(test, strlcat(dest, "AB", sizeof(dest)), 6); + KUNIT_EXPECT_STREQ(test, dest, "fourAB"); + /* 2 characters ignored if max size (7) reached. */ + KUNIT_EXPECT_EQ(test, strlcat(dest, "CD", 7), 8); + KUNIT_EXPECT_STREQ(test, dest, "fourAB"); + /* 1 of 2 characters skipped, now at true max size. */ + KUNIT_EXPECT_EQ(test, strlcat(dest, "EFG", sizeof(dest)), 9); + KUNIT_EXPECT_STREQ(test, dest, "fourABE"); + /* Everything else ignored, now at full size. */ + KUNIT_EXPECT_EQ(test, strlcat(dest, "1234", sizeof(dest)), 11); + KUNIT_EXPECT_STREQ(test, dest, "fourABE"); +} + +static struct kunit_case strcat_test_cases[] = { + KUNIT_CASE(strcat_test), + KUNIT_CASE(strncat_test), + KUNIT_CASE(strlcat_test), + {} +}; + +static struct kunit_suite strcat_test_suite = { + .name = "strcat", + .test_cases = strcat_test_cases, +}; + +kunit_test_suite(strcat_test_suite); + +MODULE_LICENSE("GPL");