Skip to contentSkip to conversation

fortify: Add KUnit tests for runtime overflows

PATCH Superseded Kees Cook 10 participants 32 messages Apr 6, 2023 – Feb 22, 2024

Subsystems (6)

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Kees Cook

author
#0 lore raw

cover fortify: Add KUnit tests for runtime overflows

Hi,

This series adds KUnit tests for the CONFIG_FORTIFY_SOURCE behavior of the standard C string functions, and for the strcat() family of functions, as those were updated during refactoring. Finally, fortification error messages are improved to give more context for the failure condition.

-Kees

Kees Cook (9):
kunit: tool: Enable CONFIG_FORTIFY_SOURCE under UML
fortify: Allow KUnit test to build without FORTIFY
string: Add Kunit tests for strcat() family
fortify: Add protection for strlcat()
fortify: strcat: Move definition to use fortified strlcat()
fortify: Split reporting and avoid passing string pointer
fortify: Provide KUnit counters for failure testing
fortify: Add KUnit tests for runtime overflows
fortify: Improve buffer overflow reporting

 MAINTAINERS                                  |   1 +
 include/linux/fortify-string.h               | 204 +++--
 lib/Kconfig.debug                            |   7 +-
 lib/Makefile                                 |   1 +
 lib/fortify_kunit.c                          | 795 +++++++++++++++++++
 lib/strcat_kunit.c                           | 100 +++
 lib/string_helpers.c                         |  74 +-
 tools/objtool/check.c                        |   2 +-
 tools/testing/kunit/configs/all_tests.config |   2 +
 tools/testing/kunit/configs/arch_uml.config  |   3 +
 10 files changed, 1133 insertions(+), 56 deletions(-)
 create mode 100644 lib/strcat_kunit.c
Signature · 1 line
2.34.1
author
#1 lore raw

PATCH 1/9 kunit: tool: Enable CONFIG_FORTIFY_SOURCE under UML

From: Kees Cook <[email protected]>

Since commit ba38961a069b ("um: Enable FORTIFY_SOURCE"), it's possible to run the FORTIFY tests under UML. Enable CONFIG_FORTIFY_SOURCE when running with --altests to gain additional coverage, and by default under UML.

---
 tools/testing/kunit/configs/all_tests.config | 2 ++
 tools/testing/kunit/configs/arch_uml.config  | 3 +++
 2 files changed, 5 insertions(+)
tools/testing/kunit/configs/all_tests.config +2 -0
index f990cbb73250..0393940c706a 100644
@@ -9,6 +9,8 @@ CONFIG_KUNIT=y
 CONFIG_KUNIT_EXAMPLE_TEST=y
 CONFIG_KUNIT_ALL_TESTS=y
 
+CONFIG_FORTIFY_SOURCE=y
+
 CONFIG_IIO=y
 
 CONFIG_EXT4_FS=y
tools/testing/kunit/configs/arch_uml.config +3 -0
index e824ce43b05a..54ad8972681a 100644
@@ -3,3 +3,6 @@
 # Enable virtio/pci, as a lot of tests require it.
 CONFIG_VIRTIO_UML=y
 CONFIG_UML_PCI_OVER_VIRTIO=y
+
+# Enable FORTIFY_SOURCE for wider checking.
+CONFIG_FORTIFY_SOURCE=y
Signature · 1 line
2.34.1
#2 lore raw

Re: [PATCH 1/9] kunit: tool: Enable CONFIG_FORTIFY_SOURCE under UML

Quoted 1 line · Kees Cook

From: Kees Cook <[email protected]>

Hah oops. Wrong From/SoB. That's what I get for writing this on a plane.

Signature · 1 line
Kees Cook
author
#3 lore raw

PATCH 2/9 fortify: Allow KUnit test to build without FORTIFY

In reply to Kees Cook

From: Kees Cook <[email protected]>

In order for CI systems to notice all the skipped tests related to CONFIG_FORTIFY_SOURCE, allow the FORTIFY_SOURCE KUnit tests to build with or without CONFIG_FORTIFY_SOURCE.

---
 lib/Kconfig.debug   |  2 +-
 lib/fortify_kunit.c | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)
lib/Kconfig.debug +1 -1
index c8b379e2e9ad..d48a5f4b471e 100644
@@ -2614,7 +2614,7 @@ config STACKINIT_KUNIT_TEST
 
 config FORTIFY_KUNIT_TEST
 	tristate "Test fortified str*() and mem*() function internals at runtime" if !KUNIT_ALL_TESTS
-	depends on KUNIT && FORTIFY_SOURCE
+	depends on KUNIT
 	default KUNIT_ALL_TESTS
 	help
 	  Builds unit tests for checking internals of FORTIFY_SOURCE as used
lib/fortify_kunit.c +15 -0
index c8c33cbaae9e..d054fc20a7d5 100644
@@ -25,8 +25,21 @@ static const char array_of_10[] = "this is 10";
 static const char *ptr_of_11 = "this is 11!";
 static char array_unknown[] = "compiler thinks I might change";
 
+/* Handle being built without CONFIG_FORTIFY_SOURCE */
+#ifndef __compiletime_strlen
+# define __compiletime_strlen __builtin_strlen
+#endif
+
+#define skip_without_fortify()	\
+do {				\
+	if (!IS_ENABLED(CONFIG_FORTIFY_SOURCE))	\
+		kunit_skip(test, "Not built with CONFIG_FORTIFY_SOURCE=y"); \
+} while (0)
+
 static void known_sizes_test(struct kunit *test)
 {
+	skip_without_fortify();
+
 	KUNIT_EXPECT_EQ(test, __compiletime_strlen("88888888"), 8);
 	KUNIT_EXPECT_EQ(test, __compiletime_strlen(array_of_10), 10);
 	KUNIT_EXPECT_EQ(test, __compiletime_strlen(ptr_of_11), 11);
@@ -60,6 +73,8 @@ static noinline size_t want_minus_one(int pick)
 
 static void control_flow_split_test(struct kunit *test)
 {
+	skip_without_fortify();
+
 	KUNIT_EXPECT_EQ(test, want_minus_one(pick), SIZE_MAX);
 }
 
Signature · 1 line
2.34.1
#4 lore raw

Re: [PATCH 2/9] fortify: Allow KUnit test to build without FORTIFY

Quoted 6 lines · Kees Cook

From: Kees Cook <[email protected]>

In order for CI systems to notice all the skipped tests related to CONFIG_FORTIFY_SOURCE, allow the FORTIFY_SOURCE KUnit tests to build with or without CONFIG_FORTIFY_SOURCE.

Hmm, I wonder if this warrants a deeper discussion.
It's a lot easier to have tests get disabled by kconfig if their deps aren't met.

If there's pressure to have them compiled and just get marked skipped, that sounds like that could be annoying.
Esp. in the cases where more code needs to be put behind

#ifdef CONFIG_MY_DEP
<test helpers, etc>
#endif

But I have a suggestion below to simplify this a bit

Quoted 28 lines
---
 lib/Kconfig.debug   |  2 +-
 lib/fortify_kunit.c | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)
lib/Kconfig.debug +1 -1
index c8b379e2e9ad..d48a5f4b471e 100644
@@ -2614,7 +2614,7 @@ config STACKINIT_KUNIT_TEST
 
 config FORTIFY_KUNIT_TEST
        tristate "Test fortified str*() and mem*() function internals at runtime" if !KUNIT_ALL_TESTS
-       depends on KUNIT && FORTIFY_SOURCE
+       depends on KUNIT
        default KUNIT_ALL_TESTS
        help
          Builds unit tests for checking internals of FORTIFY_SOURCE as used
lib/fortify_kunit.c +0 -0
index c8c33cbaae9e..d054fc20a7d5 100644
@@ -25,8 +25,21 @@ static const char array_of_10[] = "this is 10";
 static const char *ptr_of_11 = "this is 11!";
 static char array_unknown[] = "compiler thinks I might change";
 

lib/fortify_kunit.c static const char array_of_10[] = "this is 10"; · line 37

Quoted 10 lines
lib/fortify_kunit.c +10 -0
+/* Handle being built without CONFIG_FORTIFY_SOURCE */
+#ifndef __compiletime_strlen
+# define __compiletime_strlen __builtin_strlen
+#endif
+
+#define skip_without_fortify() \
+do {                           \
+       if (!IS_ENABLED(CONFIG_FORTIFY_SOURCE)) \
+               kunit_skip(test, "Not built with CONFIG_FORTIFY_SOURCE=y"); \
+} while (0)

Note: you can add an init function to the suite and skip the tests there.

static void fortify_init(struct kunit *test) {
       if (!IS_ENABLED(CONFIG_FORTIFY_SOURCE))
               kunit_skip(test, "Not built with CONFIG_FORTIFY_SOURCE=y");
}

...

  static struct kunit_suite fortify_test_suite = {
          .name = "fortify",

+ .init = fortify_init,
.test_cases = fortify_test_cases,
};

That way we don't have to add it to each test case.

author
#5 lore raw

Re: [PATCH 2/9] fortify: Allow KUnit test to build without FORTIFY

Quoted 8 lines · Daniel Latypov
Quoted 3 lines · Kees Cook

In order for CI systems to notice all the skipped tests related to CONFIG_FORTIFY_SOURCE, allow the FORTIFY_SOURCE KUnit tests to build with or without CONFIG_FORTIFY_SOURCE.

Hmm, I wonder if this warrants a deeper discussion.
It's a lot easier to have tests get disabled by kconfig if their deps aren't met.

Yeah, I wasn't sure where to put the "kunit defconfig" settings. "default.config" didn't seem to actually work as I was expecting. The real "problem" I'm solving is that FORTIFY_SOURCE isn't in the standard defconfig.

Quoted 56 lines

If there's pressure to have them compiled and just get marked skipped, that sounds like that could be annoying.
Esp. in the cases where more code needs to be put behind

#ifdef CONFIG_MY_DEP
<test helpers, etc>
#endif

But I have a suggestion below to simplify this a bit

Quoted 28 lines
---
 lib/Kconfig.debug   |  2 +-
 lib/fortify_kunit.c | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)
lib/Kconfig.debug +1 -1
index c8b379e2e9ad..d48a5f4b471e 100644
@@ -2614,7 +2614,7 @@ config STACKINIT_KUNIT_TEST
 
 config FORTIFY_KUNIT_TEST
        tristate "Test fortified str*() and mem*() function internals at runtime" if !KUNIT_ALL_TESTS
-       depends on KUNIT && FORTIFY_SOURCE
+       depends on KUNIT
        default KUNIT_ALL_TESTS
        help
          Builds unit tests for checking internals of FORTIFY_SOURCE as used
lib/fortify_kunit.c +0 -0
index c8c33cbaae9e..d054fc20a7d5 100644
@@ -25,8 +25,21 @@ static const char array_of_10[] = "this is 10";
 static const char *ptr_of_11 = "this is 11!";
 static char array_unknown[] = "compiler thinks I might change";
 

lib/fortify_kunit.c static const char array_of_10[] = "this is 10"; · line 37

Quoted 10 lines
lib/fortify_kunit.c +10 -0
+/* Handle being built without CONFIG_FORTIFY_SOURCE */
+#ifndef __compiletime_strlen
+# define __compiletime_strlen __builtin_strlen
+#endif
+
+#define skip_without_fortify() \
+do {                           \
+       if (!IS_ENABLED(CONFIG_FORTIFY_SOURCE)) \
+               kunit_skip(test, "Not built with CONFIG_FORTIFY_SOURCE=y"); \
+} while (0)

Note: you can add an init function to the suite and skip the tests there.

static void fortify_init(struct kunit *test) {
       if (!IS_ENABLED(CONFIG_FORTIFY_SOURCE))
               kunit_skip(test, "Not built with CONFIG_FORTIFY_SOURCE=y");
}
Quoted 8 lines

...

  static struct kunit_suite fortify_test_suite = {
          .name = "fortify",

+ .init = fortify_init,
.test_cases = fortify_test_cases,
};

That way we don't have to add it to each test case.

Ah! Excellent. I didn't realize it would have that effect. I will do that. :)

Signature · 1 line
Kees Cook
author
#6 lore raw

PATCH 3/9 string: Add Kunit tests for strcat() family

In reply to Kees Cook

From: Kees Cook <[email protected]>

Add tests to make sure the strcat() family of functions behave correctly.

---
 MAINTAINERS        |   1 +
 lib/Kconfig.debug  |   5 +++
 lib/Makefile       |   1 +
 lib/strcat_kunit.c | 100 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 107 insertions(+)
 create mode 100644 lib/strcat_kunit.c
MAINTAINERS +1 -0
index ec57c42ed544..86c0012b5130 100644
@@ -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
lib/Kconfig.debug +5 -0
index d48a5f4b471e..86157aa5e979 100644
@@ -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
lib/Makefile +1 -0
index baf2821f7a00..6582d8fe1a77 100644
@@ -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
 
lib/strcat_kunit.c new file +100 -0

100 lines not on this page · raw message

Signature · 1 line
2.34.1
#7 lore raw

Re: [PATCH 3/9] string: Add Kunit tests for strcat() family

kernel test robot noticed the following build warnings:

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/[email protected]/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 <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/

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	
Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
#8 lore raw

Re: [PATCH 3/9] string: Add Kunit tests for strcat() family

In reply to Kees Cook
Quoted 11 lines
lib/strcat_kunit.c new file +11 -0
+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. */

lib/strcat_kunit.c line 50

Quoted 10 lines
lib/strcat_kunit.c new file +10 -0
+       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?

author
#9 lore raw

Re: [PATCH 3/9] string: Add Kunit tests for strcat() family

Quoted 16 lines · Alexander Potapenko
Quoted 16 lines
lib/strcat_kunit.c new file +16 -0
+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, "");
Quoted 8 lines

lib/strcat_kunit.c line 50

