[PATCH v2 0/3] fortify: fix various issues in test_fortify Makefile

[PATCH v2 0/3] fortify: fix various issues in test_fortify Makefile

[Masahiro Yamada]

This version fixes new warnings for GCC <= 7, which were reported
by 0 day bot.

I changed the patch order, as 3/3 is the most controvertial.
I am confident with 1/3 and 2/3.
3/3 drops the test coverage for GCC <= 7.



Masahiro Yamada (3):
  fortify: refactor test_fortify Makefile to fix some build problems
  fortify: move test_fortify.sh to lib/test_fortify/
  fortify: use if_changed_dep to record header dependency in *.cmd files

 MAINTAINERS                                   |  1 -
 lib/.gitignore                                |  2 -
 lib/Makefile                                  | 38 +------------------
 lib/test_fortify/.gitignore                   |  2 +
 lib/test_fortify/Makefile                     | 28 ++++++++++++++
 {scripts => lib/test_fortify}/test_fortify.sh |  0
 6 files changed, 31 insertions(+), 40 deletions(-)
 create mode 100644 lib/test_fortify/.gitignore
 create mode 100644 lib/test_fortify/Makefile
 rename {scripts => lib/test_fortify}/test_fortify.sh (100%)

-- 
2.43.0

[PATCH v2 1/3] fortify: refactor test_fortify Makefile to fix some build problems

[Masahiro Yamada]

There are some issues in the test_fortify Makefile code.

Problem 1: cc-disable-warning invokes compiler dozens of times

To see how many times the cc-disable-warning is evaluated, change
this code:

  $(call cc-disable-warning,fortify-source)

to:

  $(call cc-disable-warning,$(shell touch /tmp/fortify-$$$$)fortify-source)

Then, build the kernel with CONFIG_FORTIFY_SOURCE=y. You will see a
large number of '/tmp/fortify-<PID>' files created:

  $ ls -1 /tmp/fortify-* | wc
       80      80    1600

This means the compiler was invoked 80 times just for checking the
-Wno-fortify-source flag support.

$(call cc-disable-warning,fortify-source) should be added to a simple
variable instead of a recursive variable.

Problem 2: do not recompile string.o when the test code is updated

The test cases are independent of the kernel. However, when the test
code is updated, $(obj)/string.o is rebuilt and vmlinux is relinked
due to this dependency:

  $(obj)/string.o: $(obj)/$(TEST_FORTIFY_LOG)

always-y is suitable for building the log files.

Problem 3: redundant code

  clean-files += $(addsuffix .o, $(TEST_FORTIFY_LOGS))

... is unneeded because the top Makefile globally cleans *.o files.

This commit fixes these issues and makes the code readable.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

(no changes since v1)

 lib/.gitignore              |  2 --
 lib/Makefile                | 38 +------------------------------------
 lib/test_fortify/.gitignore |  2 ++
 lib/test_fortify/Makefile   | 28 +++++++++++++++++++++++++++
 4 files changed, 31 insertions(+), 39 deletions(-)
 create mode 100644 lib/test_fortify/.gitignore
 create mode 100644 lib/test_fortify/Makefile

diff --git a/lib/.gitignore b/lib/.gitignore
index 54596b634ecb..101a4aa92fb5 100644
--- a/lib/.gitignore
+++ b/lib/.gitignore
@@ -5,5 +5,3 @@
 /gen_crc32table
 /gen_crc64table
 /oid_registry_data.c
