From 4e9903b0861c9df3464b82db4a7025863bac1897 Mon Sep 17 00:00:00 2001 From: Masahiro Yamada Date: Sun, 28 Jul 2024 00:02:36 +0900 Subject: [PATCH 01/15] fortify: refactor test_fortify Makefile to fix some build problems 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-' 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 Link: https://lore.kernel.org/r/20240727150302.1823750-2-masahiroy@kernel.org Signed-off-by: Kees Cook --- lib/.gitignore | 2 -- lib/Makefile | 38 +------------------------------------ lib/test_fortify/.gitignore | 2 ++ lib/test_fortify/Makefile | 28 +++++++++++++++++++++++++++ scripts/remove-stale-files | 2 ++ 5 files changed, 33 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 > $@ || 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 diff --git a/scripts/remove-stale-files b/scripts/remove-stale-files index f38d26b78c2a..8fc55a749ccc 100755 --- a/scripts/remove-stale-files +++ b/scripts/remove-stale-files @@ -21,3 +21,5 @@ set -e # then will be really dead and removed from the code base entirely. rm -f *.spec + +rm -f lib/test_fortify.log From 5a8d0c46c9e024bed4805a9335fe6124d8a78d3a Mon Sep 17 00:00:00 2001 From: Masahiro Yamada Date: Sun, 28 Jul 2024 00:02:37 +0900 Subject: [PATCH 02/15] fortify: move test_fortify.sh to lib/test_fortify/ This script is only used in lib/test_fortify/. There is no reason to keep it in scripts/. Signed-off-by: Masahiro Yamada Link: https://lore.kernel.org/r/20240727150302.1823750-3-masahiroy@kernel.org Signed-off-by: Kees Cook --- 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 8766f3e5e87e..36c0af94cf08 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8772,7 +8772,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 From 9c6b7fbbd7a2c2772a592adf9b2835371927a1d3 Mon Sep 17 00:00:00 2001 From: Masahiro Yamada Date: Sun, 28 Jul 2024 00:02:38 +0900 Subject: [PATCH 03/15] fortify: use if_changed_dep to record header dependency in *.cmd files 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 Link: https://lore.kernel.org/r/20240727150302.1823750-4-masahiroy@kernel.org Signed-off-by: Kees Cook --- 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. From a98ae7f045b29de4f48b191d5aeb4e803183d759 Mon Sep 17 00:00:00 2001 From: Michal Wajdeczko Date: Thu, 25 Jul 2024 12:18:40 +0200 Subject: [PATCH 04/15] lib/string_choices: Add str_up_down() helper Add str_up_down() helper to return "up" or "down" string literal. Signed-off-by: Michal Wajdeczko Link: https://lore.kernel.org/r/20240725101841.574-1-michal.wajdeczko@intel.com Signed-off-by: Kees Cook --- include/linux/string_choices.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/include/linux/string_choices.h b/include/linux/string_choices.h index d9ebe20229f8..bcde3c9cff81 100644 --- a/include/linux/string_choices.h +++ b/include/linux/string_choices.h @@ -42,6 +42,11 @@ static inline const char *str_yes_no(bool v) return v ? "yes" : "no"; } +static inline const char *str_up_down(bool v) +{ + return v ? "up" : "down"; +} + /** * str_plural - Return the simple pluralization based on English counts * @num: Number used for deciding pluralization From 9b97452bcce77f8ef29b20c9662d95988b5990e4 Mon Sep 17 00:00:00 2001 From: Michal Wajdeczko Date: Thu, 25 Jul 2024 12:18:41 +0200 Subject: [PATCH 05/15] coccinelle: Add rules to find str_up_down() replacements Add rules for finding places where str_up_down() can be used. This currently finds over 20 locations. Signed-off-by: Michal Wajdeczko Link: https://lore.kernel.org/r/20240725101841.574-2-michal.wajdeczko@intel.com Signed-off-by: Kees Cook --- scripts/coccinelle/api/string_choices.cocci | 23 +++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/scripts/coccinelle/api/string_choices.cocci b/scripts/coccinelle/api/string_choices.cocci index a71966c0494e..d517f6bc850b 100644 --- a/scripts/coccinelle/api/string_choices.cocci +++ b/scripts/coccinelle/api/string_choices.cocci @@ -39,3 +39,26 @@ e << str_plural_r.E; @@ coccilib.report.print_report(p[0], "opportunity for str_plural(%s)" % e) + +@str_up_down depends on patch@ +expression E; +@@ +( +- ((E) ? "up" : "down") ++ str_up_down(E) +) + +@str_up_down_r depends on !patch exists@ +expression E; +position P; +@@ +( +* ((E@P) ? "up" : "down") +) + +@script:python depends on report@ +p << str_up_down_r.P; +e << str_up_down_r.E; +@@ + +coccilib.report.print_report(p[0], "opportunity for str_up_down(%s)" % e) From f5c1ca3a15fdb867d2b535003f74e0b975eff116 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Mon, 12 Aug 2024 11:29:40 -0700 Subject: [PATCH 06/15] string_choices: Add wrapper for str_down_up() The string choice functions which are not clearly true/false synonyms also have inverted wrappers. Add this for str_down_up() as well. Suggested-by: Andy Shevchenko Link: https://lore.kernel.org/r/20240812182939.work.424-kees@kernel.org Reviewed-by: Andy Shevchenko Signed-off-by: Kees Cook --- include/linux/string_choices.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/linux/string_choices.h b/include/linux/string_choices.h index bcde3c9cff81..1320bcdcb89c 100644 --- a/include/linux/string_choices.h +++ b/include/linux/string_choices.h @@ -46,6 +46,7 @@ static inline const char *str_up_down(bool v) { return v ? "up" : "down"; } +#define str_down_up(v) str_up_down(!(v)) /** * str_plural - Return the simple pluralization based on English counts From 0336f898881ae13b92dfd8b72e69ed1246eac762 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Mon, 12 Aug 2024 11:36:38 -0700 Subject: [PATCH 07/15] coccinelle: Add rules to find str_down_up() replacements As done with str_up_down(), add checks for str_down_up() opportunities. 5 cases currently exist in the tree. Suggested-by: Andy Shevchenko Link: https://lore.kernel.org/r/20240812183637.work.999-kees@kernel.org Reviewed-by: Andy Shevchenko Signed-off-by: Kees Cook --- scripts/coccinelle/api/string_choices.cocci | 23 +++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/scripts/coccinelle/api/string_choices.cocci b/scripts/coccinelle/api/string_choices.cocci index d517f6bc850b..5e729f187f22 100644 --- a/scripts/coccinelle/api/string_choices.cocci +++ b/scripts/coccinelle/api/string_choices.cocci @@ -62,3 +62,26 @@ e << str_up_down_r.E; @@ coccilib.report.print_report(p[0], "opportunity for str_up_down(%s)" % e) + +@str_down_up depends on patch@ +expression E; +@@ +( +- ((E) ? "down" : "up") ++ str_down_up(E) +) + +@str_down_up_r depends on !patch exists@ +expression E; +position P; +@@ +( +* ((E@P) ? "down" : "up") +) + +@script:python depends on report@ +p << str_down_up_r.P; +e << str_down_up_r.E; +@@ + +coccilib.report.print_report(p[0], "opportunity for str_down_up(%s)" % e) From bbf3c7ff9dfa45be51500d23a1276991a7cd8c6e Mon Sep 17 00:00:00 2001 From: Justin Stitt Date: Thu, 8 Aug 2024 14:43:56 -0700 Subject: [PATCH 08/15] lib/string_helpers: rework overflow-dependent code When @size is 0, the desired behavior is to allow unlimited bytes to be parsed. Currently, this relies on some intentional arithmetic overflow where --size gives us SIZE_MAX when size is 0. Explicitly spell out the desired behavior without relying on intentional overflow/underflow. Signed-off-by: Justin Stitt Link: https://lore.kernel.org/r/20240808-b4-string_helpers_caa133-v1-1-686a455167c4@google.com Signed-off-by: Kees Cook --- lib/string_helpers.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/string_helpers.c b/lib/string_helpers.c index 69ba49b853c7..4f887aa62fa0 100644 --- a/lib/string_helpers.c +++ b/lib/string_helpers.c @@ -321,6 +321,9 @@ int string_unescape(char *src, char *dst, size_t size, unsigned int flags) { char *out = dst; + if (!size) + size = SIZE_MAX; + while (*src && --size) { if (src[0] == '\\' && src[1] != '\0' && size > 1) { src++; From 5ac86f0ed04bce41242167ffa12ad92038788a95 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Wed, 10 Jul 2024 16:15:55 -0700 Subject: [PATCH 09/15] virt: vbox: struct vmmdev_hgcm_pagelist: Replace 1-element array with flexible array Replace the deprecated[1] use of a 1-element array in struct vmmdev_hgcm_pagelist with a modern flexible array. As this is UAPI, we cannot trivially change the size of the struct, so use a union to retain the old first element's size, but switch "pages" to a flexible array. No binary differences are present after this conversion. Link: https://github.com/KSPP/linux/issues/79 [1] Reviewed-by: Gustavo A. R. Silva Link: https://lore.kernel.org/r/20240710231555.work.406-kees@kernel.org Signed-off-by: Kees Cook --- include/uapi/linux/vbox_vmmdev_types.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/include/uapi/linux/vbox_vmmdev_types.h b/include/uapi/linux/vbox_vmmdev_types.h index f8a8d6b3c521..6073858d52a2 100644 --- a/include/uapi/linux/vbox_vmmdev_types.h +++ b/include/uapi/linux/vbox_vmmdev_types.h @@ -282,7 +282,10 @@ struct vmmdev_hgcm_pagelist { __u32 flags; /** VMMDEV_HGCM_F_PARM_*. */ __u16 offset_first_page; /** Data offset in the first page. */ __u16 page_count; /** Number of pages. */ - __u64 pages[1]; /** Page addresses. */ + union { + __u64 unused; /** Deprecated place-holder for first "pages" entry. */ + __DECLARE_FLEX_ARRAY(__u64, pages); /** Page addresses. */ + }; }; VMMDEV_ASSERT_SIZE(vmmdev_hgcm_pagelist, 4 + 2 + 2 + 8); From c93452777f537b2f6c3c601dc484821142b07dc7 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Wed, 10 Jul 2024 16:09:12 -0700 Subject: [PATCH 10/15] media: venus: hfi_cmds: struct hfi_session_release_buffer_pkt: Replace 1-element array with flexible array Replace the deprecated[1] use of a 1-element array in struct hfi_session_release_buffer_pkt with a modern flexible array. No binary differences are present after this conversion. Link: https://github.com/KSPP/linux/issues/79 [1] Reviewed-by: Vikash Garodia Reviewed-by: Gustavo A. R. Silva Reviewed-by: Dikshita Agarwal Reviewed-by: Bryan O'Donoghue Acked-by: Vikash Garodia Reviewed-by: Laurent Pinchart Link: https://lore.kernel.org/r/20240710230914.3156277-1-kees@kernel.org Signed-off-by: Kees Cook --- drivers/media/platform/qcom/venus/hfi_cmds.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.h b/drivers/media/platform/qcom/venus/hfi_cmds.h index 20acd412ee7b..42825f07939d 100644 --- a/drivers/media/platform/qcom/venus/hfi_cmds.h +++ b/drivers/media/platform/qcom/venus/hfi_cmds.h @@ -227,7 +227,7 @@ struct hfi_session_release_buffer_pkt { u32 extradata_size; u32 response_req; u32 num_buffers; - u32 buffer_info[1]; + u32 buffer_info[]; }; struct hfi_session_release_resources_pkt { From 32ef4b710cbe1a8901534033872a5bc6b1618bbc Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Wed, 10 Jul 2024 16:09:13 -0700 Subject: [PATCH 11/15] media: venus: hfi_cmds: struct hfi_session_release_buffer_pkt: Add __counted_by annotation The only direct user of struct hfi_session_release_buffer_pkt is pkt_session_unset_buffers() which sets "num_buffers" before using it as a loop counter for accessing "buffer_info". Add the __counted_by annotation to reflect the relationship. Reviewed-by: Vikash Garodia Reviewed-by: Bryan O'Donoghue Reviewed-by: Dikshita Agarwal Reviewed-by: Gustavo A. R. Silva Link: https://lore.kernel.org/r/20240710230914.3156277-2-kees@kernel.org Signed-off-by: Kees Cook --- drivers/media/platform/qcom/venus/hfi_cmds.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.h b/drivers/media/platform/qcom/venus/hfi_cmds.h index 42825f07939d..1adf2d2ae5f2 100644 --- a/drivers/media/platform/qcom/venus/hfi_cmds.h +++ b/drivers/media/platform/qcom/venus/hfi_cmds.h @@ -227,7 +227,7 @@ struct hfi_session_release_buffer_pkt { u32 extradata_size; u32 response_req; u32 num_buffers; - u32 buffer_info[]; + u32 buffer_info[] __counted_by(num_buffers); }; struct hfi_session_release_resources_pkt { From 559048d156ff3391c4b793779a824c9193e20442 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Mon, 5 Aug 2024 14:43:44 -0700 Subject: [PATCH 12/15] string: Check for "nonstring" attribute on strscpy() arguments GCC already checks for arguments that are marked with the "nonstring"[1] attribute when used on standard C String API functions (e.g. strcpy). Gain this compile-time checking also for the kernel's primary string copying function, strscpy(). Note that Clang has neither "nonstring" nor __builtin_has_attribute(). Link: https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#index-nonstring-variable-attribute [1] Reviewed-by: Miguel Ojeda Tested-by: Miguel Ojeda Link: https://lore.kernel.org/r/20240805214340.work.339-kees@kernel.org Signed-off-by: Kees Cook --- include/linux/compiler.h | 3 +++ include/linux/compiler_types.h | 7 +++++++ include/linux/string.h | 12 ++++++++---- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/include/linux/compiler.h b/include/linux/compiler.h index 2df665fa2964..ec55bcce4146 100644 --- a/include/linux/compiler.h +++ b/include/linux/compiler.h @@ -242,6 +242,9 @@ static inline void *offset_to_ptr(const int *off) /* &a[0] degrades to a pointer: a different type from an array */ #define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0])) +/* Require C Strings (i.e. NUL-terminated) lack the "nonstring" attribute. */ +#define __must_be_cstr(p) BUILD_BUG_ON_ZERO(__annotated(p, nonstring)) + /* * This returns a constant expression while determining if an argument is * a constant expression, most importantly without evaluating the argument. diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h index f14c275950b5..1a957ea2f4fe 100644 --- a/include/linux/compiler_types.h +++ b/include/linux/compiler_types.h @@ -421,6 +421,13 @@ struct ftrace_likely_data { #define __member_size(p) __builtin_object_size(p, 1) #endif +/* Determine if an attribute has been applied to a variable. */ +#if __has_builtin(__builtin_has_attribute) +#define __annotated(var, attr) __builtin_has_attribute(var, attr) +#else +#define __annotated(var, attr) (false) +#endif + /* * Some versions of gcc do not mark 'asm goto' volatile: * diff --git a/include/linux/string.h b/include/linux/string.h index 9edace076ddb..95b3fc308f4f 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -76,12 +76,16 @@ ssize_t sized_strscpy(char *, const char *, size_t); * known size. */ #define __strscpy0(dst, src, ...) \ - sized_strscpy(dst, src, sizeof(dst) + __must_be_array(dst)) -#define __strscpy1(dst, src, size) sized_strscpy(dst, src, size) + sized_strscpy(dst, src, sizeof(dst) + __must_be_array(dst) + \ + __must_be_cstr(dst) + __must_be_cstr(src)) +#define __strscpy1(dst, src, size) \ + sized_strscpy(dst, src, size + __must_be_cstr(dst) + __must_be_cstr(src)) #define __strscpy_pad0(dst, src, ...) \ - sized_strscpy_pad(dst, src, sizeof(dst) + __must_be_array(dst)) -#define __strscpy_pad1(dst, src, size) sized_strscpy_pad(dst, src, size) + sized_strscpy_pad(dst, src, sizeof(dst) + __must_be_array(dst) + \ + __must_be_cstr(dst) + __must_be_cstr(src)) +#define __strscpy_pad1(dst, src, size) \ + sized_strscpy_pad(dst, src, size + __must_be_cstr(dst) + __must_be_cstr(src)) /** * strscpy - Copy a C-string into a sized buffer From 6ff4cd1160afafc12ad1603e3d2f39256e4b708d Mon Sep 17 00:00:00 2001 From: Hongbo Li Date: Tue, 27 Aug 2024 10:45:15 +0800 Subject: [PATCH 13/15] lib/string_choices: Add str_true_false()/str_false_true() helper Add str_true_false()/str_false_true() helper to return "true" or "false" string literal. Signed-off-by: Hongbo Li Link: https://lore.kernel.org/r/20240827024517.914100-2-lihongbo22@huawei.com Signed-off-by: Kees Cook --- include/linux/string_choices.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/include/linux/string_choices.h b/include/linux/string_choices.h index 1320bcdcb89c..ebcc56b28ede 100644 --- a/include/linux/string_choices.h +++ b/include/linux/string_choices.h @@ -48,6 +48,12 @@ static inline const char *str_up_down(bool v) } #define str_down_up(v) str_up_down(!(v)) +static inline const char *str_true_false(bool v) +{ + return v ? "true" : "false"; +} +#define str_false_true(v) str_true_false(!(v)) + /** * str_plural - Return the simple pluralization based on English counts * @num: Number used for deciding pluralization From c2708ba91c3c1fba424b77de83b6fc45cbf38c46 Mon Sep 17 00:00:00 2001 From: Hongbo Li Date: Thu, 5 Sep 2024 17:25:39 +0800 Subject: [PATCH 14/15] lib/string_choices: Introduce several opposite string choice helpers Similar to the exists helper: str_enable_disable/ str_enabled_disabled/str_on_off/str_yes_no helpers, we can add the opposite helpers. That's str_disable_enable, str_disabled_enabled, str_off_on and str_no_yes. There are more than 10 cases currently (expect str_disable_enable now has 3 use cases) exist in the code can be replaced with these helper. Signed-off-by: Hongbo Li Acked-by: Andy Shevchenko Link: https://lore.kernel.org/r/20240905092540.2962122-2-lihongbo22@huawei.com Signed-off-by: Kees Cook --- include/linux/string_choices.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/include/linux/string_choices.h b/include/linux/string_choices.h index ebcc56b28ede..fd067992260a 100644 --- a/include/linux/string_choices.h +++ b/include/linux/string_choices.h @@ -8,11 +8,13 @@ static inline const char *str_enable_disable(bool v) { return v ? "enable" : "disable"; } +#define str_disable_enable(v) str_enable_disable(!(v)) static inline const char *str_enabled_disabled(bool v) { return v ? "enabled" : "disabled"; } +#define str_disabled_enabled(v) str_enabled_disabled(!(v)) static inline const char *str_hi_lo(bool v) { @@ -36,11 +38,13 @@ static inline const char *str_on_off(bool v) { return v ? "on" : "off"; } +#define str_off_on(v) str_on_off(!(v)) static inline const char *str_yes_no(bool v) { return v ? "yes" : "no"; } +#define str_no_yes(v) str_yes_no(!(v)) static inline const char *str_up_down(bool v) { From c121d5cc3a993cdbfab46a152bdd50227a4d5e8c Mon Sep 17 00:00:00 2001 From: Hongbo Li Date: Thu, 5 Sep 2024 17:25:40 +0800 Subject: [PATCH 15/15] lib/string_choices: Add some comments to make more clear for string choices helpers. Add some comments to explain why we should use string_choices helpers. Signed-off-by: Hongbo Li Acked-by: Andy Shevchenko Link: https://lore.kernel.org/r/20240905092540.2962122-3-lihongbo22@huawei.com Signed-off-by: Kees Cook --- include/linux/string_choices.h | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/include/linux/string_choices.h b/include/linux/string_choices.h index fd067992260a..120ca0f28e95 100644 --- a/include/linux/string_choices.h +++ b/include/linux/string_choices.h @@ -2,6 +2,19 @@ #ifndef _LINUX_STRING_CHOICES_H_ #define _LINUX_STRING_CHOICES_H_ +/* + * Here provide a series of helpers in the str_$TRUE_$FALSE format (you can + * also expand some helpers as needed), where $TRUE and $FALSE are their + * corresponding literal strings. These helpers can be used in the printing + * and also in other places where constant strings are required. Using these + * helpers offers the following benefits: + * 1) Reducing the hardcoding of strings, which makes the code more elegant + * through these simple literal-meaning helpers. + * 2) Unifying the output, which prevents the same string from being printed + * in various forms, such as enable/disable, enabled/disabled, en/dis. + * 3) Deduping by the linker, which results in a smaller binary file. + */ + #include static inline const char *str_enable_disable(bool v)