Quoted 5 lines
lib/strcat_kunit.c new file +5 -0
+
+       /* 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.

Signature · 1 line
Kees Cook
#10 lore raw

Re: [PATCH 3/9] string: Add Kunit tests for strcat() family

Quoted 27 lines · Kees Cook
Quoted 16 lines · Alexander Potapenko
Quoted 16 lines
lib/strcat_kunit.c new file +16 -0
+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, "");
Quoted 8 lines

lib/strcat_kunit.c line 50

Quoted 5 lines
lib/strcat_kunit.c new file +5 -0
+
+       /* 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?

Quoted 4 lines

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.

Quoted 13 lines

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

Signature · 10 lines
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Liana Sebastian
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
author
#11 lore raw

PATCH 4/9 fortify: Add protection for strlcat()

In reply to Kees Cook

From: Kees Cook <[email protected]>

The definition of strcat() was was defined in terms of unfortified strlcat(), but that meant there was no bounds checking done on the internal strlen() calls, and the (bounded) copy would be performed before reporting a failure. Additionally, pathological cases (i.e. unterminated destination buffer) did not make calls to fortify_panic(), which will make future unit testing more difficult. Instead, explicitly define a fortified strlcat() wrapper for strcat() to use.

---
 include/linux/fortify-string.h | 64 ++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)
include/linux/fortify-string.h +64 -0
index c9de1f59ee80..875689aa83c3 100644
@@ -371,6 +371,70 @@ __FORTIFY_INLINE ssize_t strscpy(char * const POS p, const char * const POS q, s
 	return __real_strscpy(p, q, len);
 }
 
+/* Defined after fortified strlen() to reuse it. */
+extern size_t __real_strlcat(char *p, const char *q, size_t avail) __RENAME(strlcat);
+/**
+ * strlcat - Append a string to an existing string
+ *
+ * @p: pointer to %NUL-terminated string to append to
+ * @q: pointer to %NUL-terminated string to append from
+ * @avail: Maximum bytes available in @p
+ *
+ * Appends %NUL-terminated string @q after the %NUL-terminated
+ * string at @p, but will not write beyond @avail bytes total,
+ * potentially truncating the copy from @q. @p will stay
+ * %NUL-terminated only if a %NUL already existed within
+ * the @avail bytes of @p. If so, the resulting number of
+ * bytes copied from @q will be at most "@avail - strlen(@p) - 1".
+ *
+ * Do not use this function. While FORTIFY_SOURCE tries to avoid
+ * read and write overflows, this is only possible when the sizes
+ * of @p and @q are known to the compiler. Prefer building the
+ * string with formatting, via scnprintf(), seq_buf, or similar.
+ *
+ * Returns total bytes that _would_ have been contained by @p
+ * regardless of truncation, similar to snprintf(). If return
+ * value is >= @avail, the string has been truncated.
+ *
+ */
+__FORTIFY_INLINE
+size_t strlcat(char * const POS p, const char * const POS q, size_t avail)
+{
+	size_t p_len, copy_len;
+	size_t p_size = __member_size(p);
+	size_t q_size = __member_size(q);
+	size_t actual, wanted;
+
+	/* Give up immediately if both buffer sizes are unknown. */
+	if (p_size == SIZE_MAX && q_size == SIZE_MAX)
+		return __real_strlcat(p, q, avail);
+
+	p_len = strnlen(p, avail);
+	copy_len = strlen(q);
+	wanted = actual = p_len + copy_len;
+
+	/* Cannot append any more: report truncation. */
+	if (avail <= p_len)
+		return wanted;
+
+	/* Give up if string is already overflowed. */
+	if (p_size <= p_len)
+		fortify_panic(__func__);
+
+	if (actual >= avail) {
+		copy_len = avail - p_len - 1;
+		actual = p_len + copy_len;
+	}
+
+	/* Give up if copy will overflow. */
+	if (p_size <= actual)
+		fortify_panic(__func__);
+	__underlying_memcpy(p + p_len, q, copy_len);
+	p[actual] = '\0';
+
+	return wanted;
+}
+
 /**
  * strncat - Append a string to an existing string
  *
Signature · 1 line
2.34.1
#12 lore raw

Re: [PATCH 4/9] fortify: Add protection for strlcat()

include/linux/fortify-string.h __FORTIFY_INLINE ssize_t strscpy(char * const POS p, const char * const POS q, s · line 405

Quoted 3 lines · Kees Cook
include/linux/fortify-string.h +2 -0
+       size_t p_size = __member_size(p);
+       size_t q_size = __member_size(q);

Since I noticed the patches appear to go for const-correctness even for parameters (which is great, I support it), these could be const too.

Cheers,
Miguel

author
#13 lore raw

Re: [PATCH 4/9] fortify: Add protection for strlcat()

Quoted 8 lines · Miguel Ojeda

include/linux/fortify-string.h __FORTIFY_INLINE ssize_t strscpy(char * const POS p, const char * const POS q, s · line 405

Quoted 3 lines · Kees Cook
include/linux/fortify-string.h +2 -0
+       size_t p_size = __member_size(p);
+       size_t q_size = __member_size(q);

Since I noticed the patches appear to go for const-correctness even for parameters (which is great, I support it), these could be const too.

Will do. Thanks!

Signature · 1 line
Kees Cook
author
#14 lore raw

PATCH 5/9 fortify: strcat: Move definition to use fortified strlcat()

In reply to Kees Cook

From: Kees Cook <[email protected]>

Move the definition of fortified strcat() to after strlcat() to use it for bounds checking.

---
 include/linux/fortify-string.h | 53 +++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 27 deletions(-)
include/linux/fortify-string.h +26 -27
index 875689aa83c3..41dbd641f55c 100644
@@ -151,33 +151,6 @@ char *strncpy(char * const POS p, const char *q, __kernel_size_t size)
 	return __underlying_strncpy(p, q, size);
 }
 
-/**
- * strcat - Append a string to an existing string
- *
- * @p: pointer to NUL-terminated string to append to
- * @q: pointer to NUL-terminated source string to append from
- *
- * Do not use this function. While FORTIFY_SOURCE tries to avoid
- * read and write overflows, this is only possible when the
- * destination buffer size is known to the compiler. Prefer
- * building the string with formatting, via scnprintf() or similar.
- * At the very least, use strncat().
- *
- * Returns @p.
- *
- */
-__FORTIFY_INLINE __diagnose_as(__builtin_strcat, 1, 2)
-char *strcat(char * const POS p, const char *q)
-{
-	size_t p_size = __member_size(p);
-
-	if (p_size == SIZE_MAX)
-		return __underlying_strcat(p, q);
-	if (strlcat(p, q, p_size) >= p_size)
-		fortify_panic(__func__);
-	return p;
-}
-
 extern __kernel_size_t __real_strnlen(const char *, __kernel_size_t) __RENAME(strnlen);
 /**
  * strnlen - Return bounded count of characters in a NUL-terminated string
@@ -435,6 +408,32 @@ size_t strlcat(char * const POS p, const char * const POS q, size_t avail)
 	return wanted;
 }
 
+/* Defined after fortified strlcat() to reuse it. */
+/**
+ * strcat - Append a string to an existing string
+ *
+ * @p: pointer to NUL-terminated string to append to
+ * @q: pointer to NUL-terminated source string to append from
+ *
+ * Do not use this function. While FORTIFY_SOURCE tries to avoid
+ * read and write overflows, this is only possible when the
+ * destination buffer size is known to the compiler. Prefer
+ * building the string with formatting, via scnprintf() or similar.
+ * At the very least, use strncat().
+ *
+ * Returns @p.
+ *
+ */
+__FORTIFY_INLINE __diagnose_as(__builtin_strcat, 1, 2)
+char *strcat(char * const POS p, const char *q)
+{
+	size_t p_size = __member_size(p);
+
+	if (strlcat(p, q, p_size) >= p_size)
+		fortify_panic(__func__);
+	return p;
+}
+
 /**
  * strncat - Append a string to an existing string
  *
Signature · 1 line
2.34.1
author
#15 lore raw

PATCH 6/9 fortify: Split reporting and avoid passing string pointer

In reply to Kees Cook

In preparation for KUnit testing and further improvements in fortify failure reporting, split out the report and encode the function and access failure (read or write overflow) into a single int argument. This mainly ends up saving some space in the data segment. For a defconfig with FORTIFY_SOURCE enabled:

$ size gcc/vmlinux.before gcc/vmlinux.after
   text  	  data     bss     dec    	    hex filename
26132309        9760658 2195460 38088427        2452eeb gcc/vmlinux.before
26132386        9748382 2195460 38076228        244ff44 gcc/vmlinux.after
Cc Andy Shevchenko <[email protected]>
Cc Cezary Rojewski <[email protected]>
Cc Puyou Lu <[email protected]>
Cc Mark Brown <[email protected]>
---
 include/linux/fortify-string.h | 72 +++++++++++++++++++++++-----------
 lib/string_helpers.c           | 70 +++++++++++++++++++++++++++++++--
 tools/objtool/check.c          |  2 +-
 3 files changed, 118 insertions(+), 26 deletions(-)
include/linux/fortify-string.h +50 -22
index 41dbd641f55c..6db4052db459 100644
@@ -9,7 +9,34 @@
 #define __FORTIFY_INLINE extern __always_inline __gnu_inline __overloadable
 #define __RENAME(x) __asm__(#x)
 
-void fortify_panic(const char *name) __noreturn __cold;
+#define fortify_reason(func, write)	(((func) << 1) | !!(write))
+
+#define fortify_panic(func, write)	\
+	__fortify_panic(fortify_reason(func, write))
+
+#define FORTIFY_READ		 0
+#define FORTIFY_WRITE		 1
+
+#define FORTIFY_FUNC_strncpy	 0
+#define FORTIFY_FUNC_strnlen	 1
+#define FORTIFY_FUNC_strlen	 2
+#define FORTIFY_FUNC_strlcpy	 3
+#define FORTIFY_FUNC_strscpy	 4
+#define FORTIFY_FUNC_strlcat	 5
+#define FORTIFY_FUNC_strcat	 6
+#define FORTIFY_FUNC_strncat	 7
+#define FORTIFY_FUNC_memset	 8
+#define FORTIFY_FUNC_memcpy	 9
+#define FORTIFY_FUNC_memmove	10
+#define FORTIFY_FUNC_memscan	11
+#define FORTIFY_FUNC_memcmp	12
+#define FORTIFY_FUNC_memchr	13
+#define FORTIFY_FUNC_memchr_inv	14
+#define FORTIFY_FUNC_kmemdup	15
+#define FORTIFY_FUNC_strcpy	16
+
+void __fortify_report(u8 reason);
+void __fortify_panic(u8 reason) __cold __noreturn;
 void __read_overflow(void) __compiletime_error("detected read beyond size of object (1st parameter)");
 void __read_overflow2(void) __compiletime_error("detected read beyond size of object (2nd parameter)");
 void __read_overflow2_field(size_t avail, size_t wanted) __compiletime_warning("detected read beyond size of field (2nd parameter); maybe use struct_group()?");
@@ -147,7 +174,7 @@ char *strncpy(char * const POS p, const char *q, __kernel_size_t size)
 	if (__compiletime_lessthan(p_size, size))
 		__write_overflow();
 	if (p_size < size)
-		fortify_panic(__func__);
+		fortify_panic(FORTIFY_FUNC_strncpy, FORTIFY_WRITE);
 	return __underlying_strncpy(p, q, size);
 }
 
@@ -178,7 +205,7 @@ __FORTIFY_INLINE __kernel_size_t strnlen(const char * const POS p, __kernel_size
 	/* Do not check characters beyond the end of p. */
 	ret = __real_strnlen(p, maxlen < p_size ? maxlen : p_size);
 	if (p_size <= ret && maxlen != ret)
-		fortify_panic(__func__);
+		fortify_panic(FORTIFY_FUNC_strnlen, FORTIFY_READ);
 	return ret;
 }
 
@@ -214,7 +241,7 @@ __kernel_size_t __fortify_strlen(const char * const POS p)
 		return __underlying_strlen(p);
 	ret = strnlen(p, p_size);
 	if (p_size <= ret)
-		fortify_panic(__func__);
+		fortify_panic(FORTIFY_FUNC_strlen, FORTIFY_READ);
 	return ret;
 }
 
@@ -256,7 +283,7 @@ __FORTIFY_INLINE size_t strlcpy(char * const POS p, const char * const POS q, si
 	}
 	if (size) {
 		if (len >= p_size)
-			fortify_panic(__func__);
+			fortify_panic(FORTIFY_FUNC_strlcpy, FORTIFY_WRITE);
 		__underlying_memcpy(p, q, len);
 		p[len] = '\0';
 	}
