From 167ebeda36fae4bb47ace32bcacecde7d24d2850 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Tue, 23 Jan 2024 16:32:54 -0800 Subject: ubsan: Use Clang's -fsanitize-trap=undefined option Clang changed the way it enables UBSan trapping mode. Update the Makefile logic to discover it. Suggested-by: Fangrui Song Link: https://lore.kernel.org/lkml/CAFP8O3JivZh+AAV7N90Nk7U2BHRNST6MRP0zHtfQ-Vj0m4+pDA@mail.gmail.com/ Reviewed-by: Fangrui Song Reviewed-by: Justin Stitt Cc: Nathan Chancellor Cc: Masahiro Yamada Cc: Nicolas Schier Cc: Nick Desaulniers Cc: Bill Wendling Cc: linux-kbuild@vger.kernel.org Cc: llvm@lists.linux.dev Signed-off-by: Kees Cook --- scripts/Makefile.ubsan | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'scripts') diff --git a/scripts/Makefile.ubsan b/scripts/Makefile.ubsan index 4749865c1b2c..7cf42231042b 100644 --- a/scripts/Makefile.ubsan +++ b/scripts/Makefile.ubsan @@ -10,6 +10,6 @@ ubsan-cflags-$(CONFIG_UBSAN_DIV_ZERO) += -fsanitize=integer-divide-by-zero ubsan-cflags-$(CONFIG_UBSAN_UNREACHABLE) += -fsanitize=unreachable ubsan-cflags-$(CONFIG_UBSAN_BOOL) += -fsanitize=bool ubsan-cflags-$(CONFIG_UBSAN_ENUM) += -fsanitize=enum -ubsan-cflags-$(CONFIG_UBSAN_TRAP) += -fsanitize-undefined-trap-on-error +ubsan-cflags-$(CONFIG_UBSAN_TRAP) += $(call cc-option,-fsanitize-trap=undefined,-fsanitize-undefined-trap-on-error) export CFLAGS_UBSAN := $(ubsan-cflags-y) -- cgit From 918327e9b7ffb45321cbb4b9b86b58ec555fe6b3 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Sun, 28 Jan 2024 10:45:29 -0800 Subject: ubsan: Remove CONFIG_UBSAN_SANITIZE_ALL For simplicity in splitting out UBSan options into separate rules, remove CONFIG_UBSAN_SANITIZE_ALL, effectively defaulting to "y", which is how it is generally used anyway. (There are no ":= y" cases beyond where a specific file is enabled when a top-level ":= n" is in effect.) Cc: Andrey Konovalov Cc: Marco Elver Cc: linux-doc@vger.kernel.org Cc: linux-kbuild@vger.kernel.org Signed-off-by: Kees Cook --- scripts/Makefile.lib | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'scripts') diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index cd5b181060f1..52efc520ae4f 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -175,7 +175,7 @@ endif ifeq ($(CONFIG_UBSAN),y) _c_flags += $(if $(patsubst n%,, \ - $(UBSAN_SANITIZE_$(basetarget).o)$(UBSAN_SANITIZE)$(CONFIG_UBSAN_SANITIZE_ALL)), \ + $(UBSAN_SANITIZE_$(basetarget).o)$(UBSAN_SANITIZE)y), \ $(CFLAGS_UBSAN)) endif -- cgit From 557f8c582a9ba8abe6aa0fd734b6f342af106b26 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Thu, 18 Jan 2024 15:06:05 -0800 Subject: ubsan: Reintroduce signed overflow sanitizer In order to mitigate unexpected signed wrap-around[1], bring back the signed integer overflow sanitizer. It was removed in commit 6aaa31aeb9cf ("ubsan: remove overflow checks") because it was effectively a no-op when combined with -fno-strict-overflow (which correctly changes signed overflow from being "undefined" to being explicitly "wrap around"). Compilers are adjusting their sanitizers to trap wrap-around and to detecting common code patterns that should not be instrumented (e.g. "var + offset < var"). Prepare for this and explicitly rename the option from "OVERFLOW" to "WRAP" to more accurately describe the behavior. To annotate intentional wrap-around arithmetic, the helpers wrapping_add/sub/mul_wrap() can be used for individual statements. At the function level, the __signed_wrap attribute can be used to mark an entire function as expecting its signed arithmetic to wrap around. For a single object file the Makefile can use "UBSAN_SIGNED_WRAP_target.o := n" to mark it as wrapping, and for an entire directory, "UBSAN_SIGNED_WRAP := n" can be used. Additionally keep these disabled under CONFIG_COMPILE_TEST for now. Link: https://github.com/KSPP/linux/issues/26 [1] Cc: Miguel Ojeda Cc: Nathan Chancellor Cc: Peter Zijlstra Cc: Hao Luo Reviewed-by: Marco Elver Reviewed-by: Justin Stitt Signed-off-by: Kees Cook --- scripts/Makefile.lib | 3 +++ scripts/Makefile.ubsan | 3 +++ 2 files changed, 6 insertions(+) (limited to 'scripts') diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index 52efc520ae4f..b4a248c20654 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -177,6 +177,9 @@ ifeq ($(CONFIG_UBSAN),y) _c_flags += $(if $(patsubst n%,, \ $(UBSAN_SANITIZE_$(basetarget).o)$(UBSAN_SANITIZE)y), \ $(CFLAGS_UBSAN)) +_c_flags += $(if $(patsubst n%,, \ + $(UBSAN_SIGNED_WRAP_$(basetarget).o)$(UBSAN_SANITIZE_$(basetarget).o)$(UBSAN_SIGNED_WRAP)$(UBSAN_SANITIZE)y), \ + $(CFLAGS_UBSAN_SIGNED_WRAP)) endif ifeq ($(CONFIG_KCOV),y) diff --git a/scripts/Makefile.ubsan b/scripts/Makefile.ubsan index 7cf42231042b..b2d3b273b802 100644 --- a/scripts/Makefile.ubsan +++ b/scripts/Makefile.ubsan @@ -13,3 +13,6 @@ ubsan-cflags-$(CONFIG_UBSAN_ENUM) += -fsanitize=enum ubsan-cflags-$(CONFIG_UBSAN_TRAP) += $(call cc-option,-fsanitize-trap=undefined,-fsanitize-undefined-trap-on-error) export CFLAGS_UBSAN := $(ubsan-cflags-y) + +ubsan-signed-wrap-cflags-$(CONFIG_UBSAN_SIGNED_WRAP) += -fsanitize=signed-integer-overflow +export CFLAGS_UBSAN_SIGNED_WRAP := $(ubsan-signed-wrap-cflags-y) -- cgit From 1d02f252339e2eaf3ba35b9dc77e7a1a9aa7414c Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Thu, 15 Feb 2024 09:58:10 -0800 Subject: coccinelle: Add rules to find str_plural() replacements Add rules for finding places where str_plural() can be used. This currently finds: 54 files changed, 62 insertions(+), 61 deletions(-) Co-developed-by: Michal Wajdeczko Signed-off-by: Michal Wajdeczko Link: https://lore.kernel.org/all/fc1b25a8-6381-47c2-831c-ab6b8201a82b@intel.com/ Signed-off-by: Kees Cook --- scripts/coccinelle/api/string_choices.cocci | 41 +++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 scripts/coccinelle/api/string_choices.cocci (limited to 'scripts') diff --git a/scripts/coccinelle/api/string_choices.cocci b/scripts/coccinelle/api/string_choices.cocci new file mode 100644 index 000000000000..a71966c0494e --- /dev/null +++ b/scripts/coccinelle/api/string_choices.cocci @@ -0,0 +1,41 @@ +// SPDX-License-Identifier: GPL-2.0-only +/// Find places to use string_choices.h's various helpers. +// +// Confidence: Medium +// Options: --no-includes --include-headers +virtual patch +virtual context +virtual report + +@str_plural depends on patch@ +expression E; +@@ +( +- ((E == 1) ? "" : "s") ++ str_plural(E) +| +- ((E != 1) ? "s" : "") ++ str_plural(E) +| +- ((E > 1) ? "s" : "") ++ str_plural(E) +) + +@str_plural_r depends on !patch exists@ +expression E; +position P; +@@ +( +* ((E@P == 1) ? "" : "s") +| +* ((E@P != 1) ? "s" : "") +| +* ((E@P > 1) ? "s" : "") +) + +@script:python depends on report@ +p << str_plural_r.P; +e << str_plural_r.E; +@@ + +coccilib.report.print_report(p[0], "opportunity for str_plural(%s)" % e) -- cgit From e7549481255167dcdab355c539562c7ace17e111 Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Mon, 27 Feb 2023 12:24:28 -0800 Subject: coccinelle: semantic patch to check for potential struct_size calls include/linux/overflow.h includes helper macros intended for calculating sizes of allocations. These macros prevent accidental overflow by saturating at SIZE_MAX. In general when calculating such sizes use of the macros is preferred. Add a semantic patch which can detect code patterns which can be replaced by struct_size. Note that I set the confidence to medium because this patch doesn't make an attempt to ensure that the relevant array is actually a flexible array. The struct_size macro does specifically require a flexible array. In many cases the detected code could be refactored to a flexible array, but this is not always possible (such as if there are multiple over-allocations). Signed-off-by: Jacob Keller Link: https://lore.kernel.org/r/20230227202428.3657443-1-jacob.e.keller@intel.com Signed-off-by: Kees Cook --- scripts/coccinelle/misc/struct_size.cocci | 74 +++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 scripts/coccinelle/misc/struct_size.cocci (limited to 'scripts') diff --git a/scripts/coccinelle/misc/struct_size.cocci b/scripts/coccinelle/misc/struct_size.cocci new file mode 100644 index 000000000000..9b02c37438e4 --- /dev/null +++ b/scripts/coccinelle/misc/struct_size.cocci @@ -0,0 +1,74 @@ +// SPDX-License-Identifier: GPL-2.0-only +/// +/// Check for code that could use struct_size(). +/// +// Confidence: Medium +// Author: Jacob Keller +// Copyright: (C) 2023 Intel Corporation +// Options: --no-includes --include-headers + +virtual patch +virtual context +virtual org +virtual report + +// the overflow Kunit tests have some code which intentionally does not use +// the macros, so we want to ignore this code when reporting potential +// issues. +@overflow_tests@ +identifier f = overflow_size_helpers_test; +@@ + +f + +//---------------------------------------------------------- +// For context mode +//---------------------------------------------------------- + +@depends on !overflow_tests && context@ +expression E1, E2; +identifier m; +@@ +( +* (sizeof(*E1) + (E2 * sizeof(*E1->m))) +) + +//---------------------------------------------------------- +// For patch mode +//---------------------------------------------------------- + +@depends on !overflow_tests && patch@ +expression E1, E2; +identifier m; +@@ +( +- (sizeof(*E1) + (E2 * sizeof(*E1->m))) ++ struct_size(E1, m, E2) +) + +//---------------------------------------------------------- +// For org and report mode +//---------------------------------------------------------- + +@r depends on !overflow_tests && (org || report)@ +expression E1, E2; +identifier m; +position p; +@@ +( + (sizeof(*E1)@p + (E2 * sizeof(*E1->m))) +) + +@script:python depends on org@ +p << r.p; +@@ + +coccilib.org.print_todo(p[0], "WARNING should use struct_size") + +@script:python depends on report@ +p << r.p; +@@ + +msg="WARNING: Use struct_size" +coccilib.report.print_report(p[0], msg) + -- cgit From 1b1bcbf454f8e422c1e8e36bb21d726c39833576 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Thu, 22 Feb 2024 14:00:49 -0800 Subject: leaking_addresses: Use File::Temp for /tmp files Instead of using a statically named path in /tmp, use File::Temp to create (and remove) the temporary file used for parsing /proc/config.gz. Reviewed-by: Tycho Andersen Link: https://lore.kernel.org/r/20240222220053.1475824-2-keescook@chromium.org Signed-off-by: Kees Cook --- scripts/leaking_addresses.pl | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) (limited to 'scripts') diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl index e695634d153d..dd05fbcf15c5 100755 --- a/scripts/leaking_addresses.pl +++ b/scripts/leaking_addresses.pl @@ -23,6 +23,7 @@ use strict; use POSIX; use File::Basename; use File::Spec; +use File::Temp qw/tempfile/; use Cwd 'abs_path'; use Term::ANSIColor qw(:constants); use Getopt::Long qw(:config no_auto_abbrev); @@ -221,6 +222,7 @@ sub get_kernel_config_option { my ($option) = @_; my $value = ""; + my $tmp_fh; my $tmp_file = ""; my @config_files; @@ -228,7 +230,8 @@ sub get_kernel_config_option if ($kernel_config_file ne "") { @config_files = ($kernel_config_file); } elsif (-R "/proc/config.gz") { - my $tmp_file = "/tmp/tmpkconf"; + ($tmp_fh, $tmp_file) = tempfile("config.gz-XXXXXX", + UNLINK => 1); if (system("gunzip < /proc/config.gz > $tmp_file")) { dprint("system(gunzip < /proc/config.gz) failed\n"); @@ -250,10 +253,6 @@ sub get_kernel_config_option } } - if ($tmp_file ne "") { - system("rm -f $tmp_file"); - } - return $value; } -- cgit From 3e389d457badb1dc07f9fb3197bd7cb5c2833e36 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Thu, 22 Feb 2024 14:00:50 -0800 Subject: leaking_addresses: Ignore input device status lines These are false positives from the input subsystem: /proc/bus/input/devices: B: KEY=402000000 3803078f800d001 feffffdfffefffff fffffffffffffffe /sys/devices/platform/i8042/serio0/input/input1/uevent: KEY=402000000 3803078f800d001 feffffdfffefffff fffffffffffffffe /sys/devices/platform/i8042/serio0/input/input1/capabilities/key: 402000000 3803078f800d001 feffffdf Pass in the filename for more context and expand the "ignored pattern" matcher to notice these. Reviewed-by: Tycho Andersen Link: https://lore.kernel.org/r/20240222220053.1475824-3-keescook@chromium.org Signed-off-by: Kees Cook --- scripts/leaking_addresses.pl | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) (limited to 'scripts') diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl index dd05fbcf15c5..73cfcc5c8854 100755 --- a/scripts/leaking_addresses.pl +++ b/scripts/leaking_addresses.pl @@ -284,9 +284,10 @@ sub is_false_positive return is_false_positive_32bit($match); } - # 64 bit false positives. - - if ($match =~ '\b(0x)?(f|F){16}\b' or + # Ignore 64 bit false positives: + # 0xfffffffffffffff[0-f] + # 0x0000000000000000 + if ($match =~ '\b(0x)?(f|F){15}[0-9a-f]\b' or $match =~ '\b(0x)?0{16}\b') { return 1; } @@ -303,7 +304,7 @@ sub is_false_positive_32bit my ($match) = @_; state $page_offset = get_page_offset(); - if ($match =~ '\b(0x)?(f|F){8}\b') { + if ($match =~ '\b(0x)?(f|F){7}[0-9a-f]\b') { return 1; } @@ -346,18 +347,23 @@ sub is_in_vsyscall_memory_region # True if argument potentially contains a kernel address. sub may_leak_address { - my ($line) = @_; + my ($path, $line) = @_; my $address_re; - # Signal masks. + # Ignore Signal masks. if ($line =~ '^SigBlk:' or $line =~ '^SigIgn:' or $line =~ '^SigCgt:') { return 0; } - if ($line =~ '\bKEY=[[:xdigit:]]{14} [[:xdigit:]]{16} [[:xdigit:]]{16}\b' or - $line =~ '\b[[:xdigit:]]{14} [[:xdigit:]]{16} [[:xdigit:]]{16}\b') { + # Ignore input device reporting. + # /proc/bus/input/devices: B: KEY=402000000 3803078f800d001 feffffdfffefffff fffffffffffffffe + # /sys/devices/platform/i8042/serio0/input/input1/uevent: KEY=402000000 3803078f800d001 feffffdfffefffff fffffffffffffffe + # /sys/devices/platform/i8042/serio0/input/input1/capabilities/key: 402000000 3803078f800d001 feffffdfffefffff fffffffffffffffe + if ($line =~ '\bKEY=[[:xdigit:]]{9,14} [[:xdigit:]]{16} [[:xdigit:]]{16}\b' or + ($path =~ '\bkey$' and + $line =~ '\b[[:xdigit:]]{9,14} [[:xdigit:]]{16} [[:xdigit:]]{16}\b')) { return 0; } @@ -400,7 +406,7 @@ sub parse_dmesg { open my $cmd, '-|', 'dmesg'; while (<$cmd>) { - if (may_leak_address($_)) { + if (may_leak_address("dmesg", $_)) { print 'dmesg: ' . $_; } } @@ -456,7 +462,7 @@ sub parse_file open my $fh, "<", $file or return; while ( <$fh> ) { chomp; - if (may_leak_address($_)) { + if (may_leak_address($file, $_)) { printf("$file: $_\n"); } } @@ -468,7 +474,7 @@ sub check_path_for_leaks { my ($path) = @_; - if (may_leak_address($path)) { + if (may_leak_address($path, $path)) { printf("Path name may contain address: $path\n"); } } -- cgit From 67bbd2f00735d7f5ad6c3d08eff6c5403c3a9c33 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Thu, 22 Feb 2024 14:00:51 -0800 Subject: leaking_addresses: Provide mechanism to scan binary files Introduce --kallsyms argument for scanning binary files for known symbol addresses. This would have found the exposure in /sys/kernel/notes: $ scripts/leaking_addresses.pl --kallsyms=<(sudo cat /proc/kallsyms) /sys/kernel/notes: hypercall_page @ 156 /sys/kernel/notes: xen_hypercall_set_trap_table @ 156 /sys/kernel/notes: startup_xen @ 132 Acked-by: Greg Kroah-Hartman Reviewed-by: Tycho Andersen Link: https://lore.kernel.org/r/20240222220053.1475824-4-keescook@chromium.org Signed-off-by: Kees Cook --- scripts/leaking_addresses.pl | 53 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) (limited to 'scripts') diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl index 73cfcc5c8854..8e992b18bcd9 100755 --- a/scripts/leaking_addresses.pl +++ b/scripts/leaking_addresses.pl @@ -52,10 +52,13 @@ my $input_raw = ""; # Read raw results from file instead of scanning. my $suppress_dmesg = 0; # Don't show dmesg in output. my $squash_by_path = 0; # Summary report grouped by absolute path. my $squash_by_filename = 0; # Summary report grouped by filename. +my $kallsyms_file = ""; # Kernel symbols file. my $kernel_config_file = ""; # Kernel configuration file. my $opt_32bit = 0; # Scan 32-bit kernel. my $page_offset_32bit = 0; # Page offset for 32-bit kernel. +my @kallsyms = (); + # Skip these absolute paths. my @skip_abs = ( '/proc/kmsg', @@ -96,6 +99,8 @@ Options: --squash-by-path Show one result per unique path. --squash-by-filename Show one result per unique filename. --kernel-config-file= Kernel configuration file (e.g /boot/config) + --kallsyms= Read kernel symbol addresses from file (for + scanning binary files). --32-bit Scan 32-bit kernel. --page-offset-32-bit=o Page offset (for 32-bit kernel 0xABCD1234). -d, --debug Display debugging output. @@ -116,6 +121,7 @@ GetOptions( 'squash-by-path' => \$squash_by_path, 'squash-by-filename' => \$squash_by_filename, 'raw' => \$raw, + 'kallsyms=s' => \$kallsyms_file, 'kernel-config-file=s' => \$kernel_config_file, '32-bit' => \$opt_32bit, 'page-offset-32-bit=o' => \$page_offset_32bit, @@ -156,6 +162,25 @@ if ($output_raw) { select $fh; } +if ($kallsyms_file) { + open my $fh, '<', $kallsyms_file or die "$0: $kallsyms_file: $!\n"; + while (<$fh>) { + chomp; + my @entry = split / /, $_; + my $addr_text = $entry[0]; + if ($addr_text !~ /^0/) { + # TODO: Why is hex() so impossibly slow? + my $addr = hex($addr_text); + my $symbol = $entry[2]; + # Only keep kernel text addresses. + my $long = pack("J", $addr); + my $entry = [$long, $symbol]; + push @kallsyms, $entry; + } + } + close $fh; +} + parse_dmesg(); walk(@DIRS); @@ -447,6 +472,25 @@ sub timed_parse_file } } +sub parse_binary +{ + my ($file) = @_; + + open my $fh, "<:raw", $file or return; + local $/ = undef; + my $bytes = <$fh>; + close $fh; + + foreach my $entry (@kallsyms) { + my $addr = $entry->[0]; + my $symbol = $entry->[1]; + my $offset = index($bytes, $addr); + if ($offset != -1) { + printf("$file: $symbol @ $offset\n"); + } + } +} + sub parse_file { my ($file) = @_; @@ -456,6 +500,15 @@ sub parse_file } if (! -T $file) { + if ($file =~ m|^/sys/kernel/btf/| or + $file =~ m|^/sys/devices/pci| or + $file =~ m|^/sys/firmware/efi/efivars/| or + $file =~ m|^/proc/bus/pci/|) { + return; + } + if (scalar @kallsyms > 0) { + parse_binary($file); + } return; } -- cgit