-/test_fortify.log
-/test_fortify/*.log
diff --git a/lib/Makefile b/lib/Makefile
index 322bb127b4dc..4df3c28b23b4 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -393,40 +393,4 @@ obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o
 
 obj-$(CONFIG_FIRMWARE_TABLE) += fw_table.o
 
-# FORTIFY_SOURCE compile-time behavior tests
-TEST_FORTIFY_SRCS = $(wildcard $(src)/test_fortify/*-*.c)
-TEST_FORTIFY_LOGS = $(patsubst $(src)/%.c, %.log, $(TEST_FORTIFY_SRCS))
-TEST_FORTIFY_LOG = test_fortify.log
-
-quiet_cmd_test_fortify = TEST    $@
-      cmd_test_fortify = $(CONFIG_SHELL) $(srctree)/scripts/test_fortify.sh \
-			$< $@ "$(NM)" $(CC) $(c_flags) \
-			$(call cc-disable-warning,fortify-source) \
-			-DKBUILD_EXTRA_WARN1
-
-targets += $(TEST_FORTIFY_LOGS)
-clean-files += $(TEST_FORTIFY_LOGS)
-clean-files += $(addsuffix .o, $(TEST_FORTIFY_LOGS))
-$(obj)/test_fortify/%.log: $(src)/test_fortify/%.c \
-			   $(src)/test_fortify/test_fortify.h \
-			   $(srctree)/include/linux/fortify-string.h \
-			   $(srctree)/scripts/test_fortify.sh \
-			   FORCE
-	$(call if_changed,test_fortify)
-
-quiet_cmd_gen_fortify_log = GEN     $@
-      cmd_gen_fortify_log = cat </dev/null $(filter-out FORCE,$^) 2>/dev/null > $@ || true
-
-targets += $(TEST_FORTIFY_LOG)
-clean-files += $(TEST_FORTIFY_LOG)
-$(obj)/$(TEST_FORTIFY_LOG): $(addprefix $(obj)/, $(TEST_FORTIFY_LOGS)) FORCE
-	$(call if_changed,gen_fortify_log)
-
-# Fake dependency to trigger the fortify tests.
-ifeq ($(CONFIG_FORTIFY_SOURCE),y)
-$(obj)/string.o: $(obj)/$(TEST_FORTIFY_LOG)
-endif
-
-# Some architectures define __NO_FORTIFY if __SANITIZE_ADDRESS__ is undefined.
-# Pass CFLAGS_KASAN to avoid warnings.
-$(foreach x, $(patsubst %.log,%.o,$(TEST_FORTIFY_LOGS)), $(eval KASAN_SANITIZE_$(x) := y))
+subdir-$(CONFIG_FORTIFY_SOURCE) += test_fortify
diff --git a/lib/test_fortify/.gitignore b/lib/test_fortify/.gitignore
new file mode 100644
index 000000000000..c1ba37d14b50
--- /dev/null
+++ b/lib/test_fortify/.gitignore
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+/*.log
diff --git a/lib/test_fortify/Makefile b/lib/test_fortify/Makefile
new file mode 100644
index 000000000000..3907a2242ef9
--- /dev/null
+++ b/lib/test_fortify/Makefile
@@ -0,0 +1,28 @@
+# SPDX-License-Identifier: GPL-2.0
+
+ccflags-y := $(call cc-disable-warning,fortify-source)
+
+quiet_cmd_test_fortify = TEST    $@
+      cmd_test_fortify = $(CONFIG_SHELL) $(srctree)/scripts/test_fortify.sh \
+			$< $@ "$(NM)" $(CC) $(c_flags) -DKBUILD_EXTRA_WARN1
+
+$(obj)/%.log: $(src)/%.c $(srctree)/scripts/test_fortify.sh \
+	      $(src)/test_fortify.h \
+	      $(srctree)/include/linux/fortify-string.h \
+	      FORCE
+	$(call if_changed,test_fortify)
+
+logs = $(patsubst $(src)/%.c, %.log, $(wildcard $(src)/*-*.c))
+targets += $(logs)
+
+quiet_cmd_gen_fortify_log = CAT     $@
+      cmd_gen_fortify_log = cat $(or $(real-prereqs),/dev/null) > $@
+
+$(obj)/test_fortify.log: $(addprefix $(obj)/, $(logs)) FORCE
+	$(call if_changed,gen_fortify_log)
+
+always-y += test_fortify.log
+
+# Some architectures define __NO_FORTIFY if __SANITIZE_ADDRESS__ is undefined.
+# Pass CFLAGS_KASAN to avoid warnings.
+KASAN_SANITIZE := y
-- 
2.43.0

[PATCH v2 2/3] fortify: move test_fortify.sh to lib/test_fortify/

[Masahiro Yamada]

This script is only used in lib/test_fortify/.

There is no reason to keep it in scripts/.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

(no changes since v1)

 MAINTAINERS                                   | 1 -
 lib/test_fortify/Makefile                     | 4 ++--
 {scripts => lib/test_fortify}/test_fortify.sh | 0
 3 files changed, 2 insertions(+), 3 deletions(-)
 rename {scripts => lib/test_fortify}/test_fortify.sh (100%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 85fbbc25112f..6e14bd77e3c8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8758,7 +8758,6 @@ F:	include/linux/fortify-string.h
 F:	lib/fortify_kunit.c
 F:	lib/memcpy_kunit.c
 F:	lib/test_fortify/*
-F:	scripts/test_fortify.sh
 K:	\b__NO_FORTIFY\b
 
 FPGA DFL DRIVERS
diff --git a/lib/test_fortify/Makefile b/lib/test_fortify/Makefile
index 3907a2242ef9..1826172c32d4 100644
--- a/lib/test_fortify/Makefile
+++ b/lib/test_fortify/Makefile
@@ -3,10 +3,10 @@
 ccflags-y := $(call cc-disable-warning,fortify-source)
 
 quiet_cmd_test_fortify = TEST    $@
-      cmd_test_fortify = $(CONFIG_SHELL) $(srctree)/scripts/test_fortify.sh \
+      cmd_test_fortify = $(CONFIG_SHELL) $(src)/test_fortify.sh \
 			$< $@ "$(NM)" $(CC) $(c_flags) -DKBUILD_EXTRA_WARN1
 
-$(obj)/%.log: $(src)/%.c $(srctree)/scripts/test_fortify.sh \
+$(obj)/%.log: $(src)/%.c $(src)/test_fortify.sh \
 	      $(src)/test_fortify.h \
 	      $(srctree)/include/linux/fortify-string.h \
 	      FORCE
diff --git a/scripts/test_fortify.sh b/lib/test_fortify/test_fortify.sh
similarity index 100%
rename from scripts/test_fortify.sh
rename to lib/test_fortify/test_fortify.sh
-- 
2.43.0

[PATCH v2 3/3] fortify: use if_changed_dep to record header dependency in *.cmd files

[Masahiro Yamada]

After building with CONFIG_FORTIFY_SOURCE=y, many .*.d files are left
in lib/test_fortify/ because the compiler outputs header dependencies
into *.d without fixdep being invoked.

When compiling C files, if_changed_dep should be used so that the
auto-generated header dependencies are recorded in .*.cmd files.

Currently, if_changed is incorrectly used, and only two headers are
hard-coded in lib/Makefile.

In the previous patch version, the kbuild test robot detected new errors
on GCC 7.

GCC 7 or older does not produce test.d with the following test code:

 $ echo 'void b(void) __attribute__((__error__(""))); void a(void) { b(); }' |
   gcc -Wp,-MMD,test.d -c -o /dev/null -x c -

Perhaps, this was a bug that existed in older GCC versions.

Skip the tests for GCC<=7 for now, as this will be eventually solved
when we bump the minimal supported GCC version.

Link: https://lore.kernel.org/oe-kbuild-all/CAK7LNARmJcyyzL-jVJfBPi3W684LTDmuhMf1koF0TXoCpKTmcw@mail.gmail.com/T/#m13771bf78ae21adff22efc4d310c973fb4bcaf67
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

Changes in v2:
  - Skip the tests for GCC <= 7

 lib/test_fortify/Makefile | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/test_fortify/Makefile b/lib/test_fortify/Makefile
index 1826172c32d4..1c3f82ad8bb2 100644
--- a/lib/test_fortify/Makefile
+++ b/lib/test_fortify/Makefile
@@ -6,11 +6,8 @@ quiet_cmd_test_fortify = TEST    $@
       cmd_test_fortify = $(CONFIG_SHELL) $(src)/test_fortify.sh \
 			$< $@ "$(NM)" $(CC) $(c_flags) -DKBUILD_EXTRA_WARN1
 
-$(obj)/%.log: $(src)/%.c $(src)/test_fortify.sh \
-	      $(src)/test_fortify.h \
-	      $(srctree)/include/linux/fortify-string.h \
-	      FORCE
-	$(call if_changed,test_fortify)
+$(obj)/%.log: $(src)/%.c $(src)/test_fortify.sh FORCE
+	$(call if_changed_dep,test_fortify)
 
 logs = $(patsubst $(src)/%.c, %.log, $(wildcard $(src)/*-*.c))
 targets += $(logs)
@@ -21,7 +18,10 @@ quiet_cmd_gen_fortify_log = CAT     $@
 $(obj)/test_fortify.log: $(addprefix $(obj)/, $(logs)) FORCE
 	$(call if_changed,gen_fortify_log)
 
-always-y += test_fortify.log
+# GCC<=7 does not always produce *.d files.
+# Run the tests only for GCC>=8 or Clang.
+always-$(call gcc-min-version, 80000) += test_fortify.log
+always-$(CONFIG_CC_IS_CLANG)          += test_fortify.log
 
 # Some architectures define __NO_FORTIFY if __SANITIZE_ADDRESS__ is undefined.
 # Pass CFLAGS_KASAN to avoid warnings.
-- 
2.43.0

Re: [PATCH v2 0/3] fortify: fix various issues in test_fortify Makefile

[Kees Cook]

On Sun, 28 Jul 2024 00:02:35 +0900, Masahiro Yamada wrote:
> This version fixes new warnings for GCC <= 7, which were reported
> by 0 day bot.
> 
> I changed the patch order, as 3/3 is the most controvertial.
> I am confident with 1/3 and 2/3.
> 3/3 drops the test coverage for GCC <= 7.
> 
> [...]

Applied to for-next/hardening, thanks!

[1/3] fortify: refactor test_fortify Makefile to fix some build problems
      https://git.kernel.org/kees/c/61b317f70aa7
[2/3] fortify: move test_fortify.sh to lib/test_fortify/
      https://git.kernel.org/kees/c/728dc04bc4e3
[3/3] fortify: use if_changed_dep to record header dependency in *.cmd files
      https://git.kernel.org/kees/c/634a52a98f04

Take care,

-- 
Kees Cook