@@ -334,7 +361,7 @@ __FORTIFY_INLINE ssize_t strscpy(char * const POS p, const char * const POS q, s
 	 * p_size.
 	 */
 	if (len > p_size)
-		fortify_panic(__func__);
+		fortify_panic(FORTIFY_FUNC_strscpy, FORTIFY_WRITE);
 
 	/*
 	 * We can now safely call vanilla strscpy because we are protected from:
@@ -392,7 +419,7 @@ size_t strlcat(char * const POS p, const char * const POS q, size_t avail)
 
 	/* Give up if string is already overflowed. */
 	if (p_size <= p_len)
-		fortify_panic(__func__);
+		fortify_panic(FORTIFY_FUNC_strlcat, FORTIFY_READ);
 
 	if (actual >= avail) {
 		copy_len = avail - p_len - 1;
@@ -401,7 +428,7 @@ size_t strlcat(char * const POS p, const char * const POS q, size_t avail)
 
 	/* Give up if copy will overflow. */
 	if (p_size <= actual)
-		fortify_panic(__func__);
+		fortify_panic(FORTIFY_FUNC_strlcat, FORTIFY_WRITE);
 	__underlying_memcpy(p + p_len, q, copy_len);
 	p[actual] = '\0';
 
@@ -430,7 +457,7 @@ char *strcat(char * const POS p, const char *q)
 	size_t p_size = __member_size(p);
 
 	if (strlcat(p, q, p_size) >= p_size)
-		fortify_panic(__func__);
+		fortify_panic(FORTIFY_FUNC_strcat, FORTIFY_WRITE);
 	return p;
 }
 
@@ -466,7 +493,7 @@ char *strncat(char * const POS p, const char * const POS q, __kernel_size_t coun
 	p_len = strlen(p);
 	copy_len = strnlen(q, count);
 	if (p_size < p_len + copy_len + 1)
-		fortify_panic(__func__);
+		fortify_panic(FORTIFY_FUNC_strncat, FORTIFY_WRITE);
 	__underlying_memcpy(p + p_len, q, copy_len);
 	p[p_len + copy_len] = '\0';
 	return p;
@@ -507,7 +534,7 @@ __FORTIFY_INLINE void fortify_memset_chk(__kernel_size_t size,
 	 * lengths are unknown.)
 	 */
 	if (p_size != SIZE_MAX && p_size < size)
-		fortify_panic("memset");
+		fortify_panic(FORTIFY_FUNC_memset, FORTIFY_WRITE);
 }
 
 #define __fortify_memset_chk(p, c, size, p_size, p_size_field) ({	\
@@ -561,7 +588,7 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,
 					 const size_t q_size,
 					 const size_t p_size_field,
 					 const size_t q_size_field,
-					 const char *func)
+					 const u8 func)
 {
 	if (__builtin_constant_p(size)) {
 		/*
@@ -605,9 +632,10 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,
 	 * (The SIZE_MAX test is to optimize away checks where the buffer
 	 * lengths are unknown.)
 	 */
-	if ((p_size != SIZE_MAX && p_size < size) ||
-	    (q_size != SIZE_MAX && q_size < size))
-		fortify_panic(func);
+	if (p_size != SIZE_MAX && p_size < size)
+		fortify_panic(func, FORTIFY_WRITE);
+	else if (q_size != SIZE_MAX && q_size < size)
+		fortify_panic(func, FORTIFY_READ);
 
 	/*
 	 * Warn when writing beyond destination field size.
@@ -640,7 +668,7 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,
 	const size_t __q_size_field = (q_size_field);			\
 	WARN_ONCE(fortify_memcpy_chk(__fortify_size, __p_size,		\
 				     __q_size, __p_size_field,		\
-				     __q_size_field, #op),		\
+				     __q_size_field, FORTIFY_FUNC_ ##op), \
 		  #op ": detected field-spanning write (size %zu) of single %s (size %zu)\n", \
 		  __fortify_size,					\
 		  "field \"" #p "\" at " __FILE__ ":" __stringify(__LINE__), \
@@ -707,7 +735,7 @@ __FORTIFY_INLINE void *memscan(void * const POS0 p, int c, __kernel_size_t size)
 	if (__compiletime_lessthan(p_size, size))
 		__read_overflow();
 	if (p_size < size)
-		fortify_panic(__func__);
+		fortify_panic(FORTIFY_FUNC_memscan, FORTIFY_READ);
 	return __real_memscan(p, c, size);
 }
 
@@ -724,7 +752,7 @@ int memcmp(const void * const POS0 p, const void * const POS0 q, __kernel_size_t
 			__read_overflow2();
 	}
 	if (p_size < size || q_size < size)
-		fortify_panic(__func__);
+		fortify_panic(FORTIFY_FUNC_memcmp, FORTIFY_READ);
 	return __underlying_memcmp(p, q, size);
 }
 
@@ -736,7 +764,7 @@ void *memchr(const void * const POS0 p, int c, __kernel_size_t size)
 	if (__compiletime_lessthan(p_size, size))
 		__read_overflow();
 	if (p_size < size)
-		fortify_panic(__func__);
+		fortify_panic(FORTIFY_FUNC_memchr, FORTIFY_READ);
 	return __underlying_memchr(p, c, size);
 }
 
@@ -748,7 +776,7 @@ __FORTIFY_INLINE void *memchr_inv(const void * const POS0 p, int c, size_t size)
 	if (__compiletime_lessthan(p_size, size))
 		__read_overflow();
 	if (p_size < size)
-		fortify_panic(__func__);
+		fortify_panic(FORTIFY_FUNC_memchr_inv, FORTIFY_READ);
 	return __real_memchr_inv(p, c, size);
 }
 
@@ -761,7 +789,7 @@ __FORTIFY_INLINE void *kmemdup(const void * const POS0 p, size_t size, gfp_t gfp
 	if (__compiletime_lessthan(p_size, size))
 		__read_overflow();
 	if (p_size < size)
-		fortify_panic(__func__);
+		fortify_panic(FORTIFY_FUNC_kmemdup, FORTIFY_READ);
 	return __real_kmemdup(p, size, gfp);
 }
 
@@ -798,7 +826,7 @@ char *strcpy(char * const POS p, const char * const POS q)
 		__write_overflow();
 	/* Run-time check for dynamic size overflow. */
 	if (p_size < size)
-		fortify_panic(__func__);
+		fortify_panic(FORTIFY_FUNC_strcpy, FORTIFY_WRITE);
 	__underlying_memcpy(p, q, size);
 	return p;
 }
lib/string_helpers.c +67 -3

77 lines not on this page · raw message

tools/objtool/check.c +1 -1
index f937be1afe65..2d0a67ce1c51 100644
@@ -197,6 +197,7 @@ static bool __dead_end_function(struct objtool_file *file, struct symbol *func,
 	 * attribute isn't provided in ELF data. Keep 'em sorted.
 	 */
 	static const char * const global_noreturns[] = {
+		"__fortify_panic",
 		"__invalid_creds",
 		"__module_put_and_kthread_exit",
 		"__reiserfs_panic",
@@ -208,7 +209,6 @@ static bool __dead_end_function(struct objtool_file *file, struct symbol *func,
 		"do_group_exit",
 		"do_task_dead",
 		"ex_handler_msr_mce",
-		"fortify_panic",
 		"kthread_complete_and_exit",
 		"kthread_exit",
 		"kunit_try_catch_throw",
Signature · 1 line
2.34.1
#16 lore raw

Re: [PATCH 6/9] fortify: Split reporting and avoid passing string pointer

Quoted 11 lines · Kees Cook

In preparation for KUnit testing and further improvements in fortify failure reporting, split out the report and encode the function and access failure (read or write overflow) into a single int argument. This mainly ends up saving some space in the data segment. For a defconfig with FORTIFY_SOURCE enabled:

$ size gcc/vmlinux.before gcc/vmlinux.after
   text           data     bss     dec              hex filename
26132309        9760658 2195460 38088427        2452eeb gcc/vmlinux.before
26132386        9748382 2195460 38076228        244ff44 gcc/vmlinux.after

...

lib/string_helpers.c EXPORT_SYMBOL(__read_overflow2_field); · line 1027

Quoted 2 lines
lib/string_helpers.c +2 -0
+       const char *name;
+       const bool write = !!(reason & 0x1);

Perhaps define that as

FORTIFY_READ_WRITE BIT(0)
FORTIFY_FUNC_SHIFT 1

const bool write = reason & FORTIFY_READ_WRITE; // and note no need for !! part

switch (reason >> FORTIFY_FUNC_SHIFT) {

Quoted 55 lines
lib/string_helpers.c +55 -0
+       switch (reason >> 1) {
+       case FORTIFY_FUNC_strncpy:
+               name = "strncpy";
+               break;
+       case FORTIFY_FUNC_strnlen:
+               name = "strnlen";
+               break;
+       case FORTIFY_FUNC_strlen:
+               name = "strlen";
+               break;
+       case FORTIFY_FUNC_strlcpy:
+               name = "strlcpy";
+               break;
+       case FORTIFY_FUNC_strscpy:
+               name = "strscpy";
+               break;
+       case FORTIFY_FUNC_strlcat:
+               name = "strlcat";
+               break;
+       case FORTIFY_FUNC_strcat:
+               name = "strcat";
+               break;
+       case FORTIFY_FUNC_strncat:
+               name = "strncat";
+               break;
+       case FORTIFY_FUNC_memset:
+               name = "memset";
+               break;
+       case FORTIFY_FUNC_memcpy:
+               name = "memcpy";
+               break;
+       case FORTIFY_FUNC_memmove:
+               name = "memmove";
+               break;
+       case FORTIFY_FUNC_memscan:
+               name = "memscan";
+               break;
+       case FORTIFY_FUNC_memcmp:
+               name = "memcmp";
+               break;
+       case FORTIFY_FUNC_memchr:
+               name = "memchr";
+               break;
+       case FORTIFY_FUNC_memchr_inv:
+               name = "memchr_inv";
+               break;
+       case FORTIFY_FUNC_kmemdup:
+               name = "kmemdup";
+               break;
+       case FORTIFY_FUNC_strcpy:
+               name = "strcpy";
+               break;
+       default:
+               name = "unknown";
+       }

...

Quoted 1 line

+ WARN(1, "%s: detected buffer %s overflow\n", name, write ? "write" : "read");

Using str_read_write() ?

Dunno if it's already there or needs to be added. I have some patches to move those str_*() to string_choices.h. We can also prepend yours with those.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#17 lore raw

Re: [PATCH 6/9] fortify: Split reporting and avoid passing string pointer

Quoted 18 lines · Andy Shevchenko
Quoted 11 lines · Kees Cook

In preparation for KUnit testing and further improvements in fortify failure reporting, split out the report and encode the function and access failure (read or write overflow) into a single int argument. This mainly ends up saving some space in the data segment. For a defconfig with FORTIFY_SOURCE enabled:

$ size gcc/vmlinux.before gcc/vmlinux.after
   text           data     bss     dec              hex filename
26132309        9760658 2195460 38088427        2452eeb gcc/vmlinux.before
26132386        9748382 2195460 38076228        244ff44 gcc/vmlinux.after

...

lib/string_helpers.c EXPORT_SYMBOL(__read_overflow2_field); · line 1027

Quoted 2 lines
lib/string_helpers.c +2 -0
+       const char *name;
+       const bool write = !!(reason & 0x1);
Quoted 6 lines

Perhaps define that as

FORTIFY_READ_WRITE BIT(0)
FORTIFY_FUNC_SHIFT 1

const bool write = reason & FORTIFY_READ_WRITE; // and note no need for !! part

Yeah, that reads better. The FIELD_GET suggestion down-thread is probably how I'll go.

Quoted 61 lines

switch (reason >> FORTIFY_FUNC_SHIFT) {

Quoted 55 lines
lib/string_helpers.c +55 -0
+       switch (reason >> 1) {
+       case FORTIFY_FUNC_strncpy:
+               name = "strncpy";
+               break;
+       case FORTIFY_FUNC_strnlen:
+               name = "strnlen";
+               break;
+       case FORTIFY_FUNC_strlen:
+               name = "strlen";
+               break;
+       case FORTIFY_FUNC_strlcpy:
+               name = "strlcpy";
+               break;
+       case FORTIFY_FUNC_strscpy:
+               name = "strscpy";
+               break;
+       case FORTIFY_FUNC_strlcat:
+               name = "strlcat";
+               break;
+       case FORTIFY_FUNC_strcat:
+               name = "strcat";
+               break;
+       case FORTIFY_FUNC_strncat:
+               name = "strncat";
+               break;
+       case FORTIFY_FUNC_memset:
+               name = "memset";
+               break;
+       case FORTIFY_FUNC_memcpy:
+               name = "memcpy";
+               break;
+       case FORTIFY_FUNC_memmove:
+               name = "memmove";
+               break;
+       case FORTIFY_FUNC_memscan:
+               name = "memscan";
+               break;
+       case FORTIFY_FUNC_memcmp:
+               name = "memcmp";
+               break;
+       case FORTIFY_FUNC_memchr:
+               name = "memchr";
+               break;
+       case FORTIFY_FUNC_memchr_inv:
+               name = "memchr_inv";
+               break;
+       case FORTIFY_FUNC_kmemdup:
+               name = "kmemdup";
+               break;
+       case FORTIFY_FUNC_strcpy:
+               name = "strcpy";
+               break;
+       default:
+               name = "unknown";
+       }

...

Quoted 7 lines
Quoted 1 line

+ WARN(1, "%s: detected buffer %s overflow\n", name, write ? "write" : "read");

Using str_read_write() ?

Dunno if it's already there or needs to be added. I have some patches to move those str_*() to string_choices.h. We can also prepend yours with those.

Oh! Hah. I totally forgot about str_read_write. :) I will use that.

Signature · 1 line
Kees Cook
#18 lore raw

Re: [PATCH 6/9] fortify: Split reporting and avoid passing string pointer

Quoted 2 lines · Kees Cook
Quoted 1 line · Andy Shevchenko

On Thu, Apr 6, 2023 at 3:02 AM Kees Cook <[email protected]> wrote:

...

Quoted 9 lines
Quoted 7 lines
Quoted 1 line

+ WARN(1, "%s: detected buffer %s overflow\n", name, write ? "write" : "read");

Using str_read_write() ?

Dunno if it's already there or needs to be added. I have some patches to move those str_*() to string_choices.h. We can also prepend yours with those.

Oh! Hah. I totally forgot about str_read_write. :) I will use that.

Btw, makes sense to add

  #define str_write_read(v)    str_read_write(!(v))

to the header, so we won't use negation in the parameter for better readability.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
maintainer
#19 lore raw

Re: [PATCH 6/9] fortify: Split reporting and avoid passing string pointer

Quoted 14 lines · Andy Shevchenko
Quoted 2 lines · Kees Cook
Quoted 1 line · Andy Shevchenko

On Thu, Apr 6, 2023 at 3:02 AM Kees Cook <[email protected]> wrote:

...

Quoted 8 lines
Quoted 7 lines
Quoted 1 line

+ WARN(1, "%s: detected buffer %s overflow\n", name, write ? "write" : "read");

Using str_read_write() ?

Dunno if it's already there or needs to be added. I have some patches to move those str_*() to string_choices.h. We can also prepend yours with those.

Quoted 7 lines
Quoted 1 line

Oh! Hah. I totally forgot about str_read_write. :) I will use that.

Btw, makes sense to add

#define str_write_read(v) str_read_write(!(v))

to the header, so we won't use negation in the parameter for better readability.

I ended up not going this far because the use of str_read_write() gets removed again in the last patch in the series.

Signature · 1 line
Kees Cook
#20 lore raw

Re: [PATCH 6/9] fortify: Split reporting and avoid passing string pointer

In reply to Kees Cook
kernel test robot noticed the following build errors:

Hi Kees,

kernel test robot noticed the following build errors:

[auto build test ERROR on kees/for-next/hardening]
[also build test ERROR on kees/for-next/pstore kees/for-next/kspp akpm-mm/mm-everything linus/master v6.3-rc5 next-20230406] [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-6-keescook%40chromium.org
patch subject: [PATCH 6/9] fortify: Split reporting and avoid passing string pointer
config: arm-randconfig-r025-20230403 (https://download.01.org/0day-ci/archive/20230406/[email protected]/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)

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
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/intel-lab-lkp/linux/commit/4fd520e6ee549e1ffe8859e26e57ea64b48e78ea
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 4fd520e6ee549e1ffe8859e26e57ea64b48e78ea
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>):

>> ld.lld: error: undefined symbol: __fortify_panic
   >>> referenced by fortify-string.h:208 (include/linux/fortify-string.h:208)
   >>>               arch/arm/boot/compressed/fdt_ro.o:(fdt_stringlist_count)
   >>> referenced by fortify-string.h:208 (include/linux/fortify-string.h:208)
   >>>               arch/arm/boot/compressed/fdt_ro.o:(fdt_stringlist_search)
   >>> referenced by fortify-string.h:208 (include/linux/fortify-string.h:208)
   >>>               arch/arm/boot/compressed/fdt_ro.o:(fdt_stringlist_get)
Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
#21 lore raw

Re: [PATCH 6/9] fortify: Split reporting and avoid passing string pointer

Quoted 5 lines · kernel test robot

If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot <[email protected]>
| Link:
https://lore.kernel.org/oe-kbuild-all/[email protected]/

Quoted 9 lines

All errors (new ones prefixed by >>):

Quoted 7 lines
Quoted 7 lines

ld.lld: error: undefined symbol: __fortify_panic

Quoted 6 lines

referenced by fortify-string.h:208 (include/linux/fortify-string.h:208)
arch/arm/boot/compressed/fdt_ro.o:(fdt_stringlist_count)
referenced by fortify-string.h:208 (include/linux/fortify-string.h:208)
arch/arm/boot/compressed/fdt_ro.o:(fdt_stringlist_search)
referenced by fortify-string.h:208 (include/linux/fortify-string.h:208)
arch/arm/boot/compressed/fdt_ro.o:(fdt_stringlist_get)

I get a related build failure from the same commit:

arch/arm/boot/compressed/misc.c:157:6: error: no previous prototype for '__fortify_panic' [-Werror=missing-prototypes]
157 | void __fortify_panic(const u8 reason, size_t avail, size_t size)

Arnd

author
#22 lore raw

Re: [PATCH 6/9] fortify: Split reporting and avoid passing string pointer

Quoted 17 lines · Arnd Bergmann
Quoted 5 lines · kernel test robot

If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot <[email protected]>
| Link:
https://lore.kernel.org/oe-kbuild-all/[email protected]/

Quoted 9 lines

All errors (new ones prefixed by >>):

Quoted 7 lines
Quoted 7 lines

ld.lld: error: undefined symbol: __fortify_panic

Quoted 6 lines

referenced by fortify-string.h:208 (include/linux/fortify-string.h:208)
arch/arm/boot/compressed/fdt_ro.o:(fdt_stringlist_count)
referenced by fortify-string.h:208 (include/linux/fortify-string.h:208)
arch/arm/boot/compressed/fdt_ro.o:(fdt_stringlist_search)
referenced by fortify-string.h:208 (include/linux/fortify-string.h:208)
arch/arm/boot/compressed/fdt_ro.o:(fdt_stringlist_get)

Quoted 4 lines

I get a related build failure from the same commit:

arch/arm/boot/compressed/misc.c:157:6: error: no previous prototype for '__fortify_panic' [-Werror=missing-prototypes]
157 | void __fortify_panic(const u8 reason, size_t avail, size_t size)

Whoops, thank you! This series got a refresh after you did the missing-prototypes work and I missed this one. I've fixed it and double-checked for any others. Hopefully this will be clean in the next -next. :)

-Kees

Signature · 1 line
Kees Cook
#23 lore raw

Re: [PATCH 6/9] fortify: Split reporting and avoid passing string pointer

Quoted 20 lines · Kees Cook
Quoted 19 lines · Arnd Bergmann
Quoted 5 lines · kernel test robot

If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot <[email protected]>
| Link:
https://lore.kernel.org/oe-kbuild-all/[email protected]/

Quoted 9 lines

All errors (new ones prefixed by >>):

Quoted 7 lines
Quoted 7 lines

ld.lld: error: undefined symbol: __fortify_panic

Quoted 6 lines

referenced by fortify-string.h:208 (include/linux/fortify-string.h:208)
arch/arm/boot/compressed/fdt_ro.o:(fdt_stringlist_count)
referenced by fortify-string.h:208 (include/linux/fortify-string.h:208)
arch/arm/boot/compressed/fdt_ro.o:(fdt_stringlist_search)
referenced by fortify-string.h:208 (include/linux/fortify-string.h:208)
arch/arm/boot/compressed/fdt_ro.o:(fdt_stringlist_get)

I get a related build failure from the same commit:

Quoted 7 lines
Quoted 2 lines

arch/arm/boot/compressed/misc.c:157:6: error: no previous prototype for '__fortify_panic' [-Werror=missing-prototypes]
157 | void __fortify_panic(const u8 reason, size_t avail, size_t size)

Whoops, thank you! This series got a refresh after you did the missing-prototypes work and I missed this one. I've fixed it and double-checked for any others. Hopefully this will be clean in the next -next. :)

Thank you! (FWIW, I also hit it today)

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#24 lore raw

Re: [PATCH 6/9] fortify: Split reporting and avoid passing string pointer

In reply to Kees Cook
Quoted 3 lines · Kees Cook
include/linux/fortify-string.h +2 -0
+void __fortify_report(u8 reason);
+void __fortify_panic(u8 reason) __cold __noreturn;

(snip)

Quoted 1 line

+void __fortify_report(u8 reason)

(snip)

Quoted 1 line

+void __fortify_panic(const u8 reason)

I am curious: for some reason (no pun intended :) the reasons above are not const except this one, but then in a later patch they become const (including the declarations).

So perhaps make everything const when they are introduced? Or is there some other reason? (e.g. I saw one patch that moved a function, so there it seemed to make sense to keep things as they are to make the copy 1:1).

Cheers,
Miguel

author
#25 lore raw

Re: [PATCH 6/9] fortify: Split reporting and avoid passing string pointer

Quoted 13 lines · Miguel Ojeda
Quoted 3 lines · Kees Cook
include/linux/fortify-string.h +2 -0
+void __fortify_report(u8 reason);
+void __fortify_panic(u8 reason) __cold __noreturn;

(snip)

Quoted 1 line

+void __fortify_report(u8 reason)

(snip)

Quoted 1 line

+void __fortify_panic(const u8 reason)

Quoted 8 lines

I am curious: for some reason (no pun intended :) the reasons above are not const except this one, but then in a later patch they become const (including the declarations).

So perhaps make everything const when they are introduced? Or is there some other reason? (e.g. I saw one patch that moved a function, so there it seemed to make sense to keep things as they are to make the copy 1:1).

I will adjust it -- this was an artifact of splitting up my patches.

Thanks!

Signature · 1 line
Kees Cook
#26 lore raw

Re: [PATCH 6/9] fortify: Split reporting and avoid passing string pointer

In reply to Kees Cook

From: Kees Cook <[email protected]>
Date: Wed, 5 Apr 2023 17:02:05 -0700

Quoted 47 lines

In preparation for KUnit testing and further improvements in fortify failure reporting, split out the report and encode the function and access failure (read or write overflow) into a single int argument. This mainly ends up saving some space in the data segment. For a defconfig with FORTIFY_SOURCE enabled:

$ size gcc/vmlinux.before gcc/vmlinux.after
   text  	  data     bss     dec    	    hex filename
26132309        9760658 2195460 38088427        2452eeb gcc/vmlinux.before
26132386        9748382 2195460 38076228        244ff44 gcc/vmlinux.after
Cc Andy Shevchenko <[email protected]>
Cc Cezary Rojewski <[email protected]>
Cc Puyou Lu <[email protected]>
Cc Mark Brown <[email protected]>
---
 include/linux/fortify-string.h | 72 +++++++++++++++++++++++-----------
 lib/string_helpers.c           | 70 +++++++++++++++++++++++++++++++--
 tools/objtool/check.c          |  2 +-
 3 files changed, 118 insertions(+), 26 deletions(-)
include/linux/fortify-string.h +15 -1
index 41dbd641f55c..6db4052db459 100644
@@ -9,7 +9,34 @@
 #define __FORTIFY_INLINE extern __always_inline __gnu_inline __overloadable
 #define __RENAME(x) __asm__(#x)
 
-void fortify_panic(const char *name) __noreturn __cold;
+#define fortify_reason(func, write)	(((func) << 1) | !!(write))
+
+#define fortify_panic(func, write)	\
+	__fortify_panic(fortify_reason(func, write))
+
+#define FORTIFY_READ		 0
+#define FORTIFY_WRITE		 1
+
+#define FORTIFY_FUNC_strncpy	 0
+#define FORTIFY_FUNC_strnlen	 1
+#define FORTIFY_FUNC_strlen	 2
+#define FORTIFY_FUNC_strlcpy	 3
+#define FORTIFY_FUNC_strscpy	 4
+#define FORTIFY_FUNC_strlcat	 5
+#define FORTIFY_FUNC_strcat	 6

include/linux/fortify-string.h line 36

Quoted 10 lines
include/linux/fortify-string.h +10 -0
+#define FORTIFY_FUNC_strncat	 7
+#define FORTIFY_FUNC_memset	 8
+#define FORTIFY_FUNC_memcpy	 9
+#define FORTIFY_FUNC_memmove	10
+#define FORTIFY_FUNC_memscan	11
+#define FORTIFY_FUNC_memcmp	12
+#define FORTIFY_FUNC_memchr	13
+#define FORTIFY_FUNC_memchr_inv	14
+#define FORTIFY_FUNC_kmemdup	15
+#define FORTIFY_FUNC_strcpy	16

enum?

Quoted 4 lines
lib/string_helpers.c +0 -0
@@ -1021,10 +1021,74 @@ EXPORT_SYMBOL(__read_overflow2_field);
 void __write_overflow_field(size_t avail, size_t wanted) { }

lib/string_helpers.c EXPORT_SYMBOL(__read_overflow2_field); · line 1029

Quoted 10 lines
lib/string_helpers.c +5 -2
 EXPORT_SYMBOL(__write_overflow_field);
 
-void fortify_panic(const char *name)
+void __fortify_report(u8 reason)
 {
-	pr_emerg("detected buffer overflow in %s\n", name);
+	const char *name;
+	const bool write = !!(reason & 0x1);
+
+	switch (reason >> 1) {

As already mentioned, I'd use bitfield helpers + couple definitions to not miss something when changing the way it's encoded

#define FORTIFY_REASON_DIR(r) FIELD_GET(BIT(0), r)
#define FORTIFY_REASON_FUNC(r) FIELD_GET(GENMASK(7, 1), r)

(+ set pair)

Quoted 44 lines
lib/string_helpers.c +44 -0
+	case FORTIFY_FUNC_strncpy:
+		name = "strncpy";
+		break;
+	case FORTIFY_FUNC_strnlen:
+		name = "strnlen";
+		break;
+	case FORTIFY_FUNC_strlen:
+		name = "strlen";
+		break;
+	case FORTIFY_FUNC_strlcpy:
+		name = "strlcpy";
+		break;
+	case FORTIFY_FUNC_strscpy:
+		name = "strscpy";
+		break;
+	case FORTIFY_FUNC_strlcat:
+		name = "strlcat";
+		break;
+	case FORTIFY_FUNC_strcat:
+		name = "strcat";
+		break;
+	case FORTIFY_FUNC_strncat:
+		name = "strncat";
+		break;
+	case FORTIFY_FUNC_memset:
+		name = "memset";
+		break;
+	case FORTIFY_FUNC_memcpy:
+		name = "memcpy";
+		break;
+	case FORTIFY_FUNC_memmove:
+		name = "memmove";
+		break;
+	case FORTIFY_FUNC_memscan:
+		name = "memscan";
+		break;
+	case FORTIFY_FUNC_memcmp:
+		name = "memcmp";
+		break;
+	case FORTIFY_FUNC_memchr:
+		name = "memchr";
+		break;
+	case FORTIFY_FUNC_memchr_inv:
+		name = "memchr_inv";

lib/string_helpers.c EXPORT_SYMBOL(__read_overflow2_field); · line 1083

Quoted 10 lines
lib/string_helpers.c +10 -0
+		break;
+	case FORTIFY_FUNC_kmemdup:
+		name = "kmemdup";
+		break;
+	case FORTIFY_FUNC_strcpy:
+		name = "strcpy";
+		break;
+	default:
+		name = "unknown";
+	}

I know this is far from hotpath, but could we save some object code and do that via O(1) array lookup? Plus some macro to compress things:

#define FORTIFY_ENTRY(name)		\
	[FORTIFY_FUNC_##name]	= #name
static const char * const fortify_funcs[] = {
	FORTIFY_ENTRY(strncpy),
	...
}
	// array bounds check here if you wish, I wouldn't bother as
	// we're panicking anyway

	name = fortify_funcs[reason >> 1];

lib/string_helpers.c EXPORT_SYMBOL(__read_overflow2_field); · line 1094

Quoted 12 lines
lib/string_helpers.c +8 -1
+	WARN(1, "%s: detected buffer %s overflow\n", name, write ? "write" : "read");
+}
+EXPORT_SYMBOL(__fortify_report);
+
+void __fortify_panic(const u8 reason)
+{
+	__fortify_report(reason);
 	BUG();
 }
-EXPORT_SYMBOL(fortify_panic);
+EXPORT_SYMBOL(__fortify_panic);
 #endif /* CONFIG_FORTIFY_SOURCE */

Thanks,
Olek

author
#27 lore raw

Re: [PATCH 6/9] fortify: Split reporting and avoid passing string pointer

Quoted 54 lines · Alexander Lobakin

From: Kees Cook <[email protected]>
Date: Wed, 5 Apr 2023 17:02:05 -0700

Quoted 51 lines

In preparation for KUnit testing and further improvements in fortify failure reporting, split out the report and encode the function and access failure (read or write overflow) into a single int argument. This mainly ends up saving some space in the data segment. For a defconfig with FORTIFY_SOURCE enabled:

$ size gcc/vmlinux.before gcc/vmlinux.after
   text  	  data     bss     dec    	    hex filename
26132309        9760658 2195460 38088427        2452eeb gcc/vmlinux.before
26132386        9748382 2195460 38076228        244ff44 gcc/vmlinux.after
Cc Andy Shevchenko <[email protected]>
Cc Cezary Rojewski <[email protected]>
Cc Puyou Lu <[email protected]>
Cc Mark Brown <[email protected]>
---
 include/linux/fortify-string.h | 72 +++++++++++++++++++++++-----------
 lib/string_helpers.c           | 70 +++++++++++++++++++++++++++++++--
 tools/objtool/check.c          |  2 +-
 3 files changed, 118 insertions(+), 26 deletions(-)
include/linux/fortify-string.h +19 -1
index 41dbd641f55c..6db4052db459 100644
@@ -9,7 +9,34 @@
 #define __FORTIFY_INLINE extern __always_inline __gnu_inline __overloadable
 #define __RENAME(x) __asm__(#x)
 
-void fortify_panic(const char *name) __noreturn __cold;
+#define fortify_reason(func, write)	(((func) << 1) | !!(write))
+
+#define fortify_panic(func, write)	\
+	__fortify_panic(fortify_reason(func, write))
+
+#define FORTIFY_READ		 0
+#define FORTIFY_WRITE		 1
+
+#define FORTIFY_FUNC_strncpy	 0
+#define FORTIFY_FUNC_strnlen	 1
+#define FORTIFY_FUNC_strlen	 2
+#define FORTIFY_FUNC_strlcpy	 3
+#define FORTIFY_FUNC_strscpy	 4
+#define FORTIFY_FUNC_strlcat	 5
+#define FORTIFY_FUNC_strcat	 6
+#define FORTIFY_FUNC_strncat	 7
+#define FORTIFY_FUNC_memset	 8
+#define FORTIFY_FUNC_memcpy	 9
+#define FORTIFY_FUNC_memmove	10
Quoted 8 lines

include/linux/fortify-string.h line 36

Quoted 6 lines
include/linux/fortify-string.h +6 -0
+#define FORTIFY_FUNC_memscan	11
+#define FORTIFY_FUNC_memcmp	12
+#define FORTIFY_FUNC_memchr	13
+#define FORTIFY_FUNC_memchr_inv	14
+#define FORTIFY_FUNC_kmemdup	15
+#define FORTIFY_FUNC_strcpy	16

enum?

I opted to avoid an enum due to the binary operations used in "fortify_reason" to collapse it to a u8. It just seemed like more work to put in enums, and then do u8 casts, etc, all for a strictly "internal" set of magic numbers.

Quoted 19 lines
Quoted 5 lines
lib/string_helpers.c +0 -0
@@ -1021,10 +1021,74 @@ EXPORT_SYMBOL(__read_overflow2_field);
 void __write_overflow_field(size_t avail, size_t wanted) { }

lib/string_helpers.c EXPORT_SYMBOL(__read_overflow2_field); · line 1029

Quoted 10 lines
lib/string_helpers.c +5 -2
 EXPORT_SYMBOL(__write_overflow_field);
 
-void fortify_panic(const char *name)
+void __fortify_report(u8 reason)
 {
-	pr_emerg("detected buffer overflow in %s\n", name);
+	const char *name;
+	const bool write = !!(reason & 0x1);
+
+	switch (reason >> 1) {

As already mentioned, I'd use bitfield helpers + couple definitions to not miss something when changing the way it's encoded

Quoted 2 lines

#define FORTIFY_REASON_DIR(r) FIELD_GET(BIT(0), r)
#define FORTIFY_REASON_FUNC(r) FIELD_GET(GENMASK(7, 1), r)

Yeah, good idea. Thanks for the FIELD_GET examples!

Quoted 17 lines

[...]

lib/string_helpers.c EXPORT_SYMBOL(__read_overflow2_field); · line 1083

Quoted 4 lines
lib/string_helpers.c +4 -0
+		break;
+	default:
+		name = "unknown";
+	}

I know this is far from hotpath, but could we save some object code and do that via O(1) array lookup? Plus some macro to compress things:

#define FORTIFY_ENTRY(name)		\
	[FORTIFY_FUNC_##name]	= #name
static const char * const fortify_funcs[] = {
	FORTIFY_ENTRY(strncpy),
	...
}
Quoted 4 lines
	// array bounds check here if you wish, I wouldn't bother as
	// we're panicking anyway

	name = fortify_funcs[reason >> 1];

Fair enough. I had considered it and then forgot about it for some reason. :)

Signature · 1 line
Kees Cook
#28 lore raw

Re: [PATCH 6/9] fortify: Split reporting and avoid passing string pointer

In reply to Kees Cook
kernel test robot noticed the following build warnings:

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-20230406] [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-6-keescook%40chromium.org
patch subject: [PATCH 6/9] fortify: Split reporting and avoid passing string pointer
config: arm64-randconfig-s053-20230406 (https://download.01.org/0day-ci/archive/20230407/[email protected]/config)
compiler: aarch64-linux-gcc (GCC) 12.1.0

reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/4fd520e6ee549e1ffe8859e26e57ea64b48e78ea
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 4fd520e6ee549e1ffe8859e26e57ea64b48e78ea
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=arm64 olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=arm64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/

sparse warnings: (new ones prefixed by >>)
lib/string_helpers.c:1030:14: sparse: sparse: undefined identifier 'FORTIFY_FUNC_strncpy' lib/string_helpers.c:1033:14: sparse: sparse: undefined identifier 'FORTIFY_FUNC_strnlen' lib/string_helpers.c:1036:14: sparse: sparse: undefined identifier 'FORTIFY_FUNC_strlen' lib/string_helpers.c:1039:14: sparse: sparse: undefined identifier 'FORTIFY_FUNC_strlcpy' lib/string_helpers.c:1042:14: sparse: sparse: undefined identifier 'FORTIFY_FUNC_strscpy' lib/string_helpers.c:1045:14: sparse: sparse: undefined identifier 'FORTIFY_FUNC_strlcat' lib/string_helpers.c:1048:14: sparse: sparse: undefined identifier 'FORTIFY_FUNC_strcat' lib/string_helpers.c:1051:14: sparse: sparse: undefined identifier 'FORTIFY_FUNC_strncat' lib/string_helpers.c:1054:14: sparse: sparse: undefined identifier 'FORTIFY_FUNC_memset' lib/string_helpers.c:1057:14: sparse: sparse: undefined identifier 'FORTIFY_FUNC_memcpy' lib/string_helpers.c:1060:14: sparse: sparse: undefined identifier 'FORTIFY_FUNC_memmove' lib/string_helpers.c:1063:14: sparse: sparse: undefined identifier 'FORTIFY_FUNC_memscan' lib/string_helpers.c:1066:14: sparse: sparse: undefined identifier 'FORTIFY_FUNC_memcmp' lib/string_helpers.c:1069:14: sparse: sparse: undefined identifier 'FORTIFY_FUNC_memchr' lib/string_helpers.c:1072:14: sparse: sparse: undefined identifier 'FORTIFY_FUNC_memchr_inv' lib/string_helpers.c:1075:14: sparse: sparse: undefined identifier 'FORTIFY_FUNC_kmemdup' lib/string_helpers.c:1078:14: sparse: sparse: undefined identifier 'FORTIFY_FUNC_strcpy'

>> lib/string_helpers.c:1030:14: sparse: sparse: incompatible types for 'case' statement

lib/string_helpers.c:1033:14: sparse: sparse: incompatible types for 'case' statement lib/string_helpers.c:1036:14: sparse: sparse: incompatible types for 'case' statement lib/string_helpers.c:1039:14: sparse: sparse: incompatible types for 'case' statement lib/string_helpers.c:1042:14: sparse: sparse: incompatible types for 'case' statement lib/string_helpers.c:1045:14: sparse: sparse: incompatible types for 'case' statement lib/string_helpers.c:1048:14: sparse: sparse: incompatible types for 'case' statement lib/string_helpers.c:1051:14: sparse: sparse: incompatible types for 'case' statement lib/string_helpers.c:1054:14: sparse: sparse: incompatible types for 'case' statement lib/string_helpers.c:1057:14: sparse: sparse: incompatible types for 'case' statement lib/string_helpers.c:1060:14: sparse: sparse: incompatible types for 'case' statement lib/string_helpers.c:1063:14: sparse: sparse: incompatible types for 'case' statement lib/string_helpers.c:1066:14: sparse: sparse: incompatible types for 'case' statement lib/string_helpers.c:1069:14: sparse: sparse: incompatible types for 'case' statement lib/string_helpers.c:1072:14: sparse: sparse: incompatible types for 'case' statement lib/string_helpers.c:1075:14: sparse: sparse: incompatible types for 'case' statement lib/string_helpers.c:1078:14: sparse: sparse: incompatible types for 'case' statement lib/string_helpers.c:1030:14: sparse: sparse: Expected constant expression in case statement lib/string_helpers.c:1033:14: sparse: sparse: Expected constant expression in case statement lib/string_helpers.c:1036:14: sparse: sparse: Expected constant expression in case statement lib/string_helpers.c:1039:14: sparse: sparse: Expected constant expression in case statement lib/string_helpers.c:1042:14: sparse: sparse: Expected constant expression in case statement lib/string_helpers.c:1045:14: sparse: sparse: Expected constant expression in case statement lib/string_helpers.c:1048:14: sparse: sparse: Expected constant expression in case statement lib/string_helpers.c:1051:14: sparse: sparse: Expected constant expression in case statement lib/string_helpers.c:1054:14: sparse: sparse: Expected constant expression in case statement lib/string_helpers.c:1057:14: sparse: sparse: Expected constant expression in case statement lib/string_helpers.c:1060:14: sparse: sparse: Expected constant expression in case statement lib/string_helpers.c:1063:14: sparse: sparse: Expected constant expression in case statement lib/string_helpers.c:1066:14: sparse: sparse: Expected constant expression in case statement lib/string_helpers.c:1069:14: sparse: sparse: Expected constant expression in case statement lib/string_helpers.c:1072:14: sparse: sparse: Expected constant expression in case statement lib/string_helpers.c:1075:14: sparse: sparse: Expected constant expression in case statement lib/string_helpers.c:1078:14: sparse: sparse: Expected constant expression in case statement

vim +/case +1030 lib/string_helpers.c

1023
1024 void __fortify_report(u8 reason)
1025 {
1026 const char *name;
1027 const bool write = !!(reason & 0x1);
1028
1029 switch (reason >> 1) {

> 1030		case FORTIFY_FUNC_strncpy:
  1031			name = "strncpy";
  1032			break;
  1033		case FORTIFY_FUNC_strnlen:
  1034			name = "strnlen";
  1035			break;
  1036		case FORTIFY_FUNC_strlen:
  1037			name = "strlen";
  1038			break;
  1039		case FORTIFY_FUNC_strlcpy:
  1040			name = "strlcpy";
  1041			break;
  1042		case FORTIFY_FUNC_strscpy:
  1043			name = "strscpy";
  1044			break;
  1045		case FORTIFY_FUNC_strlcat:
  1046			name = "strlcat";
  1047			break;
  1048		case FORTIFY_FUNC_strcat:
  1049			name = "strcat";
  1050			break;
  1051		case FORTIFY_FUNC_strncat:
  1052			name = "strncat";
  1053			break;
> 1054		case FORTIFY_FUNC_memset:

1055 name = "memset";
1056 break;
1057 case FORTIFY_FUNC_memcpy:
1058 name = "memcpy";
1059 break;
1060 case FORTIFY_FUNC_memmove:
1061 name = "memmove";
1062 break;
1063 case FORTIFY_FUNC_memscan:
1064 name = "memscan";
1065 break;
1066 case FORTIFY_FUNC_memcmp:
1067 name = "memcmp";
1068 break;
1069 case FORTIFY_FUNC_memchr:
1070 name = "memchr";
1071 break;
1072 case FORTIFY_FUNC_memchr_inv:
1073 name = "memchr_inv";
1074 break;
1075 case FORTIFY_FUNC_kmemdup:
1076 name = "kmemdup";
1077 break;
1078 case FORTIFY_FUNC_strcpy:
1079 name = "strcpy";
1080 break;
1081 default:
1082 name = "unknown";
1083 }
1084 WARN(1, "%s: detected buffer %s overflow\n", name, write ? "write" : "read");
1085 }
1086 EXPORT_SYMBOL(__fortify_report);
1087

Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
author
#29 lore raw

PATCH 7/9 fortify: Provide KUnit counters for failure testing

In reply to Kees Cook

From: Kees Cook <[email protected]>

The standard C string APIs were not designed to have a failure mode; they were expected to always succeed without memory safety issues. Normally, CONFIG_FORTIFY_SOURCE will use fortify_panic() to stop processing, as truncating a read or write may provide an even worse system state. However, this creates a problem for testing under things like KUnit, which needs a way to survive failures.

When building with CONFIG_KUNIT, provide a failure path for all users for fortify_panic, and track whether the failure was a read overflow or a write overflow, for KUnit tests to examine. Inspired by similar logic in the slab tests.

---
 include/linux/fortify-string.h | 45 +++++++++++++++++---------------
 lib/fortify_kunit.c            | 47 ++++++++++++++++++++++++++++++++++
 lib/string_helpers.c           |  3 +++
 3 files changed, 75 insertions(+), 20 deletions(-)
include/linux/fortify-string.h +25 -20
index 6db4052db459..2bbee7b28e71 100644
@@ -11,8 +11,12 @@
 
 #define fortify_reason(func, write)	(((func) << 1) | !!(write))
 
-#define fortify_panic(func, write)	\
+#ifdef FORTIFY_KUNIT_OVERRIDE
+# define fortify_panic kunit_fortify_panic
+#else
+# define fortify_panic(func, write, retfail)	\
 	__fortify_panic(fortify_reason(func, write))
+#endif
 
 #define FORTIFY_READ		 0
 #define FORTIFY_WRITE		 1
@@ -174,7 +178,7 @@ char *strncpy(char * const POS p, const char *q, __kernel_size_t size)
 	if (__compiletime_lessthan(p_size, size))
 		__write_overflow();
 	if (p_size < size)
-		fortify_panic(FORTIFY_FUNC_strncpy, FORTIFY_WRITE);
+		fortify_panic(FORTIFY_FUNC_strncpy, FORTIFY_WRITE, p);
 	return __underlying_strncpy(p, q, size);
 }
 
@@ -205,7 +209,7 @@ __FORTIFY_INLINE __kernel_size_t strnlen(const char * const POS p, __kernel_size
 	/* Do not check characters beyond the end of p. */
 	ret = __real_strnlen(p, maxlen < p_size ? maxlen : p_size);
 	if (p_size <= ret && maxlen != ret)
-		fortify_panic(FORTIFY_FUNC_strnlen, FORTIFY_READ);
+		fortify_panic(FORTIFY_FUNC_strnlen, FORTIFY_READ, ret);
 	return ret;
 }
 
@@ -241,7 +245,7 @@ __kernel_size_t __fortify_strlen(const char * const POS p)
 		return __underlying_strlen(p);
 	ret = strnlen(p, p_size);
 	if (p_size <= ret)
-		fortify_panic(FORTIFY_FUNC_strlen, FORTIFY_READ);
+		fortify_panic(FORTIFY_FUNC_strlen, FORTIFY_READ, ret);
 	return ret;
 }
 
@@ -283,7 +287,7 @@ __FORTIFY_INLINE size_t strlcpy(char * const POS p, const char * const POS q, si
 	}
 	if (size) {
 		if (len >= p_size)
-			fortify_panic(FORTIFY_FUNC_strlcpy, FORTIFY_WRITE);
+			fortify_panic(FORTIFY_FUNC_strlcpy, FORTIFY_WRITE, q_len);
 		__underlying_memcpy(p, q, len);
 		p[len] = '\0';
 	}
@@ -361,7 +365,7 @@ __FORTIFY_INLINE ssize_t strscpy(char * const POS p, const char * const POS q, s
 	 * p_size.
 	 */
 	if (len > p_size)
-		fortify_panic(FORTIFY_FUNC_strscpy, FORTIFY_WRITE);
+		fortify_panic(FORTIFY_FUNC_strscpy, FORTIFY_WRITE, -E2BIG);
 
 	/*
 	 * We can now safely call vanilla strscpy because we are protected from:
@@ -419,7 +423,7 @@ size_t strlcat(char * const POS p, const char * const POS q, size_t avail)
 
 	/* Give up if string is already overflowed. */
 	if (p_size <= p_len)
-		fortify_panic(FORTIFY_FUNC_strlcat, FORTIFY_READ);
+		fortify_panic(FORTIFY_FUNC_strlcat, FORTIFY_READ, wanted);
 
 	if (actual >= avail) {
 		copy_len = avail - p_len - 1;
@@ -428,7 +432,7 @@ size_t strlcat(char * const POS p, const char * const POS q, size_t avail)
 
 	/* Give up if copy will overflow. */
 	if (p_size <= actual)
-		fortify_panic(FORTIFY_FUNC_strlcat, FORTIFY_WRITE);
+		fortify_panic(FORTIFY_FUNC_strlcat, FORTIFY_WRITE, wanted);
 	__underlying_memcpy(p + p_len, q, copy_len);
 	p[actual] = '\0';
 
@@ -457,7 +461,7 @@ char *strcat(char * const POS p, const char *q)
 	size_t p_size = __member_size(p);
 
 	if (strlcat(p, q, p_size) >= p_size)
-		fortify_panic(FORTIFY_FUNC_strcat, FORTIFY_WRITE);
+		fortify_panic(FORTIFY_FUNC_strcat, FORTIFY_WRITE, p);
 	return p;
 }
 
@@ -493,13 +497,13 @@ char *strncat(char * const POS p, const char * const POS q, __kernel_size_t coun
 	p_len = strlen(p);
 	copy_len = strnlen(q, count);
 	if (p_size < p_len + copy_len + 1)
-		fortify_panic(FORTIFY_FUNC_strncat, FORTIFY_WRITE);
+		fortify_panic(FORTIFY_FUNC_strncat, FORTIFY_WRITE, p);
 	__underlying_memcpy(p + p_len, q, copy_len);
 	p[p_len + copy_len] = '\0';
 	return p;
 }
 
-__FORTIFY_INLINE void fortify_memset_chk(__kernel_size_t size,
+__FORTIFY_INLINE bool fortify_memset_chk(__kernel_size_t size,
 					 const size_t p_size,
 					 const size_t p_size_field)
 {
@@ -534,7 +538,8 @@ __FORTIFY_INLINE void fortify_memset_chk(__kernel_size_t size,
 	 * lengths are unknown.)
 	 */
 	if (p_size != SIZE_MAX && p_size < size)
-		fortify_panic(FORTIFY_FUNC_memset, FORTIFY_WRITE);
+		fortify_panic(FORTIFY_FUNC_memset, FORTIFY_WRITE, true);
+	return false;
 }
 
 #define __fortify_memset_chk(p, c, size, p_size, p_size_field) ({	\
@@ -633,9 +638,9 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,
 	 * lengths are unknown.)
 	 */
 	if (p_size != SIZE_MAX && p_size < size)
-		fortify_panic(func, FORTIFY_WRITE);
+		fortify_panic(func, FORTIFY_WRITE, true);
 	else if (q_size != SIZE_MAX && q_size < size)
-		fortify_panic(func, FORTIFY_READ);
+		fortify_panic(func, FORTIFY_READ, true);
 
 	/*
 	 * Warn when writing beyond destination field size.
@@ -735,7 +740,7 @@ __FORTIFY_INLINE void *memscan(void * const POS0 p, int c, __kernel_size_t size)
 	if (__compiletime_lessthan(p_size, size))
 		__read_overflow();
 	if (p_size < size)
-		fortify_panic(FORTIFY_FUNC_memscan, FORTIFY_READ);
+		fortify_panic(FORTIFY_FUNC_memscan, FORTIFY_READ, NULL);
 	return __real_memscan(p, c, size);
 }
 
@@ -752,7 +757,7 @@ int memcmp(const void * const POS0 p, const void * const POS0 q, __kernel_size_t
 			__read_overflow2();
 	}
 	if (p_size < size || q_size < size)
-		fortify_panic(FORTIFY_FUNC_memcmp, FORTIFY_READ);
+		fortify_panic(FORTIFY_FUNC_memcmp, FORTIFY_READ, INT_MIN);
 	return __underlying_memcmp(p, q, size);
 }
 
@@ -764,7 +769,7 @@ void *memchr(const void * const POS0 p, int c, __kernel_size_t size)
 	if (__compiletime_lessthan(p_size, size))
 		__read_overflow();
 	if (p_size < size)
-		fortify_panic(FORTIFY_FUNC_memchr, FORTIFY_READ);
+		fortify_panic(FORTIFY_FUNC_memchr, FORTIFY_READ, NULL);
 	return __underlying_memchr(p, c, size);
 }
 
@@ -776,7 +781,7 @@ __FORTIFY_INLINE void *memchr_inv(const void * const POS0 p, int c, size_t size)
 	if (__compiletime_lessthan(p_size, size))
 		__read_overflow();
 	if (p_size < size)
-		fortify_panic(FORTIFY_FUNC_memchr_inv, FORTIFY_READ);
+		fortify_panic(FORTIFY_FUNC_memchr_inv, FORTIFY_READ, NULL);
 	return __real_memchr_inv(p, c, size);
 }
 
@@ -789,7 +794,7 @@ __FORTIFY_INLINE void *kmemdup(const void * const POS0 p, size_t size, gfp_t gfp
 	if (__compiletime_lessthan(p_size, size))
 		__read_overflow();
 	if (p_size < size)
-		fortify_panic(FORTIFY_FUNC_kmemdup, FORTIFY_READ);
+		fortify_panic(FORTIFY_FUNC_kmemdup, FORTIFY_READ, NULL);
 	return __real_kmemdup(p, size, gfp);
 }
 
@@ -826,7 +831,7 @@ char *strcpy(char * const POS p, const char * const POS q)
 		__write_overflow();
 	/* Run-time check for dynamic size overflow. */
 	if (p_size < size)
-		fortify_panic(FORTIFY_FUNC_strcpy, FORTIFY_WRITE);
+		fortify_panic(FORTIFY_FUNC_strcpy, FORTIFY_WRITE, p);
 	__underlying_memcpy(p, q, size);
 	return p;
 }
lib/fortify_kunit.c +47 -0

77 lines not on this page · raw message

lib/string_helpers.c +3 -0
index 631c50657096..5bb65f623e40 100644
@@ -18,6 +18,8 @@
 #include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/string_helpers.h>
+#include <kunit/test.h>
+#include <kunit/test-bug.h>
 
 /**
  * string_get_size - get the size in the specified units
@@ -1091,4 +1093,5 @@ void __fortify_panic(const u8 reason)
 	BUG();
 }
 EXPORT_SYMBOL(__fortify_panic);
+
 #endif /* CONFIG_FORTIFY_SOURCE */
Signature · 1 line
2.34.1
author
#30 lore raw

PATCH 8/9 fortify: Add KUnit tests for runtime overflows

In reply to Kees Cook

From: Kees Cook <[email protected]>

With fortify overflows able to be redirected, we can use KUnit to exercise the overflow conditions. Add tests for every API covered by CONFIG_FORTIFY_SOURCE.

---
 lib/fortify_kunit.c | 733 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 733 insertions(+)
lib/fortify_kunit.c +733 -0
index f7523c25d341..b7c884037629 100644
@@ -357,6 +357,723 @@ DEFINE_ALLOC_SIZE_TEST_PAIR(kvmalloc)
 } while (0)
 DEFINE_ALLOC_SIZE_TEST_PAIR(devm_kmalloc)
 
+/*
+ * We can't have an array at the end of a structure or else
+ * builds without -fstrict-flex-arrays=3 will report them as
+ * being an unknown length. Additionally, add bytes before
+ * and after the string to catch over/underflows if tests
+ * fail.
+ */
+struct fortify_padding {
+	unsigned long bytes_before;
+	char buf[32];
+	unsigned long bytes_after;
+};
+/* Force compiler into not being able to resolve size at compile-time. */
+static volatile int unconst = 0;
+
+static void strlen_test(struct kunit *test)
+{
+	struct fortify_padding pad = { };
+	int i, end = sizeof(pad.buf) - 1;
+
+	skip_without_fortify();
+
+	fortify_read_overflows = 0;
+
+	/* Fill 31 bytes with valid characters. */
+	for (i = 0; i < sizeof(pad.buf) - 1; i++)
+		pad.buf[i] = i + '0';
+	/* Trailing bytes are still %NUL. */
+	KUNIT_EXPECT_EQ(test, pad.buf[end], '\0');
+	KUNIT_EXPECT_EQ(test, pad.bytes_after, 0);
+
+	/* String is terminated, so strlen() is valid. */
+	KUNIT_EXPECT_EQ(test, strlen(pad.buf), end);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0);
+
+	/* Make string unterminated, and recount. */
+	pad.buf[end] = 'A';
+	end = sizeof(pad.buf);
+	KUNIT_EXPECT_EQ(test, strlen(pad.buf), end);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 1);
+}
+
+static void strnlen_test(struct kunit *test)
+{
+	struct fortify_padding pad = { };
+	int i, end = sizeof(pad.buf) - 1;
+
+	skip_without_fortify();
+
+	fortify_read_overflows = 0;
+
+	/* Fill 31 bytes with valid characters. */
+	for (i = 0; i < sizeof(pad.buf) - 1; i++)
+		pad.buf[i] = i + '0';
+	/* Trailing bytes are still %NUL. */
+	KUNIT_EXPECT_EQ(test, pad.buf[end], '\0');
+	KUNIT_EXPECT_EQ(test, pad.bytes_after, 0);
+
+	/* String is terminated, so strnlen() is valid. */
+	KUNIT_EXPECT_EQ(test, strnlen(pad.buf, sizeof(pad.buf)), end);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0);
+	/* A truncated strnlen() will be safe, too. */
+	KUNIT_EXPECT_EQ(test, strnlen(pad.buf, sizeof(pad.buf) / 2),
+					sizeof(pad.buf) / 2);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0);
+
+	/* Make string unterminated, and recount. */
+	pad.buf[end] = 'A';
+	end = sizeof(pad.buf);
+	/* Reading beyond with strncpy() will fail. */
+	KUNIT_EXPECT_EQ(test, strnlen(pad.buf, end + 1), end);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 1);
+	KUNIT_EXPECT_EQ(test, strnlen(pad.buf, end + 2), end);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 2);
+
+	/* Early-truncated is safe still, though. */
+	KUNIT_EXPECT_EQ(test, strnlen(pad.buf, end), end);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 2);
+
+	end = sizeof(pad.buf) / 2;
+	KUNIT_EXPECT_EQ(test, strnlen(pad.buf, end), end);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 2);
+}
+
+static void strcpy_test(struct kunit *test)
+{
+	struct fortify_padding pad = { };
+	char src[sizeof(pad.buf) + 1] = { };
+	int i;
+
+	skip_without_fortify();
+
+	/* Fill 31 bytes with valid characters. */
+	for (i = 0; i < sizeof(src) - 2; i++)
+		src[i] = i + '0';
+
+	fortify_read_overflows = 0;
+	fortify_write_overflows = 0;
+
+	/* 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.bytes_after, 0);
+
+	/* Legitimate strcpy() 1 less than of max size. */
+	KUNIT_ASSERT_TRUE(test, strcpy(pad.buf, src)
+				== pad.buf);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0);
+	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');
+
+	src[sizeof(src) - 2] = 'A';
+	/* But now we trip the overflow checking. */
+	KUNIT_ASSERT_TRUE(test, strcpy(pad.buf, src)
+				== pad.buf);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0);
+	KUNIT_EXPECT_EQ(test, fortify_write_overflows, 1);
+	/* Trailing %NUL -- thanks to FORTIFY. */
+	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');
+	/* And we will not have gone beyond. */
+	KUNIT_EXPECT_EQ(test, pad.bytes_after, 0);
+
+	src[sizeof(src) - 1] = 'A';
+	/* And for sure now, two bytes past. */
+	KUNIT_ASSERT_TRUE(test, strcpy(pad.buf, src)
+				== pad.buf);
+	/*
+	 * Which trips both the strlen() on the unterminated src,
+	 * and the resulting copy attempt.
+	 */
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 1);
+	KUNIT_EXPECT_EQ(test, fortify_write_overflows, 2);
+	/* Trailing %NUL -- thanks to FORTIFY. */
+	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');
+	/* And we will not have gone beyond. */
+	KUNIT_EXPECT_EQ(test, pad.bytes_after, 0);
+}
+
+static void strncpy_test(struct kunit *test)
+{
+	struct fortify_padding pad = { };
+	char src[] = "Copy me fully into a small buffer and I will overflow!";
+
+	skip_without_fortify();
+
+	fortify_write_overflows = 0;
+
+	/* 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.bytes_after, 0);
+
+	/* Legitimate strncpy() 1 less than of max size. */
+	KUNIT_ASSERT_TRUE(test, strncpy(pad.buf, src,
+					sizeof(pad.buf) + unconst - 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');
+
+	/* Legitimate (though unterminated) max-size strncpy. */
+	KUNIT_ASSERT_TRUE(test, strncpy(pad.buf, src,
+					sizeof(pad.buf) + unconst)
+				== 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');
+	/* 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)
+				== 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');
+	/* 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)
+				== 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');
+	/* And we will not have gone beyond. */
+	KUNIT_EXPECT_EQ(test, pad.bytes_after, 0);
+}
+
+static void strlcpy_test(struct kunit *test)
+{
+	struct fortify_padding pad = { };
+	char src[] = "Copy me fully into a small buffer and I will overflow!";
+	char tiny[4] = "abcd";
+
+	skip_without_fortify();
+
+	fortify_read_overflows = 0;
+	fortify_write_overflows = 0;
+
+	/* 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.bytes_after, 0);
+
+	/* Legitimate strlcpy() 1 less than of max size. */
+	KUNIT_ASSERT_EQ(test, strlcpy(pad.buf, src,
+				      sizeof(pad.buf) + unconst - 1),
+			sizeof(src) - 1);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0);
+	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');
+
+	/* Legitimate max-size strlcpy. */
+	KUNIT_ASSERT_EQ(test, strlcpy(pad.buf, src,
+				      sizeof(pad.buf) + unconst),
+			sizeof(src) - 1);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0);
+	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');
+
+	/* Now verify that FORTIFY is working... */
+	KUNIT_ASSERT_EQ(test, strlcpy(pad.buf, src,
+				      sizeof(pad.buf) + unconst + 1),
+			sizeof(src) - 1);
+	/* Should catch the overflow. */
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0);
+	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');
+	/* And we will not have gone beyond. */
+	KUNIT_EXPECT_EQ(test, pad.bytes_after, 0);
+
+	/* And much further... */
+	KUNIT_ASSERT_EQ(test, strlcpy(pad.buf, src,
+				      sizeof(src) * 2 + unconst),
+			sizeof(src) - 1);
+	/* Should catch the overflow. */
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0);
+	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');
+	/* And we will not have gone beyond. */
+	KUNIT_EXPECT_EQ(test, pad.bytes_after, 0);
+
+	/* Catch over-read. */
+	KUNIT_ASSERT_EQ(test, strlcpy(pad.buf, tiny,
+				      sizeof(pad.buf) + unconst),
+			sizeof(tiny));
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 1);
+	KUNIT_EXPECT_EQ(test, fortify_write_overflows, 2);
+}
+
+static void strscpy_test(struct kunit *test)
+{
+	struct fortify_padding pad = { };
+	char src[] = "Copy me fully into a small buffer and I will overflow!";
+
+	skip_without_fortify();
+
+	fortify_write_overflows = 0;
+
+	/* 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.bytes_after, 0);
+
+	/* Legitimate strscpy() 1 less than of max size. */
+	KUNIT_ASSERT_EQ(test, strscpy(pad.buf, src,
+				      sizeof(pad.buf) + unconst - 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');
+
+	/* Legitimate max-size strscpy. */
+	KUNIT_ASSERT_EQ(test, strscpy(pad.buf, src,
+				      sizeof(pad.buf) + unconst),
+			-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');
+
+	/* Now verify that FORTIFY is working... */
+	KUNIT_ASSERT_EQ(test, strscpy(pad.buf, src,
+				      sizeof(pad.buf) + unconst + 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');
+	/* 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),
+			-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');
+	/* And we will not have gone beyond. */
+	KUNIT_EXPECT_EQ(test, pad.bytes_after, 0);
+}
+
+static void strcat_test(struct kunit *test)
+{
+	struct fortify_padding pad = { };
+	char src[sizeof(pad.buf) / 2] = { };
+	char one[] = "A";
+	char two[] = "BC";
+	int i;
+
+	skip_without_fortify();
+
+	fortify_write_overflows = 0;
+
+	/* Fill 15 bytes with valid characters. */
+	for (i = 0; i < sizeof(src) - 1; i++)
+		src[i] = i + 'A';
+
+	/* 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.bytes_after, 0);
+
+	/* Legitimate strcat() using less than half max size. */
+	KUNIT_ASSERT_TRUE(test, strcat(pad.buf, src) == pad.buf);
+	KUNIT_EXPECT_EQ(test, fortify_write_overflows, 0);
+	/* Legitimate strcat() now 2 bytes shy of end. */
+	KUNIT_ASSERT_TRUE(test, strcat(pad.buf, src) == pad.buf);
+	KUNIT_EXPECT_EQ(test, fortify_write_overflows, 0);
+	/* 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');
+
+	/* Add one more character to the end. */
+	KUNIT_ASSERT_TRUE(test, strcat(pad.buf, one) == pad.buf);
+	KUNIT_EXPECT_EQ(test, fortify_write_overflows, 0);
+	/* 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');
+
+	/* And this one char will overflow. */
+	KUNIT_ASSERT_TRUE(test, strcat(pad.buf, one) == pad.buf);
+	KUNIT_EXPECT_EQ(test, fortify_write_overflows, 1);
+	/* Last byte should be %NUL thanks to FORTIFY. */
+	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.bytes_after, 0);
+
+	/* And adding two will overflow more. */
+	KUNIT_ASSERT_TRUE(test, strcat(pad.buf, two) == pad.buf);
+	KUNIT_EXPECT_EQ(test, fortify_write_overflows, 2);
+	/* Last byte should be %NUL thanks to FORTIFY. */
+	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.bytes_after, 0);
+}
+
+static void strncat_test(struct kunit *test)
+{
+	struct fortify_padding pad = { };
+	char src[sizeof(pad.buf)] = { };
+	int i, partial;
+
+	skip_without_fortify();
+
+	fortify_read_overflows = 0;
+	fortify_write_overflows = 0;
+
+	/* Fill 31 bytes with valid characters. */
+	partial = sizeof(src) / 2 - 1;
+	for (i = 0; i < partial; i++)
+		src[i] = i + 'A';
+
+	/* 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.bytes_after, 0);
+
+	/* Legitimate strncat() using less than half max size. */
+	KUNIT_ASSERT_TRUE(test, strncat(pad.buf, src, partial) == pad.buf);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0);
+	KUNIT_EXPECT_EQ(test, fortify_write_overflows, 0);
+	/* Legitimate strncat() now 2 bytes shy of end. */
+	KUNIT_ASSERT_TRUE(test, strncat(pad.buf, src, partial) == pad.buf);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0);
+	KUNIT_EXPECT_EQ(test, fortify_write_overflows, 0);
+	/* 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');
+
+	/* Add one more character to the end. */
+	KUNIT_ASSERT_TRUE(test, strncat(pad.buf, src, 1) == pad.buf);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0);
+	KUNIT_EXPECT_EQ(test, fortify_write_overflows, 0);
+	/* 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');
+
+	/* And this one char will overflow. */
+	KUNIT_ASSERT_TRUE(test, strncat(pad.buf, src, 1) == pad.buf);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0);
+	KUNIT_EXPECT_EQ(test, fortify_write_overflows, 1);
+	/* Last byte should be %NUL thanks to FORTIFY. */
+	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.bytes_after, 0);
+
+	/* And adding two will overflow more. */
+	KUNIT_ASSERT_TRUE(test, strncat(pad.buf, src, 2) == pad.buf);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0);
+	KUNIT_EXPECT_EQ(test, fortify_write_overflows, 2);
+	/* Last byte should be %NUL thanks to FORTIFY. */
+	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.bytes_after, 0);
+
+	/* Force an unterminated destination, and overflow. */
+	pad.buf[sizeof(pad.buf) - 1] = 'A';
+	KUNIT_ASSERT_TRUE(test, strncat(pad.buf, src, 1) == pad.buf);
+	/* This will have tripped both strlen() and strcat(). */
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 1);
+	KUNIT_EXPECT_EQ(test, fortify_write_overflows, 3);
+	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) - 3], '\0');
+	/* But we should not go beyond the end. */
+	KUNIT_EXPECT_EQ(test, pad.bytes_after, 0);
+}
+
+static void strlcat_test(struct kunit *test)
+{
+	struct fortify_padding pad = { };
+	char src[sizeof(pad.buf)] = { };
+	int i, partial;
+	int len = sizeof(pad.buf) + unconst;
+
+	skip_without_fortify();
+
+	fortify_read_overflows = 0;
+	fortify_write_overflows = 0;
+
+	/* Fill 15 bytes with valid characters. */
+	partial = sizeof(src) / 2 - 1;
+	for (i = 0; i < partial; i++)
+		src[i] = i + 'A';
+
+	/* 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.bytes_after, 0);
+
+	/* Legitimate strlcat() using less than half max size. */
+	KUNIT_ASSERT_EQ(test, strlcat(pad.buf, src, len), partial);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0);
+	KUNIT_EXPECT_EQ(test, fortify_write_overflows, 0);
+	/* Legitimate strlcat() now 2 bytes shy of end. */
+	KUNIT_ASSERT_EQ(test, strlcat(pad.buf, src, len), partial * 2);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0);
+	KUNIT_EXPECT_EQ(test, fortify_write_overflows, 0);
+	/* 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');
+
+	/* Add one more character to the end. */
+	KUNIT_ASSERT_EQ(test, strlcat(pad.buf, "Q", len), partial * 2 + 1);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0);
+	KUNIT_EXPECT_EQ(test, fortify_write_overflows, 0);
+	/* 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');
+
+	/* And this one char will overflow. */
+	KUNIT_ASSERT_EQ(test, strlcat(pad.buf, "V", len * 2), len);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0);
+	KUNIT_EXPECT_EQ(test, fortify_write_overflows, 1);
+	/* Last byte should be %NUL thanks to FORTIFY. */
+	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.bytes_after, 0);
+
+	/* And adding two will overflow more. */
+	KUNIT_ASSERT_EQ(test, strlcat(pad.buf, "QQ", len * 2), len + 1);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0);
+	KUNIT_EXPECT_EQ(test, fortify_write_overflows, 2);
+	/* Last byte should be %NUL thanks to FORTIFY. */
+	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.bytes_after, 0);
+
+	/* Force an unterminated destination, and overflow. */
+	pad.buf[sizeof(pad.buf) - 1] = 'A';
+	KUNIT_ASSERT_EQ(test, strlcat(pad.buf, "TT", len * 2), len + 2);
+	/* This will have tripped both strlen() and strlcat(). */
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 2);
+	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) - 3], '\0');
+	/* But we should not go beyond the end. */
+	KUNIT_EXPECT_EQ(test, pad.bytes_after, 0);
+
+	/* Force an unterminated source, and overflow. */
+	memset(src, 'B', sizeof(src));
+	pad.buf[sizeof(pad.buf) - 1] = '\0';
+	KUNIT_ASSERT_EQ(test, strlcat(pad.buf, src, len * 3), len - 1 + sizeof(src));
+	/* This will have tripped both strlen() and strlcat(). */
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 3);
+	KUNIT_EXPECT_EQ(test, fortify_write_overflows, 3);
+	KUNIT_EXPECT_EQ(test, pad.buf[sizeof(pad.buf) - 1], '\0');
+	/* But we should not go beyond the end. */
+	KUNIT_EXPECT_EQ(test, pad.bytes_after, 0);
+}
+
+static void memscan_test(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;
+
+	skip_without_fortify();
+
+	fortify_read_overflows = 0;
+
+	KUNIT_ASSERT_PTR_EQ(test, memscan(haystack, needle, len),
+				  mem);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0);
+	/* Catch too-large range. */
+	KUNIT_ASSERT_PTR_EQ(test, memscan(haystack, needle, len + 1),
+				  NULL);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 1);
+	KUNIT_ASSERT_PTR_EQ(test, memscan(haystack, needle, len * 2),
+				  NULL);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 2);
+}
+
+static void memchr_test(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;
+
+	skip_without_fortify();
+
+	fortify_read_overflows = 0;
+
+	KUNIT_ASSERT_PTR_EQ(test, memchr(haystack, needle, len),
+				  mem);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0);
+	/* Catch too-large range. */
+	KUNIT_ASSERT_PTR_EQ(test, memchr(haystack, needle, len + 1),
+				  NULL);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 1);
+	KUNIT_ASSERT_PTR_EQ(test, memchr(haystack, needle, len * 2),
+				  NULL);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 2);
+}
+
+static void memchr_inv_test(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;
+
+	skip_without_fortify();
+
+	fortify_read_overflows = 0;
+
+	/* Normal search is okay. */
+	KUNIT_ASSERT_PTR_EQ(test, memchr_inv(haystack, needle, len),
+				  mem);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0);
+	/* Catch too-large range. */
+	KUNIT_ASSERT_PTR_EQ(test, memchr_inv(haystack, needle, len + 1),
+				  NULL);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 1);
+	KUNIT_ASSERT_PTR_EQ(test, memchr_inv(haystack, needle, len * 2),
+				  NULL);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 2);
+}
+
+static void memcmp_test(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;
+
+	skip_without_fortify();
+
+	fortify_read_overflows = 0;
+
+	/* We match the first string (ignoring the %NUL). */
+	KUNIT_ASSERT_EQ(test, memcmp(one, two, one_len), 0);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0);
+	/* Still in bounds, but no longer matching. */
+	KUNIT_ASSERT_EQ(test, memcmp(one, two, one_len + 1), -32);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0);
+
+	/* Catch too-large ranges. */
+	KUNIT_ASSERT_EQ(test, memcmp(one, two, one_len + 2), INT_MIN);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 1);
+
+	KUNIT_ASSERT_EQ(test, memcmp(two, one, two_len + 2), INT_MIN);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 2);
+}
+
+static void kmemdup_test(struct kunit *test)
+{
+	char src[] = "I got Doom running on it!";
+	char *copy;
+	size_t len = sizeof(src) + unconst;
+
+	skip_without_fortify();
+
+	fortify_read_overflows = 0;
+
+	/* Copy is within bounds. */
+	copy = kmemdup(src, len, GFP_KERNEL);
+	KUNIT_EXPECT_NOT_NULL(test, copy);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0);
+	kfree(copy);
+
+	/* Without %NUL. */
+	copy = kmemdup(src, len - 1, GFP_KERNEL);
+	KUNIT_EXPECT_NOT_NULL(test, copy);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0);
+	kfree(copy);
+
+	/* Tiny bounds. */
+	copy = kmemdup(src, 1, GFP_KERNEL);
+	KUNIT_EXPECT_NOT_NULL(test, copy);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 0);
+	kfree(copy);
+
+	/* Out of bounds by 1 byte. */
+	copy = kmemdup(src, len + 1, GFP_KERNEL);
+	KUNIT_EXPECT_NULL(test, copy);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 1);
+	kfree(copy);
+
+	/* Way out of bounds. */
+	copy = kmemdup(src, len * 2, GFP_KERNEL);
+	KUNIT_EXPECT_NULL(test, copy);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 2);
+	kfree(copy);
+
+	/* Starting offset causing out of bounds. */
+	copy = kmemdup(src + 1, len, GFP_KERNEL);
+	KUNIT_EXPECT_NULL(test, copy);
+	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 3);
+	kfree(copy);
+}
+
 static int fortify_test_init(struct kunit *test)
 {
 	kunit_add_named_resource(test, NULL, NULL, &read_resource,
@@ -379,6 +1096,22 @@ static struct kunit_case fortify_test_cases[] = {
 	KUNIT_CASE(alloc_size_kvmalloc_dynamic_test),
 	KUNIT_CASE(alloc_size_devm_kmalloc_const_test),
 	KUNIT_CASE(alloc_size_devm_kmalloc_dynamic_test),
+	KUNIT_CASE(strlen_test),
+	KUNIT_CASE(strnlen_test),
+	KUNIT_CASE(strcpy_test),
+	KUNIT_CASE(strncpy_test),
+	KUNIT_CASE(strlcpy_test),
+	KUNIT_CASE(strscpy_test),
+	KUNIT_CASE(strcat_test),
+	KUNIT_CASE(strncat_test),
+	KUNIT_CASE(strlcat_test),
+	/* skip memset: performs bounds checking on whole structs */
+	/* skip memcpy: still using warn-and-clobber instead of hard-fail */
+	KUNIT_CASE(memscan_test),
+	KUNIT_CASE(memchr_test),
+	KUNIT_CASE(memchr_inv_test),
+	KUNIT_CASE(memcmp_test),
+	KUNIT_CASE(kmemdup_test),
 	{}
 };
 
Signature · 1 line
2.34.1
author
#31 lore raw

PATCH 9/9 fortify: Improve buffer overflow reporting

In reply to Kees Cook

From: Kees Cook <[email protected]>

Improve the reporting of buffer overflows under CONFIG_FORTIFY_SOURCE to help accelerate debugging efforts. The calculations are all just sitting in registers anyway, so pass them along to the function to be reported.

For example, before:

detected buffer overflow in memcpy

and after:

memcpy: detected buffer overflow: 4096 byte read from buffer of size 1

---
 include/linux/fortify-string.h | 60 +++++++++++++++++++---------------
 lib/fortify_kunit.c            |  4 +--
 lib/string_helpers.c           |  9 ++---
 3 files changed, 40 insertions(+), 33 deletions(-)
include/linux/fortify-string.h +33 -27
index 2bbee7b28e71..d37f4597cf68 100644
@@ -14,8 +14,8 @@
 #ifdef FORTIFY_KUNIT_OVERRIDE
 # define fortify_panic kunit_fortify_panic
 #else
-# define fortify_panic(func, write, retfail)	\
-	__fortify_panic(fortify_reason(func, write))
+# define fortify_panic(func, write, avail, size, retfail)	\
+	__fortify_panic(fortify_reason(func, write), avail, size)
 #endif
 
 #define FORTIFY_READ		 0
@@ -39,8 +39,8 @@
 #define FORTIFY_FUNC_kmemdup	15
 #define FORTIFY_FUNC_strcpy	16
 
-void __fortify_report(u8 reason);
-void __fortify_panic(u8 reason) __cold __noreturn;
+void __fortify_report(const u8 reason, const size_t avail, const size_t size);
+void __fortify_panic(const u8 reason, const size_t avail, const size_t size) __cold __noreturn;
 void __read_overflow(void) __compiletime_error("detected read beyond size of object (1st parameter)");
 void __read_overflow2(void) __compiletime_error("detected read beyond size of object (2nd parameter)");
 void __read_overflow2_field(size_t avail, size_t wanted) __compiletime_warning("detected read beyond size of field (2nd parameter); maybe use struct_group()?");
@@ -178,7 +178,7 @@ char *strncpy(char * const POS p, const char *q, __kernel_size_t size)
 	if (__compiletime_lessthan(p_size, size))
 		__write_overflow();
 	if (p_size < size)
-		fortify_panic(FORTIFY_FUNC_strncpy, FORTIFY_WRITE, p);
+		fortify_panic(FORTIFY_FUNC_strncpy, FORTIFY_WRITE, p_size, size, p);
 	return __underlying_strncpy(p, q, size);
 }
 
@@ -209,7 +209,7 @@ __FORTIFY_INLINE __kernel_size_t strnlen(const char * const POS p, __kernel_size
 	/* Do not check characters beyond the end of p. */
 	ret = __real_strnlen(p, maxlen < p_size ? maxlen : p_size);
 	if (p_size <= ret && maxlen != ret)
-		fortify_panic(FORTIFY_FUNC_strnlen, FORTIFY_READ, ret);
+		fortify_panic(FORTIFY_FUNC_strnlen, FORTIFY_READ, p_size, ret + 1, ret);
 	return ret;
 }
 
@@ -245,7 +245,7 @@ __kernel_size_t __fortify_strlen(const char * const POS p)
 		return __underlying_strlen(p);
 	ret = strnlen(p, p_size);
 	if (p_size <= ret)
-		fortify_panic(FORTIFY_FUNC_strlen, FORTIFY_READ, ret);
+		fortify_panic(FORTIFY_FUNC_strlen, FORTIFY_READ, p_size, ret + 1, ret);
 	return ret;
 }
 
@@ -286,8 +286,8 @@ __FORTIFY_INLINE size_t strlcpy(char * const POS p, const char * const POS q, si
 			__write_overflow();
 	}
 	if (size) {
-		if (len >= p_size)
-			fortify_panic(FORTIFY_FUNC_strlcpy, FORTIFY_WRITE, q_len);
+		if (p_size <= len)
+			fortify_panic(FORTIFY_FUNC_strlcpy, FORTIFY_WRITE, p_size, len + 1, q_len);
 		__underlying_memcpy(p, q, len);
 		p[len] = '\0';
 	}
@@ -364,8 +364,8 @@ __FORTIFY_INLINE ssize_t strscpy(char * const POS p, const char * const POS q, s
 	 * Generate a runtime write overflow error if len is greater than
 	 * p_size.
 	 */
-	if (len > p_size)
-		fortify_panic(FORTIFY_FUNC_strscpy, FORTIFY_WRITE, -E2BIG);
+	if (p_size < len)
+		fortify_panic(FORTIFY_FUNC_strscpy, FORTIFY_WRITE, p_size, len, -E2BIG);
 
 	/*
 	 * We can now safely call vanilla strscpy because we are protected from:
@@ -423,7 +423,7 @@ size_t strlcat(char * const POS p, const char * const POS q, size_t avail)
 
 	/* Give up if string is already overflowed. */
 	if (p_size <= p_len)
-		fortify_panic(FORTIFY_FUNC_strlcat, FORTIFY_READ, wanted);
+		fortify_panic(FORTIFY_FUNC_strlcat, FORTIFY_READ, p_size, p_len + 1, wanted);
 
 	if (actual >= avail) {
 		copy_len = avail - p_len - 1;
@@ -432,7 +432,7 @@ size_t strlcat(char * const POS p, const char * const POS q, size_t avail)
 
 	/* Give up if copy will overflow. */
 	if (p_size <= actual)
-		fortify_panic(FORTIFY_FUNC_strlcat, FORTIFY_WRITE, wanted);
+		fortify_panic(FORTIFY_FUNC_strlcat, FORTIFY_WRITE, p_size, actual + 1, wanted);
 	__underlying_memcpy(p + p_len, q, copy_len);
 	p[actual] = '\0';
 
@@ -459,9 +459,11 @@ __FORTIFY_INLINE __diagnose_as(__builtin_strcat, 1, 2)
 char *strcat(char * const POS p, const char *q)
 {
 	size_t p_size = __member_size(p);
+	size_t wanted;
 
-	if (strlcat(p, q, p_size) >= p_size)
-		fortify_panic(FORTIFY_FUNC_strcat, FORTIFY_WRITE, p);
+	wanted = strlcat(p, q, p_size);
+	if (p_size <= wanted)
+		fortify_panic(FORTIFY_FUNC_strcat, FORTIFY_WRITE, p_size, wanted + 1, p);
 	return p;
 }
 
@@ -491,13 +493,15 @@ char *strncat(char * const POS p, const char * const POS q, __kernel_size_t coun
 	size_t p_len, copy_len;
 	size_t p_size = __member_size(p);
 	size_t q_size = __member_size(q);
+	size_t total;
 
 	if (p_size == SIZE_MAX && q_size == SIZE_MAX)
 		return __underlying_strncat(p, q, count);
 	p_len = strlen(p);
 	copy_len = strnlen(q, count);
-	if (p_size < p_len + copy_len + 1)
-		fortify_panic(FORTIFY_FUNC_strncat, FORTIFY_WRITE, p);
+	total = p_len + copy_len + 1;
+	if (p_size < total)
+		fortify_panic(FORTIFY_FUNC_strncat, FORTIFY_WRITE, p_size, total, p);
 	__underlying_memcpy(p + p_len, q, copy_len);
 	p[p_len + copy_len] = '\0';
 	return p;
@@ -538,7 +542,7 @@ __FORTIFY_INLINE bool fortify_memset_chk(__kernel_size_t size,
 	 * lengths are unknown.)
 	 */
 	if (p_size != SIZE_MAX && p_size < size)
-		fortify_panic(FORTIFY_FUNC_memset, FORTIFY_WRITE, true);
+		fortify_panic(FORTIFY_FUNC_memset, FORTIFY_WRITE, p_size, size, true);
 	return false;
 }
 
@@ -638,9 +642,9 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,
 	 * lengths are unknown.)
 	 */
 	if (p_size != SIZE_MAX && p_size < size)
-		fortify_panic(func, FORTIFY_WRITE, true);
+		fortify_panic(func, FORTIFY_WRITE, p_size, size, true);
 	else if (q_size != SIZE_MAX && q_size < size)
-		fortify_panic(func, FORTIFY_READ, true);
+		fortify_panic(func, FORTIFY_READ, p_size, size, true);
 
 	/*
 	 * Warn when writing beyond destination field size.
@@ -740,7 +744,7 @@ __FORTIFY_INLINE void *memscan(void * const POS0 p, int c, __kernel_size_t size)
 	if (__compiletime_lessthan(p_size, size))
 		__read_overflow();
 	if (p_size < size)
-		fortify_panic(FORTIFY_FUNC_memscan, FORTIFY_READ, NULL);
+		fortify_panic(FORTIFY_FUNC_memscan, FORTIFY_READ, p_size, size, NULL);
 	return __real_memscan(p, c, size);
 }
 
@@ -756,8 +760,10 @@ int memcmp(const void * const POS0 p, const void * const POS0 q, __kernel_size_t
 		if (__compiletime_lessthan(q_size, size))
 			__read_overflow2();
 	}
-	if (p_size < size || q_size < size)
-		fortify_panic(FORTIFY_FUNC_memcmp, FORTIFY_READ, INT_MIN);
+	if (p_size < size)
+		fortify_panic(FORTIFY_FUNC_memcmp, FORTIFY_READ, p_size, size, INT_MIN);
+	else if (q_size < size)
+		fortify_panic(FORTIFY_FUNC_memcmp, FORTIFY_READ, q_size, size, INT_MIN);
 	return __underlying_memcmp(p, q, size);
 }
 
@@ -769,7 +775,7 @@ void *memchr(const void * const POS0 p, int c, __kernel_size_t size)
 	if (__compiletime_lessthan(p_size, size))
 		__read_overflow();
 	if (p_size < size)
-		fortify_panic(FORTIFY_FUNC_memchr, FORTIFY_READ, NULL);
+		fortify_panic(FORTIFY_FUNC_memchr, FORTIFY_READ, p_size, size, NULL);
 	return __underlying_memchr(p, c, size);
 }
 
@@ -781,7 +787,7 @@ __FORTIFY_INLINE void *memchr_inv(const void * const POS0 p, int c, size_t size)
 	if (__compiletime_lessthan(p_size, size))
 		__read_overflow();
 	if (p_size < size)
-		fortify_panic(FORTIFY_FUNC_memchr_inv, FORTIFY_READ, NULL);
+		fortify_panic(FORTIFY_FUNC_memchr_inv, FORTIFY_READ, p_size, size, NULL);
 	return __real_memchr_inv(p, c, size);
 }
 
@@ -794,7 +800,7 @@ __FORTIFY_INLINE void *kmemdup(const void * const POS0 p, size_t size, gfp_t gfp
 	if (__compiletime_lessthan(p_size, size))
 		__read_overflow();
 	if (p_size < size)
-		fortify_panic(FORTIFY_FUNC_kmemdup, FORTIFY_READ, NULL);
+		fortify_panic(FORTIFY_FUNC_kmemdup, FORTIFY_READ, p_size, size, NULL);
 	return __real_kmemdup(p, size, gfp);
 }
 
@@ -831,7 +837,7 @@ char *strcpy(char * const POS p, const char * const POS q)
 		__write_overflow();
 	/* Run-time check for dynamic size overflow. */
 	if (p_size < size)
-		fortify_panic(FORTIFY_FUNC_strcpy, FORTIFY_WRITE, p);
+		fortify_panic(FORTIFY_FUNC_strcpy, FORTIFY_WRITE, p_size, size, p);
 	__underlying_memcpy(p, q, size);
 	return p;
 }
lib/fortify_kunit.c +2 -2
index b7c884037629..b022797c9fe6 100644
@@ -18,9 +18,9 @@
 /* Call kunit_fortify_panic() instead of fortify_panic() */
 #define FORTIFY_KUNIT_OVERRIDE
 void fortify_add_kunit_error(int write);
-#define kunit_fortify_panic(func, write, retfail)			\
+#define kunit_fortify_panic(func, write, avail, size, retfail)		\
 	do {								\
-		__fortify_report(fortify_reason(func, write));		\
+		__fortify_report(fortify_reason(func, write), avail, size); \
 		fortify_add_kunit_error(write);				\
 		return (retfail);					\
 	} while (0)
lib/string_helpers.c +5 -4
index 5bb65f623e40..cc15a25556fb 100644
@@ -1023,7 +1023,7 @@ EXPORT_SYMBOL(__read_overflow2_field);
 void __write_overflow_field(size_t avail, size_t wanted) { }
 EXPORT_SYMBOL(__write_overflow_field);
 
-void __fortify_report(u8 reason)
+void __fortify_report(const u8 reason, const size_t avail, const size_t size)
 {
 	const char *name;
 	const bool write = !!(reason & 0x1);
@@ -1083,13 +1083,14 @@ void __fortify_report(u8 reason)
 	default:
 		name = "unknown";
 	}
-	WARN(1, "%s: detected buffer %s overflow\n", name, write ? "write" : "read");
+	WARN(1, "%s: detected buffer overflow: %zu byte %s buffer of size %zu\n",
+		 name, size, write ? "write to" : "read from", avail);
 }
 EXPORT_SYMBOL(__fortify_report);
 
-void __fortify_panic(const u8 reason)
+void __fortify_panic(const u8 reason, const size_t avail, const size_t size)
 {
-	__fortify_report(reason);
+	__fortify_report(reason, avail, size);
 	BUG();
 }
 EXPORT_SYMBOL(__fortify_panic);
Signature · 1 line
2.34.1