[PATCH] leaking_addresses: add support for 32-bit kernel addresses

[PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Tobin C. Harding]

Currently, leaking_addresses.pl only supports scanning 64 bit
architectures. This is due to how the regular expressions are formed. We
can do better than this. 32 architectures can be supported if we take
into consideration the kernel virtual address split.

Add support for ix86 32 bit architectures.
 - Add command line option for page offset.
 - Add command line option for kernel configuration file.
 - Parse kernel config file for page offset (CONFIG_PAGE_OFFSET).
 - Use page offset when checking for kernel virtual addresses.

Signed-off-by: Kaiwan N Billimoria <kaiwan.billimoria@gmail.com>
Signed-off-by: Tobin C. Harding <me@tobin.cc>
---

As discussed this is a patch based on Kaiwan's previous patch. This
patch represents co development by Kaiwan and Tobin.

Applies on top of commit 4fbd8d194f06 (Linux 4.15-rc1)

thanks,
Tobin.

 scripts/leaking_addresses.pl | 168 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 148 insertions(+), 20 deletions(-)

diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
index bc5788000018..f03f2f140e0a 100755
--- a/scripts/leaking_addresses.pl
+++ b/scripts/leaking_addresses.pl
@@ -1,9 +1,11 @@
 #!/usr/bin/env perl
 #
 # (c) 2017 Tobin C. Harding <me@tobin.cc>
+# (c) 2017 Kaiwan N Billimoria <kaiwan.billimoria@gmail.com> (ix86 stuff)
+#
 # Licensed under the terms of the GNU GPL License version 2
 #
-# leaking_addresses.pl: Scan 64 bit kernel for potential leaking addresses.
+# leaking_addresses.pl: Scan the kernel for potential leaking addresses.
 #  - Scans dmesg output.
 #  - Walks directory tree and parses each file (for each directory in @DIRS).
 #
@@ -22,6 +24,7 @@ use Cwd 'abs_path';
 use Term::ANSIColor qw(:constants);
 use Getopt::Long qw(:config no_auto_abbrev);
 use Config;
+use feature 'state';
 
 my $P = $0;
 my $V = '0.01';
@@ -35,18 +38,19 @@ my $TIMEOUT = 10;
 # Script can only grep for kernel addresses on the following architectures. If
 # your architecture is not listed here and has a grep'able kernel address please
 # consider submitting a patch.
-my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64');
+my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64', 'i[3456]86');
 
 # Command line options.
 my $help = 0;
 my $debug = 0;
-my $raw = 0;
-my $output_raw = "";	# Write raw results to file.
-my $input_raw = "";	# Read raw results from file instead of scanning.
-
+my $raw = 0;			# Show raw output.
+my $output_raw = "";		# Write raw results to file.
+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 $page_offset_32bit = 0;	# 32-bit: value of CONFIG_PAGE_OFFSET
+my $kernel_config_file = "";	# Kernel configuration file.
 
 # Do not parse these files (absolute path).
 my @skip_parse_files_abs = ('/proc/kmsg',
@@ -95,14 +99,16 @@ Version: $V
 
 Options:
 
-	-o, --output-raw=<file>  Save results for future processing.
-	-i, --input-raw=<file>   Read results from file instead of scanning.
-	    --raw                Show raw results (default).
-	    --suppress-dmesg     Do not show dmesg results.
-	    --squash-by-path     Show one result per unique path.
-	    --squash-by-filename Show one result per unique filename.
-	-d, --debug              Display debugging output.
-	-h, --help, --version    Display this help and exit.
+	-o, --output-raw=<file>		Save results for future processing.
+	-i, --input-raw=<file>		Read results from file instead of scanning.
+	      --raw			  Show raw results (default).
+	      --suppress-dmesg            Do not show dmesg results.
+	      --squash-by-path            Show one result per unique path.
+	      --squash-by-filename        Show one result per unique filename.
+	    --page-offset-32bit=<hex>	PAGE_OFFSET value (for 32-bit kernels).
+	    --kernel-config-file=<file>	Kernel configuration file (e.g /boot/config)
+	-d, --debug			Display debugging output.
+	-h, --help, --version		Display this help and exit.
 
 Examples:
 
@@ -115,7 +121,10 @@ Examples:
 	# View summary report.
 	$0 --input-raw scan.out --squash-by-filename
 
-Scans the running (64 bit) kernel for potential leaking addresses.
+	# Scan kernel on a 32-bit system with a 2GB:2GB virtual address split.
+	$0 --page-offset-32bit=0x80000000
+
+Scans the running kernel for potential leaking addresses.
 
 EOM
 	exit($exitcode);
@@ -131,6 +140,8 @@ GetOptions(
 	'squash-by-path'        => \$squash_by_path,
 	'squash-by-filename'    => \$squash_by_filename,
 	'raw'                   => \$raw,
+	'page-offset-32bit=o'   => \$page_offset_32bit,
+	'kernel-config-file=s'  => \$kernel_config_file,
 ) or help(1);
 
 help(0) if ($help);
@@ -146,7 +157,9 @@ if (!$input_raw and ($squash_by_path or $squash_by_filename)) {
 	exit(128);
 }
 
-if (!is_supported_architecture()) {
+if (is_supported_architecture()) {
+	show_detected_architecture() if $debug;
+} else {
 	printf "\nScript does not support your architecture, sorry.\n";
 	printf "\nCurrently we support: \n\n";
 	foreach(@SUPPORTED_ARCHITECTURES) {
@@ -177,7 +190,7 @@ sub dprint
 
 sub is_supported_architecture
 {
-	return (is_x86_64() or is_ppc64());
+	return (is_x86_64() or is_ppc64() or is_ix86_32());
 }
 
 sub is_x86_64
@@ -200,10 +213,40 @@ sub is_ppc64
 	return 0;
 }
 
+sub is_ix86_32
+{
+	my $archname = $Config{archname};
+
+	if ($archname =~ m/i[3456]86-linux/) {
+		return 1;
+	}
+	return 0;
+}
+
+sub show_detected_architecture
+{
+	printf "Detected architecture: ";
+	if (is_ix86_32()) {
+		printf "32 bit x86\n";
+	} elsif (is_x86_64()) {
+		printf "x86_64\n";
+	} elsif (is_ppc64()) {
+		printf "ppc64\n";
+	} else {
+		printf "failed to detect architecture\n"
+	}
+}
+
 sub is_false_positive
 {
 	my ($match) = @_;
 
+	if (is_ix86_32()) {
+		return is_false_positive_ix86_32($match);
+	}
+
+	# 64 bit architectures
+
 	if ($match =~ '\b(0x)?(f|F){16}\b' or
 	    $match =~ '\b(0x)?0{16}\b') {
 		return 1;
@@ -220,6 +263,87 @@ sub is_false_positive
 	return 0;
 }
 
+sub is_false_positive_ix86_32
+{
+	my ($match) = @_;
+	state $page_offset = get_page_offset(); # only gets called once
+
+	if ($match =~ '\b(0x)?(f|F){8}\b') {
+		return 1;
+	}
+
+	my $addr32 = eval hex($match);
+	if ($addr32 < $page_offset) {
+		return 1;
+	}
+
+	return 0;
+}
+
+sub get_page_offset
+{
+	my $page_offset;
+	my $default_offset = "0xc0000000";
+	my @config_files;
+
+	# Allow --page-offset-32bit to over ride.
+	if ($page_offset_32bit != 0) {
+		return $page_offset_32bit;
+	}
+
+	# Allow --kernel-config-file to over ride.
+	if ($kernel_config_file != "") {
+		@config_files = ($kernel_config_file);
+	} else {
+		my $config_file = '/boot/config-' . `uname -r`;
+		@config_files = ($config_file, '/boot/config');
+	}
+
+	if (-R "/proc/config.gz") {
+		my $tmp_file = "/tmp/tmpkconf";
+		if (system("gunzip < /proc/config.gz > $tmp_file")) {
+			dprint " parse_kernel_config: system(gunzip...) failed\n";
+		} else {
+			$page_offset = parse_kernel_config_file($tmp_file);
+			if ($page_offset ne "") {
+				return $page_offset;
+			}
+		}
+		system("rm -f $tmp_file");
+	}
+
+	foreach my $config_file (@config_files) {
+		$page_offset = parse_kernel_config($config_file);
+		if ($page_offset ne "") {
+			return $page_offset;
+		}
+	}
+
+	printf STDERR "Failed to parse kernel config files\n";
+	printf STDERR "Falling back to %s\n", $default_offset;
+	return $default_offset;
+}
+
+sub parse_kernel_config_file
+{
+	my ($file) = @_;
+	my $config = 'CONFIG_PAGE_OFFSET';
+	my $val = "";
+
+	open(my $fh, "<", $file) or return "";
+	while (my $line = <$fh> ) {
+		if ($line =~ /^$config/) {
+			my ($str, $val) = split /=/, $line;
+			chomp($val);
+			last;
+		}
+	}
+
+	close $fh;
+	return $val;
+}
+
+
 # True if argument potentially contains a kernel address.
 sub may_leak_address
 {
@@ -233,9 +357,11 @@ sub may_leak_address
 		return 0;
 	}
 
-	if ($line =~ '\bKEY=[[:xdigit:]]{14} [[:xdigit:]]{16} [[:xdigit:]]{16}\b' or
-	    $line =~ '\b[[:xdigit:]]{14} [[:xdigit:]]{16} [[:xdigit:]]{16}\b') {
-		return 0;
+	if (is_x86_64() or is_ppc64()) {
+		if ($line =~ '\bKEY=[[:xdigit:]]{14} [[:xdigit:]]{16} [[:xdigit:]]{16}\b' or
+		    $line =~ '\b[[:xdigit:]]{14} [[:xdigit:]]{16} [[:xdigit:]]{16}\b') {
+			return 0;
+		}
 	}
 
 	# One of these is guaranteed to be true.
@@ -243,6 +369,8 @@ sub may_leak_address
 		$address_re = '\b(0x)?ffff[[:xdigit:]]{12}\b';
 	} elsif (is_ppc64()) {
 		$address_re = '\b(0x)?[89abcdef]00[[:xdigit:]]{13}\b';
+	} elsif (is_ix86_32()) {
+		$address_re = '\b(0x)?[[:xdigit:]]{8}\b';
 	}
 
 	while (/($address_re)/g) {
-- 
2.7.4

[kernel-hardening] [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Tobin C. Harding]

Currently, leaking_addresses.pl only supports scanning 64 bit
architectures. This is due to how the regular expressions are formed. We
can do better than this. 32 architectures can be supported if we take
into consideration the kernel virtual address split.

Add support for ix86 32 bit architectures.
 - Add command line option for page offset.
 - Add command line option for kernel configuration file.
 - Parse kernel config file for page offset (CONFIG_PAGE_OFFSET).
 - Use page offset when checking for kernel virtual addresses.

Signed-off-by: Kaiwan N Billimoria <kaiwan.billimoria@gmail.com>
Signed-off-by: Tobin C. Harding <me@tobin.cc>
---

As discussed this is a patch based on Kaiwan's previous patch. This
patch represents co development by Kaiwan and Tobin.

Applies on top of commit 4fbd8d194f06 (Linux 4.15-rc1)

thanks,
Tobin.

 scripts/leaking_addresses.pl | 168 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 148 insertions(+), 20 deletions(-)

diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
index bc5788000018..f03f2f140e0a 100755
--- a/scripts/leaking_addresses.pl
+++ b/scripts/leaking_addresses.pl
@@ -1,9 +1,11 @@
 #!/usr/bin/env perl
 #
 # (c) 2017 Tobin C. Harding <me@tobin.cc>
+# (c) 2017 Kaiwan N Billimoria <kaiwan.billimoria@gmail.com> (ix86 stuff)
+#
 # Licensed under the terms of the GNU GPL License version 2
 #
-# leaking_addresses.pl: Scan 64 bit kernel for potential leaking addresses.
+# leaking_addresses.pl: Scan the kernel for potential leaking addresses.
 #  - Scans dmesg output.
 #  - Walks directory tree and parses each file (for each directory in @DIRS).
 #
@@ -22,6 +24,7 @@ use Cwd 'abs_path';
 use Term::ANSIColor qw(:constants);
 use Getopt::Long qw(:config no_auto_abbrev);
 use Config;
+use feature 'state';
 
 my $P = $0;
 my $V = '0.01';
@@ -35,18 +38,19 @@ my $TIMEOUT = 10;
 # Script can only grep for kernel addresses on the following architectures. If
 # your architecture is not listed here and has a grep'able kernel address please
 # consider submitting a patch.
-my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64');
+my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64', 'i[3456]86');
 
 # Command line options.
 my $help = 0;
 my $debug = 0;
-my $raw = 0;
-my $output_raw = "";	# Write raw results to file.
-my $input_raw = "";	# Read raw results from file instead of scanning.
-
+my $raw = 0;			# Show raw output.
+my $output_raw = "";		# Write raw results to file.
+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 $page_offset_32bit = 0;	# 32-bit: value of CONFIG_PAGE_OFFSET
+my $kernel_config_file = "";	# Kernel configuration file.
 
 # Do not parse these files (absolute path).
 my @skip_parse_files_abs = ('/proc/kmsg',
@@ -95,14 +99,16 @@ Version: $V
 
 Options:
 
-	-o, --output-raw=<file>  Save results for future processing.
-	-i, --input-raw=<file>   Read results from file instead of scanning.
-	    --raw                Show raw results (default).
-	    --suppress-dmesg     Do not show dmesg results.
-	    --squash-by-path     Show one result per unique path.
-	    --squash-by-filename Show one result per unique filename.
-	-d, --debug              Display debugging output.
-	-h, --help, --version    Display this help and exit.
+	-o, --output-raw=<file>		Save results for future processing.
+	-i, --input-raw=<file>		Read results from file instead of scanning.
+	      --raw			  Show raw results (default).
+	      --suppress-dmesg            Do not show dmesg results.
+	      --squash-by-path            Show one result per unique path.
+	      --squash-by-filename        Show one result per unique filename.
+	    --page-offset-32bit=<hex>	PAGE_OFFSET value (for 32-bit kernels).
+	    --kernel-config-file=<file>	Kernel configuration file (e.g /boot/config)
+	-d, --debug			Display debugging output.
+	-h, --help, --version		Display this help and exit.
 
 Examples:
 
@@ -115,7 +121,10 @@ Examples:
 	# View summary report.
 	$0 --input-raw scan.out --squash-by-filename
 
-Scans the running (64 bit) kernel for potential leaking addresses.
+	# Scan kernel on a 32-bit system with a 2GB:2GB virtual address split.
+	$0 --page-offset-32bit=0x80000000
+
+Scans the running kernel for potential leaking addresses.
 
 EOM
 	exit($exitcode);
@@ -131,6 +140,8 @@ GetOptions(
 	'squash-by-path'        => \$squash_by_path,
 	'squash-by-filename'    => \$squash_by_filename,
 	'raw'                   => \$raw,
+	'page-offset-32bit=o'   => \$page_offset_32bit,
+	'kernel-config-file=s'  => \$kernel_config_file,
 ) or help(1);
 
 help(0) if ($help);
@@ -146,7 +157,9 @@ if (!$input_raw and ($squash_by_path or $squash_by_filename)) {
 	exit(128);
 }
 
-if (!is_supported_architecture()) {
+if (is_supported_architecture()) {
+	show_detected_architecture() if $debug;
+} else {
 	printf "\nScript does not support your architecture, sorry.\n";
 	printf "\nCurrently we support: \n\n";
 	foreach(@SUPPORTED_ARCHITECTURES) {
@@ -177,7 +190,7 @@ sub dprint
 
 sub is_supported_architecture
 {
-	return (is_x86_64() or is_ppc64());
+	return (is_x86_64() or is_ppc64() or is_ix86_32());
 }
 
 sub is_x86_64
@@ -200,10 +213,40 @@ sub is_ppc64
 	return 0;
 }
 
+sub is_ix86_32
+{
+	my $archname = $Config{archname};
+
+	if ($archname =~ m/i[3456]86-linux/) {
+		return 1;
+	}
+	return 0;
+}
+
+sub show_detected_architecture
+{
+	printf "Detected architecture: ";
+	if (is_ix86_32()) {
+		printf "32 bit x86\n";
+	} elsif (is_x86_64()) {
+		printf "x86_64\n";
+	} elsif (is_ppc64()) {
+		printf "ppc64\n";
+	} else {
+		printf "failed to detect architecture\n"
+	}
+}
+
 sub is_false_positive
 {
 	my ($match) = @_;
 
+	if (is_ix86_32()) {
+		return is_false_positive_ix86_32($match);
+	}
+
+	# 64 bit architectures
+
 	if ($match =~ '\b(0x)?(f|F){16}\b' or
 	    $match =~ '\b(0x)?0{16}\b') {
 		return 1;
@@ -220,6 +263,87 @@ sub is_false_positive
 	return 0;
 }
 
+sub is_false_positive_ix86_32
+{
+	my ($match) = @_;
+	state $page_offset = get_page_offset(); # only gets called once
+
+	if ($match =~ '\b(0x)?(f|F){8}\b') {
+		return 1;
+	}
+
+	my $addr32 = eval hex($match);
+	if ($addr32 < $page_offset) {
+		return 1;
+	}
+
+	return 0;
+}
+
+sub get_page_offset
+{
+	my $page_offset;
+	my $default_offset = "0xc0000000";
+	my @config_files;
+
+	# Allow --page-offset-32bit to over ride.
+	if ($page_offset_32bit != 0) {
+		return $page_offset_32bit;
+	}
+
+	# Allow --kernel-config-file to over ride.
+	if ($kernel_config_file != "") {
+		@config_files = ($kernel_config_file);
+	} else {
+		my $config_file = '/boot/config-' . `uname -r`;
+		@config_files = ($config_file, '/boot/config');
+	}
+
+	if (-R "/proc/config.gz") {
+		my $tmp_file = "/tmp/tmpkconf";
+		if (system("gunzip < /proc/config.gz > $tmp_file")) {
+			dprint " parse_kernel_config: system(gunzip...) failed\n";
+		} else {
+			$page_offset = parse_kernel_config_file($tmp_file);
+			if ($page_offset ne "") {
+				return $page_offset;
+			}
+		}
+		system("rm -f $tmp_file");
+	}
+
+	foreach my $config_file (@config_files) {
+		$page_offset = parse_kernel_config($config_file);
+		if ($page_offset ne "") {
+			return $page_offset;
+		}
+	}
+
+	printf STDERR "Failed to parse kernel config files\n";
+	printf STDERR "Falling back to %s\n", $default_offset;
+	return $default_offset;
+}
+
+sub parse_kernel_config_file
+{
+	my ($file) = @_;
+	my $config = 'CONFIG_PAGE_OFFSET';
+	my $val = "";
+
+	open(my $fh, "<", $file) or return "";
+	while (my $line = <$fh> ) {
+		if ($line =~ /^$config/) {
+			my ($str, $val) = split /=/, $line;
+			chomp($val);
+			last;
+		}
+	}
+
+	close $fh;
+	return $val;
+}
+
+
 # True if argument potentially contains a kernel address.
 sub may_leak_address
 {
@@ -233,9 +357,11 @@ sub may_leak_address
 		return 0;
 	}
 
-	if ($line =~ '\bKEY=[[:xdigit:]]{14} [[:xdigit:]]{16} [[:xdigit:]]{16}\b' or
-	    $line =~ '\b[[:xdigit:]]{14} [[:xdigit:]]{16} [[:xdigit:]]{16}\b') {
-		return 0;
+	if (is_x86_64() or is_ppc64()) {
+		if ($line =~ '\bKEY=[[:xdigit:]]{14} [[:xdigit:]]{16} [[:xdigit:]]{16}\b' or
+		    $line =~ '\b[[:xdigit:]]{14} [[:xdigit:]]{16} [[:xdigit:]]{16}\b') {
+			return 0;
+		}
 	}
 
 	# One of these is guaranteed to be true.
@@ -243,6 +369,8 @@ sub may_leak_address
 		$address_re = '\b(0x)?ffff[[:xdigit:]]{12}\b';
 	} elsif (is_ppc64()) {
 		$address_re = '\b(0x)?[89abcdef]00[[:xdigit:]]{13}\b';
+	} elsif (is_ix86_32()) {
+		$address_re = '\b(0x)?[[:xdigit:]]{8}\b';
 	}
 
 	while (/($address_re)/g) {
-- 
2.7.4

Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Alexander Kapshuk]

On Tue, Nov 28, 2017 at 8:32 AM, Tobin C. Harding <me@tobin.cc> wrote:
> Currently, leaking_addresses.pl only supports scanning 64 bit
> architectures. This is due to how the regular expressions are formed. We
> can do better than this. 32 architectures can be supported if we take
> into consideration the kernel virtual address split.
>
> Add support for ix86 32 bit architectures.
>  - Add command line option for page offset.
>  - Add command line option for kernel configuration file.
>  - Parse kernel config file for page offset (CONFIG_PAGE_OFFSET).
>  - Use page offset when checking for kernel virtual addresses.
>
> Signed-off-by: Kaiwan N Billimoria <kaiwan.billimoria@gmail.com>
> Signed-off-by: Tobin C. Harding <me@tobin.cc>
> ---
>
> As discussed this is a patch based on Kaiwan's previous patch. This
> patch represents co development by Kaiwan and Tobin.
>
> Applies on top of commit 4fbd8d194f06 (Linux 4.15-rc1)
>
> thanks,
> Tobin.
>
>  scripts/leaking_addresses.pl | 168 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 148 insertions(+), 20 deletions(-)
>
> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> index bc5788000018..f03f2f140e0a 100755
> --- a/scripts/leaking_addresses.pl
> +++ b/scripts/leaking_addresses.pl
> @@ -1,9 +1,11 @@
>  #!/usr/bin/env perl
>  #
>  # (c) 2017 Tobin C. Harding <me@tobin.cc>
> +# (c) 2017 Kaiwan N Billimoria <kaiwan.billimoria@gmail.com> (ix86 stuff)
> +#
>  # Licensed under the terms of the GNU GPL License version 2
>  #
> -# leaking_addresses.pl: Scan 64 bit kernel for potential leaking addresses.
> +# leaking_addresses.pl: Scan the kernel for potential leaking addresses.
>  #  - Scans dmesg output.
>  #  - Walks directory tree and parses each file (for each directory in @DIRS).
>  #
> @@ -22,6 +24,7 @@ use Cwd 'abs_path';
>  use Term::ANSIColor qw(:constants);
>  use Getopt::Long qw(:config no_auto_abbrev);
>  use Config;
> +use feature 'state';
>
>  my $P = $0;
>  my $V = '0.01';
> @@ -35,18 +38,19 @@ my $TIMEOUT = 10;
>  # Script can only grep for kernel addresses on the following architectures. If
>  # your architecture is not listed here and has a grep'able kernel address please
>  # consider submitting a patch.
> -my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64');
> +my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64', 'i[3456]86');
>
>  # Command line options.
>  my $help = 0;
>  my $debug = 0;
> -my $raw = 0;
> -my $output_raw = "";   # Write raw results to file.
> -my $input_raw = "";    # Read raw results from file instead of scanning.
> -
> +my $raw = 0;                   # Show raw output.
> +my $output_raw = "";           # Write raw results to file.
> +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 $page_offset_32bit = 0;     # 32-bit: value of CONFIG_PAGE_OFFSET
> +my $kernel_config_file = "";   # Kernel configuration file.
>
>  # Do not parse these files (absolute path).
>  my @skip_parse_files_abs = ('/proc/kmsg',
> @@ -95,14 +99,16 @@ Version: $V
>
>  Options:
>
> -       -o, --output-raw=<file>  Save results for future processing.
> -       -i, --input-raw=<file>   Read results from file instead of scanning.
> -           --raw                Show raw results (default).
> -           --suppress-dmesg     Do not show dmesg results.
> -           --squash-by-path     Show one result per unique path.
> -           --squash-by-filename Show one result per unique filename.
> -       -d, --debug              Display debugging output.
> -       -h, --help, --version    Display this help and exit.
> +       -o, --output-raw=<file>         Save results for future processing.
> +       -i, --input-raw=<file>          Read results from file instead of scanning.
> +             --raw                       Show raw results (default).
> +             --suppress-dmesg            Do not show dmesg results.
> +             --squash-by-path            Show one result per unique path.
> +             --squash-by-filename        Show one result per unique filename.
> +           --page-offset-32bit=<hex>   PAGE_OFFSET value (for 32-bit kernels).
> +           --kernel-config-file=<file> Kernel configuration file (e.g /boot/config)
> +       -d, --debug                     Display debugging output.
> +       -h, --help, --version           Display this help and exit.
>
>  Examples:
>
> @@ -115,7 +121,10 @@ Examples:
>         # View summary report.
>         $0 --input-raw scan.out --squash-by-filename
>
> -Scans the running (64 bit) kernel for potential leaking addresses.
> +       # Scan kernel on a 32-bit system with a 2GB:2GB virtual address split.
> +       $0 --page-offset-32bit=0x80000000
> +
> +Scans the running kernel for potential leaking addresses.
>
>  EOM
>         exit($exitcode);
> @@ -131,6 +140,8 @@ GetOptions(
>         'squash-by-path'        => \$squash_by_path,
>         'squash-by-filename'    => \$squash_by_filename,
>         'raw'                   => \$raw,
> +       'page-offset-32bit=o'   => \$page_offset_32bit,
> +       'kernel-config-file=s'  => \$kernel_config_file,
>  ) or help(1);
>
>  help(0) if ($help);
> @@ -146,7 +157,9 @@ if (!$input_raw and ($squash_by_path or $squash_by_filename)) {
>         exit(128);
>  }
>
> -if (!is_supported_architecture()) {
> +if (is_supported_architecture()) {
> +       show_detected_architecture() if $debug;
> +} else {
>         printf "\nScript does not support your architecture, sorry.\n";
>         printf "\nCurrently we support: \n\n";
>         foreach(@SUPPORTED_ARCHITECTURES) {
> @@ -177,7 +190,7 @@ sub dprint
>
>  sub is_supported_architecture
>  {
> -       return (is_x86_64() or is_ppc64());
> +       return (is_x86_64() or is_ppc64() or is_ix86_32());
>  }
>
>  sub is_x86_64
> @@ -200,10 +213,40 @@ sub is_ppc64
>         return 0;
>  }
>
> +sub is_ix86_32
> +{
> +       my $archname = $Config{archname};
> +
> +       if ($archname =~ m/i[3456]86-linux/) {
> +               return 1;
> +       }
> +       return 0;
> +}
> +
> +sub show_detected_architecture
> +{
> +       printf "Detected architecture: ";
> +       if (is_ix86_32()) {
> +               printf "32 bit x86\n";
> +       } elsif (is_x86_64()) {
> +               printf "x86_64\n";
> +       } elsif (is_ppc64()) {
> +               printf "ppc64\n";
> +       } else {
> +               printf "failed to detect architecture\n"
> +       }
> +}
> +
>  sub is_false_positive
>  {
>         my ($match) = @_;
>
> +       if (is_ix86_32()) {
> +               return is_false_positive_ix86_32($match);
> +       }
> +
> +       # 64 bit architectures
> +
>         if ($match =~ '\b(0x)?(f|F){16}\b' or
>             $match =~ '\b(0x)?0{16}\b') {
>                 return 1;
> @@ -220,6 +263,87 @@ sub is_false_positive
>         return 0;
>  }
>
> +sub is_false_positive_ix86_32
> +{
> +       my ($match) = @_;
> +       state $page_offset = get_page_offset(); # only gets called once
> +
> +       if ($match =~ '\b(0x)?(f|F){8}\b') {
> +               return 1;
> +       }
> +
> +       my $addr32 = eval hex($match);
> +       if ($addr32 < $page_offset) {
> +               return 1;
> +       }
> +
> +       return 0;
> +}
> +



> +sub get_page_offset
> +{
> +       my $page_offset;
> +       my $default_offset = "0xc0000000";
> +       my @config_files;
> +
> +       # Allow --page-offset-32bit to over ride.
> +       if ($page_offset_32bit != 0) {
> +               return $page_offset_32bit;
> +       }
> +
> +       # Allow --kernel-config-file to over ride.
> +       if ($kernel_config_file != "") {
> +               @config_files = ($kernel_config_file);
> +       } else {
> +               my $config_file = '/boot/config-' . `uname -r`;
> +               @config_files = ($config_file, '/boot/config');
> +       }
> +
> +       if (-R "/proc/config.gz") {
> +               my $tmp_file = "/tmp/tmpkconf";
> +               if (system("gunzip < /proc/config.gz > $tmp_file")) {
> +                       dprint " parse_kernel_config: system(gunzip...) failed\n";
> +               } else {
> +                       $page_offset = parse_kernel_config_file($tmp_file);
> +                       if ($page_offset ne "") {
> +                               return $page_offset;
> +                       }
> +               }
> +               system("rm -f $tmp_file");
> +       }
> +
> +       foreach my $config_file (@config_files) {
> +               $page_offset = parse_kernel_config($config_file);
> +               if ($page_offset ne "") {
> +                       return $page_offset;
> +               }
> +       }
> +
> +       printf STDERR "Failed to parse kernel config files\n";
> +       printf STDERR "Falling back to %s\n", $default_offset;
> +       return $default_offset;
> +}
> +
> +sub parse_kernel_config_file
> +{
> +       my ($file) = @_;
> +       my $config = 'CONFIG_PAGE_OFFSET';
> +       my $val = "";
> +
> +       open(my $fh, "<", $file) or return "";
> +       while (my $line = <$fh> ) {
> +               if ($line =~ /^$config/) {
> +                       my ($str, $val) = split /=/, $line;
> +                       chomp($val);
> +                       last;
> +               }
> +       }
> +
> +       close $fh;
> +       return $val;
> +}
> +
> +

Get_page_offset attempts to build a list of config files, which are
then passed into the parsing function for further processing.
This splits up the code to do with the config files between
get_page_offset() and parse_kernel_config_file().
May I suggest putting the kernel config file processing code into the
parse_kernel_config_file() instead, and let the parsing function
handle the config files and either return the page_offset or an empty
string.


See below for the proposed implementation. Apologies for the absence
of indentation.
Disclaimer: I did not test-run the code being proposed.

sub get_page_offset
{
my $default_offset = "0xc0000000";
my $page_offset;
# Allow --page-offset-32bit to over ride.
if ($page_offset_32bit != 0) {
return $page_offset_32bit;
}

if (($page_offset = parse_kernel_config_file()) != "") {
return $page_offset
}

printf STDERR "Failed to parse kernel config files\n";
printf STDERR "Falling back to %s\n", $default_offset;

return $default_offset;
}

sub parse_kernel_config_file
{
my @config_files;
my $config = 'CONFIG_PAGE_OFFSET';

# Allow --kernel-config-file to over ride.
if ($kernel_config_file != "") {
@config_files = ($kernel_config_file);
} else {
my $config_file = '/boot/config-' . `uname -r`;
@config_files = ($config_file, '/boot/config');
}

if (-R "/proc/config.gz") {
if (system("gunzip < /proc/config.gz > /tmp/tmpkconf") == 0) {
push @config_files, "/tmp/tmpkconf";
}
}

foreach my $config_file (@config_files) {
open(my $fh, "<", $config_file) or return "";
while (my $line = <$fh> ) {
if ($line =~ /^$config/) {
my ($config_name, $page_offset) = split /=/, $line;
chomp($page_offset);
last;
}
}
}
system("rm -f $tmp_file");
close $fh;

return $page_offset;
}

[kernel-hardening] Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Alexander Kapshuk]

On Tue, Nov 28, 2017 at 8:32 AM, Tobin C. Harding <me@tobin.cc> wrote:
> Currently, leaking_addresses.pl only supports scanning 64 bit
> architectures. This is due to how the regular expressions are formed. We
> can do better than this. 32 architectures can be supported if we take
> into consideration the kernel virtual address split.
>
> Add support for ix86 32 bit architectures.
>  - Add command line option for page offset.
>  - Add command line option for kernel configuration file.
>  - Parse kernel config file for page offset (CONFIG_PAGE_OFFSET).
>  - Use page offset when checking for kernel virtual addresses.
>
> Signed-off-by: Kaiwan N Billimoria <kaiwan.billimoria@gmail.com>
> Signed-off-by: Tobin C. Harding <me@tobin.cc>
> ---
>
> As discussed this is a patch based on Kaiwan's previous patch. This
> patch represents co development by Kaiwan and Tobin.
>
> Applies on top of commit 4fbd8d194f06 (Linux 4.15-rc1)
>
> thanks,
> Tobin.
>
>  scripts/leaking_addresses.pl | 168 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 148 insertions(+), 20 deletions(-)
>
> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> index bc5788000018..f03f2f140e0a 100755
> --- a/scripts/leaking_addresses.pl
> +++ b/scripts/leaking_addresses.pl
> @@ -1,9 +1,11 @@
>  #!/usr/bin/env perl
>  #
>  # (c) 2017 Tobin C. Harding <me@tobin.cc>
> +# (c) 2017 Kaiwan N Billimoria <kaiwan.billimoria@gmail.com> (ix86 stuff)
> +#
>  # Licensed under the terms of the GNU GPL License version 2
>  #
> -# leaking_addresses.pl: Scan 64 bit kernel for potential leaking addresses.
> +# leaking_addresses.pl: Scan the kernel for potential leaking addresses.
>  #  - Scans dmesg output.
>  #  - Walks directory tree and parses each file (for each directory in @DIRS).
>  #
> @@ -22,6 +24,7 @@ use Cwd 'abs_path';
>  use Term::ANSIColor qw(:constants);
>  use Getopt::Long qw(:config no_auto_abbrev);
>  use Config;
> +use feature 'state';
>
>  my $P = $0;
>  my $V = '0.01';
> @@ -35,18 +38,19 @@ my $TIMEOUT = 10;
>  # Script can only grep for kernel addresses on the following architectures. If
>  # your architecture is not listed here and has a grep'able kernel address please
>  # consider submitting a patch.
> -my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64');
> +my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64', 'i[3456]86');
>
>  # Command line options.
>  my $help = 0;
>  my $debug = 0;
> -my $raw = 0;
> -my $output_raw = "";   # Write raw results to file.
> -my $input_raw = "";    # Read raw results from file instead of scanning.
> -
> +my $raw = 0;                   # Show raw output.
> +my $output_raw = "";           # Write raw results to file.
> +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 $page_offset_32bit = 0;     # 32-bit: value of CONFIG_PAGE_OFFSET
> +my $kernel_config_file = "";   # Kernel configuration file.
>
>  # Do not parse these files (absolute path).
>  my @skip_parse_files_abs = ('/proc/kmsg',
> @@ -95,14 +99,16 @@ Version: $V
>
>  Options:
>
> -       -o, --output-raw=<file>  Save results for future processing.
> -       -i, --input-raw=<file>   Read results from file instead of scanning.
> -           --raw                Show raw results (default).
> -           --suppress-dmesg     Do not show dmesg results.
> -           --squash-by-path     Show one result per unique path.
> -           --squash-by-filename Show one result per unique filename.
> -       -d, --debug              Display debugging output.
> -       -h, --help, --version    Display this help and exit.
> +       -o, --output-raw=<file>         Save results for future processing.
> +       -i, --input-raw=<file>          Read results from file instead of scanning.
> +             --raw                       Show raw results (default).
> +             --suppress-dmesg            Do not show dmesg results.
> +             --squash-by-path            Show one result per unique path.
> +             --squash-by-filename        Show one result per unique filename.
> +           --page-offset-32bit=<hex>   PAGE_OFFSET value (for 32-bit kernels).
> +           --kernel-config-file=<file> Kernel configuration file (e.g /boot/config)
> +       -d, --debug                     Display debugging output.
> +       -h, --help, --version           Display this help and exit.
>
>  Examples:
>
> @@ -115,7 +121,10 @@ Examples:
>         # View summary report.
>         $0 --input-raw scan.out --squash-by-filename
>
> -Scans the running (64 bit) kernel for potential leaking addresses.
> +       # Scan kernel on a 32-bit system with a 2GB:2GB virtual address split.
> +       $0 --page-offset-32bit=0x80000000
> +
> +Scans the running kernel for potential leaking addresses.
>
>  EOM
>         exit($exitcode);
> @@ -131,6 +140,8 @@ GetOptions(
>         'squash-by-path'        => \$squash_by_path,
>         'squash-by-filename'    => \$squash_by_filename,
>         'raw'                   => \$raw,
> +       'page-offset-32bit=o'   => \$page_offset_32bit,
> +       'kernel-config-file=s'  => \$kernel_config_file,
>  ) or help(1);
>
>  help(0) if ($help);
> @@ -146,7 +157,9 @@ if (!$input_raw and ($squash_by_path or $squash_by_filename)) {
>         exit(128);
>  }
>
> -if (!is_supported_architecture()) {
> +if (is_supported_architecture()) {
> +       show_detected_architecture() if $debug;
> +} else {
>         printf "\nScript does not support your architecture, sorry.\n";
>         printf "\nCurrently we support: \n\n";
>         foreach(@SUPPORTED_ARCHITECTURES) {
> @@ -177,7 +190,7 @@ sub dprint
>
>  sub is_supported_architecture
>  {
> -       return (is_x86_64() or is_ppc64());
> +       return (is_x86_64() or is_ppc64() or is_ix86_32());
>  }
>
>  sub is_x86_64
> @@ -200,10 +213,40 @@ sub is_ppc64
>         return 0;
>  }
>
> +sub is_ix86_32
> +{
> +       my $archname = $Config{archname};
> +
> +       if ($archname =~ m/i[3456]86-linux/) {
> +               return 1;
> +       }
> +       return 0;
> +}
> +
> +sub show_detected_architecture
> +{
> +       printf "Detected architecture: ";
> +       if (is_ix86_32()) {
> +               printf "32 bit x86\n";
> +       } elsif (is_x86_64()) {
> +               printf "x86_64\n";
> +       } elsif (is_ppc64()) {
> +               printf "ppc64\n";
> +       } else {
> +               printf "failed to detect architecture\n"
> +       }
> +}
> +
>  sub is_false_positive
>  {
>         my ($match) = @_;
>
> +       if (is_ix86_32()) {
> +               return is_false_positive_ix86_32($match);
> +       }
> +
> +       # 64 bit architectures
> +
>         if ($match =~ '\b(0x)?(f|F){16}\b' or
>             $match =~ '\b(0x)?0{16}\b') {
>                 return 1;
> @@ -220,6 +263,87 @@ sub is_false_positive
>         return 0;
>  }
>
> +sub is_false_positive_ix86_32
> +{
> +       my ($match) = @_;
> +       state $page_offset = get_page_offset(); # only gets called once
> +
> +       if ($match =~ '\b(0x)?(f|F){8}\b') {
> +               return 1;
> +       }
> +
> +       my $addr32 = eval hex($match);
> +       if ($addr32 < $page_offset) {
> +               return 1;
> +       }
> +
> +       return 0;
> +}
> +



> +sub get_page_offset
> +{
> +       my $page_offset;
> +       my $default_offset = "0xc0000000";
> +       my @config_files;
> +
> +       # Allow --page-offset-32bit to over ride.
> +       if ($page_offset_32bit != 0) {
> +               return $page_offset_32bit;
> +       }
> +
> +       # Allow --kernel-config-file to over ride.
> +       if ($kernel_config_file != "") {
> +               @config_files = ($kernel_config_file);
> +       } else {
> +               my $config_file = '/boot/config-' . `uname -r`;
> +               @config_files = ($config_file, '/boot/config');
> +       }
> +
> +       if (-R "/proc/config.gz") {
> +               my $tmp_file = "/tmp/tmpkconf";
> +               if (system("gunzip < /proc/config.gz > $tmp_file")) {
> +                       dprint " parse_kernel_config: system(gunzip...) failed\n";
> +               } else {
> +                       $page_offset = parse_kernel_config_file($tmp_file);
> +                       if ($page_offset ne "") {
> +                               return $page_offset;
> +                       }
> +               }
> +               system("rm -f $tmp_file");
> +       }
> +
> +       foreach my $config_file (@config_files) {
> +               $page_offset = parse_kernel_config($config_file);
> +               if ($page_offset ne "") {
> +                       return $page_offset;
> +               }
> +       }
> +
> +       printf STDERR "Failed to parse kernel config files\n";
> +       printf STDERR "Falling back to %s\n", $default_offset;
> +       return $default_offset;
> +}
> +
> +sub parse_kernel_config_file
> +{
> +       my ($file) = @_;
> +       my $config = 'CONFIG_PAGE_OFFSET';
> +       my $val = "";
> +
> +       open(my $fh, "<", $file) or return "";
> +       while (my $line = <$fh> ) {
> +               if ($line =~ /^$config/) {
> +                       my ($str, $val) = split /=/, $line;
> +                       chomp($val);
> +                       last;
> +               }
> +       }
> +
> +       close $fh;
> +       return $val;
> +}
> +
> +

Get_page_offset attempts to build a list of config files, which are
then passed into the parsing function for further processing.
This splits up the code to do with the config files between
get_page_offset() and parse_kernel_config_file().
May I suggest putting the kernel config file processing code into the
parse_kernel_config_file() instead, and let the parsing function
handle the config files and either return the page_offset or an empty
string.


See below for the proposed implementation. Apologies for the absence
of indentation.
Disclaimer: I did not test-run the code being proposed.

sub get_page_offset
{
my $default_offset = "0xc0000000";
my $page_offset;
# Allow --page-offset-32bit to over ride.
if ($page_offset_32bit != 0) {
return $page_offset_32bit;
}

if (($page_offset = parse_kernel_config_file()) != "") {
return $page_offset
}

printf STDERR "Failed to parse kernel config files\n";
printf STDERR "Falling back to %s\n", $default_offset;

return $default_offset;
}

sub parse_kernel_config_file
{
my @config_files;
my $config = 'CONFIG_PAGE_OFFSET';

# Allow --kernel-config-file to over ride.
if ($kernel_config_file != "") {
@config_files = ($kernel_config_file);
} else {
my $config_file = '/boot/config-' . `uname -r`;
@config_files = ($config_file, '/boot/config');
}

if (-R "/proc/config.gz") {
if (system("gunzip < /proc/config.gz > /tmp/tmpkconf") == 0) {
push @config_files, "/tmp/tmpkconf";
}
}

foreach my $config_file (@config_files) {
open(my $fh, "<", $config_file) or return "";
while (my $line = <$fh> ) {
if ($line =~ /^$config/) {
my ($config_name, $page_offset) = split /=/, $line;
chomp($page_offset);
last;
}
}
}
system("rm -f $tmp_file");
close $fh;

return $page_offset;
}

Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Tobin C. Harding]

On Tue, Nov 28, 2017 at 03:16:24PM +0200, Alexander Kapshuk wrote:
> On Tue, Nov 28, 2017 at 8:32 AM, Tobin C. Harding <me@tobin.cc> wrote:
> > Currently, leaking_addresses.pl only supports scanning 64 bit
> > architectures. This is due to how the regular expressions are formed. We
> > can do better than this. 32 architectures can be supported if we take
> > into consideration the kernel virtual address split.
> >
> > Add support for ix86 32 bit architectures.
> >  - Add command line option for page offset.
> >  - Add command line option for kernel configuration file.
> >  - Parse kernel config file for page offset (CONFIG_PAGE_OFFSET).
> >  - Use page offset when checking for kernel virtual addresses.
> >
> > Signed-off-by: Kaiwan N Billimoria <kaiwan.billimoria@gmail.com>
> > Signed-off-by: Tobin C. Harding <me@tobin.cc>
> > ---
> >
> > As discussed this is a patch based on Kaiwan's previous patch. This
> > patch represents co development by Kaiwan and Tobin.
> >
> > Applies on top of commit 4fbd8d194f06 (Linux 4.15-rc1)
> >
> > thanks,
> > Tobin.
> >
> >  scripts/leaking_addresses.pl | 168 +++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 148 insertions(+), 20 deletions(-)
> >
> > diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> > index bc5788000018..f03f2f140e0a 100755
> > --- a/scripts/leaking_addresses.pl
> > +++ b/scripts/leaking_addresses.pl
> > @@ -1,9 +1,11 @@
> >  #!/usr/bin/env perl
> >  #
> >  # (c) 2017 Tobin C. Harding <me@tobin.cc>
> > +# (c) 2017 Kaiwan N Billimoria <kaiwan.billimoria@gmail.com> (ix86 stuff)
> > +#
> >  # Licensed under the terms of the GNU GPL License version 2
> >  #
> > -# leaking_addresses.pl: Scan 64 bit kernel for potential leaking addresses.
> > +# leaking_addresses.pl: Scan the kernel for potential leaking addresses.
> >  #  - Scans dmesg output.
> >  #  - Walks directory tree and parses each file (for each directory in @DIRS).
> >  #
> > @@ -22,6 +24,7 @@ use Cwd 'abs_path';
> >  use Term::ANSIColor qw(:constants);
> >  use Getopt::Long qw(:config no_auto_abbrev);
> >  use Config;
> > +use feature 'state';
> >
> >  my $P = $0;
> >  my $V = '0.01';
> > @@ -35,18 +38,19 @@ my $TIMEOUT = 10;
> >  # Script can only grep for kernel addresses on the following architectures. If
> >  # your architecture is not listed here and has a grep'able kernel address please
> >  # consider submitting a patch.
> > -my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64');
> > +my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64', 'i[3456]86');
> >
> >  # Command line options.
> >  my $help = 0;
> >  my $debug = 0;
> > -my $raw = 0;
> > -my $output_raw = "";   # Write raw results to file.
> > -my $input_raw = "";    # Read raw results from file instead of scanning.
> > -
> > +my $raw = 0;                   # Show raw output.
> > +my $output_raw = "";           # Write raw results to file.
> > +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 $page_offset_32bit = 0;     # 32-bit: value of CONFIG_PAGE_OFFSET
> > +my $kernel_config_file = "";   # Kernel configuration file.
> >
> >  # Do not parse these files (absolute path).
> >  my @skip_parse_files_abs = ('/proc/kmsg',
> > @@ -95,14 +99,16 @@ Version: $V
> >
> >  Options:
> >
> > -       -o, --output-raw=<file>  Save results for future processing.
> > -       -i, --input-raw=<file>   Read results from file instead of scanning.
> > -           --raw                Show raw results (default).
> > -           --suppress-dmesg     Do not show dmesg results.
> > -           --squash-by-path     Show one result per unique path.
> > -           --squash-by-filename Show one result per unique filename.
> > -       -d, --debug              Display debugging output.
> > -       -h, --help, --version    Display this help and exit.
> > +       -o, --output-raw=<file>         Save results for future processing.
> > +       -i, --input-raw=<file>          Read results from file instead of scanning.
> > +             --raw                       Show raw results (default).
> > +             --suppress-dmesg            Do not show dmesg results.
> > +             --squash-by-path            Show one result per unique path.
> > +             --squash-by-filename        Show one result per unique filename.
> > +           --page-offset-32bit=<hex>   PAGE_OFFSET value (for 32-bit kernels).
> > +           --kernel-config-file=<file> Kernel configuration file (e.g /boot/config)
> > +       -d, --debug                     Display debugging output.
> > +       -h, --help, --version           Display this help and exit.
> >
> >  Examples:
> >
> > @@ -115,7 +121,10 @@ Examples:
> >         # View summary report.
> >         $0 --input-raw scan.out --squash-by-filename
> >
> > -Scans the running (64 bit) kernel for potential leaking addresses.
> > +       # Scan kernel on a 32-bit system with a 2GB:2GB virtual address split.
> > +       $0 --page-offset-32bit=0x80000000
> > +
> > +Scans the running kernel for potential leaking addresses.
> >
> >  EOM
> >         exit($exitcode);
> > @@ -131,6 +140,8 @@ GetOptions(
> >         'squash-by-path'        => \$squash_by_path,
> >         'squash-by-filename'    => \$squash_by_filename,
> >         'raw'                   => \$raw,
> > +       'page-offset-32bit=o'   => \$page_offset_32bit,
> > +       'kernel-config-file=s'  => \$kernel_config_file,
> >  ) or help(1);
> >
> >  help(0) if ($help);
> > @@ -146,7 +157,9 @@ if (!$input_raw and ($squash_by_path or $squash_by_filename)) {
> >         exit(128);
> >  }
> >
> > -if (!is_supported_architecture()) {
> > +if (is_supported_architecture()) {
> > +       show_detected_architecture() if $debug;
> > +} else {
> >         printf "\nScript does not support your architecture, sorry.\n";
> >         printf "\nCurrently we support: \n\n";
> >         foreach(@SUPPORTED_ARCHITECTURES) {
> > @@ -177,7 +190,7 @@ sub dprint
> >
> >  sub is_supported_architecture
> >  {
> > -       return (is_x86_64() or is_ppc64());
> > +       return (is_x86_64() or is_ppc64() or is_ix86_32());
> >  }
> >
> >  sub is_x86_64
> > @@ -200,10 +213,40 @@ sub is_ppc64
> >         return 0;
> >  }
> >
> > +sub is_ix86_32
> > +{
> > +       my $archname = $Config{archname};
> > +
> > +       if ($archname =~ m/i[3456]86-linux/) {
> > +               return 1;
> > +       }
> > +       return 0;
> > +}
> > +
> > +sub show_detected_architecture
> > +{
> > +       printf "Detected architecture: ";
> > +       if (is_ix86_32()) {
> > +               printf "32 bit x86\n";
> > +       } elsif (is_x86_64()) {
> > +               printf "x86_64\n";
> > +       } elsif (is_ppc64()) {
> > +               printf "ppc64\n";
> > +       } else {
> > +               printf "failed to detect architecture\n"
> > +       }
> > +}
> > +
> >  sub is_false_positive
> >  {
> >         my ($match) = @_;
> >
> > +       if (is_ix86_32()) {
> > +               return is_false_positive_ix86_32($match);
> > +       }
> > +
> > +       # 64 bit architectures
> > +
> >         if ($match =~ '\b(0x)?(f|F){16}\b' or
> >             $match =~ '\b(0x)?0{16}\b') {
> >                 return 1;
> > @@ -220,6 +263,87 @@ sub is_false_positive
> >         return 0;
> >  }
> >
> > +sub is_false_positive_ix86_32
> > +{
> > +       my ($match) = @_;
> > +       state $page_offset = get_page_offset(); # only gets called once
> > +
> > +       if ($match =~ '\b(0x)?(f|F){8}\b') {
> > +               return 1;
> > +       }
> > +
> > +       my $addr32 = eval hex($match);
> > +       if ($addr32 < $page_offset) {
> > +               return 1;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> 
> 
> 
> > +sub get_page_offset
> > +{
> > +       my $page_offset;
> > +       my $default_offset = "0xc0000000";
> > +       my @config_files;
> > +
> > +       # Allow --page-offset-32bit to over ride.
> > +       if ($page_offset_32bit != 0) {
> > +               return $page_offset_32bit;
> > +       }
> > +
> > +       # Allow --kernel-config-file to over ride.
> > +       if ($kernel_config_file != "") {
> > +               @config_files = ($kernel_config_file);
> > +       } else {
> > +               my $config_file = '/boot/config-' . `uname -r`;
> > +               @config_files = ($config_file, '/boot/config');
> > +       }
> > +
> > +       if (-R "/proc/config.gz") {
> > +               my $tmp_file = "/tmp/tmpkconf";
> > +               if (system("gunzip < /proc/config.gz > $tmp_file")) {
> > +                       dprint " parse_kernel_config: system(gunzip...) failed\n";
> > +               } else {
> > +                       $page_offset = parse_kernel_config_file($tmp_file);
> > +                       if ($page_offset ne "") {
> > +                               return $page_offset;
> > +                       }
> > +               }
> > +               system("rm -f $tmp_file");
> > +       }
> > +
> > +       foreach my $config_file (@config_files) {
> > +               $page_offset = parse_kernel_config($config_file);
> > +               if ($page_offset ne "") {
> > +                       return $page_offset;
> > +               }
> > +       }
> > +
> > +       printf STDERR "Failed to parse kernel config files\n";
> > +       printf STDERR "Falling back to %s\n", $default_offset;
> > +       return $default_offset;
> > +}
> > +
> > +sub parse_kernel_config_file
> > +{
> > +       my ($file) = @_;
> > +       my $config = 'CONFIG_PAGE_OFFSET';
> > +       my $val = "";
> > +
> > +       open(my $fh, "<", $file) or return "";
> > +       while (my $line = <$fh> ) {
> > +               if ($line =~ /^$config/) {
> > +                       my ($str, $val) = split /=/, $line;
> > +                       chomp($val);
> > +                       last;
> > +               }
> > +       }
> > +
> > +       close $fh;
> > +       return $val;
> > +}
> > +
> > +
> 
> Get_page_offset attempts to build a list of config files, which are
> then passed into the parsing function for further processing.
> This splits up the code to do with the config files between
> get_page_offset() and parse_kernel_config_file().
> May I suggest putting the kernel config file processing code into the
> parse_kernel_config_file() instead, and let the parsing function
> handle the config files and either return the page_offset or an empty
> string.
>
> See below for the proposed implementation.

Nice, this is much better! Thanks.

> Apologies for the absence of indentation.

Re-posting with indentation, comments in line.

> Disclaimer: I did not test-run the code being proposed.

I also did not test my comments ;)

> sub get_page_offset
> {
> 	my $default_offset = "0xc0000000";
> 	my $page_offset;
> 
> 	# Allow --page-offset-32bit to over ride.
> 	if ($page_offset_32bit != 0) {
> 		return $page_offset_32bit;
> 	}
> 
> 	$page_offset = parse_kernel_config_file();
>     	if ($page_offset ne "") {
> 		return $page_offset
> 	}
> 
> 	printf STDERR "Failed to parse kernel config files\n";
> 	printf STDERR "Falling back to %s\n", $default_offset;
> 
> 	return $default_offset;
> }
> 
> sub parse_kernel_config_file
> {
> 	my @config_files;
> 	my $config = 'CONFIG_PAGE_OFFSET';
> 
> 	# Allow --kernel-config-file to over ride.
> 	if ($kernel_config_file != "") {
> 		@config_files = ($kernel_config_file);
> 	} else {
> 		my $config_file = '/boot/config-' . `uname -r`;
> 		@config_files = ($config_file, '/boot/config');
>	}
> 
> 	if (-R "/proc/config.gz") {

perhaps
		my $tmpkconf = '/tmp/tmpkconf';

> 		if (system("gunzip < /proc/config.gz > /tmp/tmpkconf") == 0) {
> 			push @config_files, "/tmp/tmpkconf";
> 		}
> 	}

Won't there only ever be a single config file? So if /proc/config.gz is
readable we could do
		
		@config_files = ($tmpkconf)

> 	foreach my $config_file (@config_files) {
> 		open(my $fh, "<", $config_file) or return "";

 		open(my $fh, "<", $config_file) or next;

> 		while (my $line = <$fh> ) {
>			if ($line =~ /^$config/) {
>				my ($config_name, $page_offset) = split /=/, $line;
>				chomp($page_offset);
>				last;
>			}
>		}
>	}
>	system("rm -f $tmp_file");
>	close $fh;
>
>	return $page_offset;
> }

thanks,
Tobin.

[kernel-hardening] Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Tobin C. Harding]

On Tue, Nov 28, 2017 at 03:16:24PM +0200, Alexander Kapshuk wrote:
> On Tue, Nov 28, 2017 at 8:32 AM, Tobin C. Harding <me@tobin.cc> wrote:
> > Currently, leaking_addresses.pl only supports scanning 64 bit
> > architectures. This is due to how the regular expressions are formed. We
> > can do better than this. 32 architectures can be supported if we take
> > into consideration the kernel virtual address split.
> >
> > Add support for ix86 32 bit architectures.
> >  - Add command line option for page offset.
> >  - Add command line option for kernel configuration file.
> >  - Parse kernel config file for page offset (CONFIG_PAGE_OFFSET).
> >  - Use page offset when checking for kernel virtual addresses.
> >
> > Signed-off-by: Kaiwan N Billimoria <kaiwan.billimoria@gmail.com>
> > Signed-off-by: Tobin C. Harding <me@tobin.cc>
> > ---
> >
> > As discussed this is a patch based on Kaiwan's previous patch. This
> > patch represents co development by Kaiwan and Tobin.
> >
> > Applies on top of commit 4fbd8d194f06 (Linux 4.15-rc1)
> >
> > thanks,
> > Tobin.
> >
> >  scripts/leaking_addresses.pl | 168 +++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 148 insertions(+), 20 deletions(-)
> >
> > diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> > index bc5788000018..f03f2f140e0a 100755
> > --- a/scripts/leaking_addresses.pl
> > +++ b/scripts/leaking_addresses.pl
> > @@ -1,9 +1,11 @@
> >  #!/usr/bin/env perl
> >  #
> >  # (c) 2017 Tobin C. Harding <me@tobin.cc>
> > +# (c) 2017 Kaiwan N Billimoria <kaiwan.billimoria@gmail.com> (ix86 stuff)
> > +#
> >  # Licensed under the terms of the GNU GPL License version 2
> >  #
> > -# leaking_addresses.pl: Scan 64 bit kernel for potential leaking addresses.
> > +# leaking_addresses.pl: Scan the kernel for potential leaking addresses.
> >  #  - Scans dmesg output.
> >  #  - Walks directory tree and parses each file (for each directory in @DIRS).
> >  #
> > @@ -22,6 +24,7 @@ use Cwd 'abs_path';
> >  use Term::ANSIColor qw(:constants);
> >  use Getopt::Long qw(:config no_auto_abbrev);
> >  use Config;
> > +use feature 'state';
> >
> >  my $P = $0;
> >  my $V = '0.01';
> > @@ -35,18 +38,19 @@ my $TIMEOUT = 10;
> >  # Script can only grep for kernel addresses on the following architectures. If
> >  # your architecture is not listed here and has a grep'able kernel address please
> >  # consider submitting a patch.
> > -my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64');
> > +my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64', 'i[3456]86');
> >
> >  # Command line options.
> >  my $help = 0;
> >  my $debug = 0;
> > -my $raw = 0;
> > -my $output_raw = "";   # Write raw results to file.
> > -my $input_raw = "";    # Read raw results from file instead of scanning.
> > -
> > +my $raw = 0;                   # Show raw output.
> > +my $output_raw = "";           # Write raw results to file.
> > +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 $page_offset_32bit = 0;     # 32-bit: value of CONFIG_PAGE_OFFSET
> > +my $kernel_config_file = "";   # Kernel configuration file.
> >
> >  # Do not parse these files (absolute path).
> >  my @skip_parse_files_abs = ('/proc/kmsg',
> > @@ -95,14 +99,16 @@ Version: $V
> >
> >  Options:
> >
> > -       -o, --output-raw=<file>  Save results for future processing.
> > -       -i, --input-raw=<file>   Read results from file instead of scanning.
> > -           --raw                Show raw results (default).
> > -           --suppress-dmesg     Do not show dmesg results.
> > -           --squash-by-path     Show one result per unique path.
> > -           --squash-by-filename Show one result per unique filename.
> > -       -d, --debug              Display debugging output.
> > -       -h, --help, --version    Display this help and exit.
> > +       -o, --output-raw=<file>         Save results for future processing.
> > +       -i, --input-raw=<file>          Read results from file instead of scanning.
> > +             --raw                       Show raw results (default).
> > +             --suppress-dmesg            Do not show dmesg results.
> > +             --squash-by-path            Show one result per unique path.
> > +             --squash-by-filename        Show one result per unique filename.
> > +           --page-offset-32bit=<hex>   PAGE_OFFSET value (for 32-bit kernels).
> > +           --kernel-config-file=<file> Kernel configuration file (e.g /boot/config)
> > +       -d, --debug                     Display debugging output.
> > +       -h, --help, --version           Display this help and exit.
> >
> >  Examples:
> >
> > @@ -115,7 +121,10 @@ Examples:
> >         # View summary report.
> >         $0 --input-raw scan.out --squash-by-filename
> >
> > -Scans the running (64 bit) kernel for potential leaking addresses.
> > +       # Scan kernel on a 32-bit system with a 2GB:2GB virtual address split.
> > +       $0 --page-offset-32bit=0x80000000
> > +
> > +Scans the running kernel for potential leaking addresses.
> >
> >  EOM
> >         exit($exitcode);
> > @@ -131,6 +140,8 @@ GetOptions(
> >         'squash-by-path'        => \$squash_by_path,
> >         'squash-by-filename'    => \$squash_by_filename,
> >         'raw'                   => \$raw,
> > +       'page-offset-32bit=o'   => \$page_offset_32bit,
> > +       'kernel-config-file=s'  => \$kernel_config_file,
> >  ) or help(1);
> >
> >  help(0) if ($help);
> > @@ -146,7 +157,9 @@ if (!$input_raw and ($squash_by_path or $squash_by_filename)) {
> >         exit(128);
> >  }
> >
> > -if (!is_supported_architecture()) {
> > +if (is_supported_architecture()) {
> > +       show_detected_architecture() if $debug;
> > +} else {
> >         printf "\nScript does not support your architecture, sorry.\n";
> >         printf "\nCurrently we support: \n\n";
> >         foreach(@SUPPORTED_ARCHITECTURES) {
> > @@ -177,7 +190,7 @@ sub dprint
> >
> >  sub is_supported_architecture
> >  {
> > -       return (is_x86_64() or is_ppc64());
> > +       return (is_x86_64() or is_ppc64() or is_ix86_32());
> >  }
> >
> >  sub is_x86_64
> > @@ -200,10 +213,40 @@ sub is_ppc64
> >         return 0;
> >  }
> >
> > +sub is_ix86_32
> > +{
> > +       my $archname = $Config{archname};
> > +
> > +       if ($archname =~ m/i[3456]86-linux/) {
> > +               return 1;
> > +       }
> > +       return 0;
> > +}
> > +
> > +sub show_detected_architecture
> > +{
> > +       printf "Detected architecture: ";
> > +       if (is_ix86_32()) {
> > +               printf "32 bit x86\n";
> > +       } elsif (is_x86_64()) {
> > +               printf "x86_64\n";
> > +       } elsif (is_ppc64()) {
> > +               printf "ppc64\n";
> > +       } else {
> > +               printf "failed to detect architecture\n"
> > +       }
> > +}
> > +
> >  sub is_false_positive
> >  {
> >         my ($match) = @_;
> >
> > +       if (is_ix86_32()) {
> > +               return is_false_positive_ix86_32($match);
> > +       }
> > +
> > +       # 64 bit architectures
> > +
> >         if ($match =~ '\b(0x)?(f|F){16}\b' or
> >             $match =~ '\b(0x)?0{16}\b') {
> >                 return 1;
> > @@ -220,6 +263,87 @@ sub is_false_positive
> >         return 0;
> >  }
> >
> > +sub is_false_positive_ix86_32
> > +{
> > +       my ($match) = @_;
> > +       state $page_offset = get_page_offset(); # only gets called once
> > +
> > +       if ($match =~ '\b(0x)?(f|F){8}\b') {
> > +               return 1;
> > +       }
> > +
> > +       my $addr32 = eval hex($match);
> > +       if ($addr32 < $page_offset) {
> > +               return 1;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> 
> 
> 
> > +sub get_page_offset
> > +{
> > +       my $page_offset;
> > +       my $default_offset = "0xc0000000";
> > +       my @config_files;
> > +
> > +       # Allow --page-offset-32bit to over ride.
> > +       if ($page_offset_32bit != 0) {
> > +               return $page_offset_32bit;
> > +       }
> > +
> > +       # Allow --kernel-config-file to over ride.
> > +       if ($kernel_config_file != "") {
> > +               @config_files = ($kernel_config_file);
> > +       } else {
> > +               my $config_file = '/boot/config-' . `uname -r`;
> > +               @config_files = ($config_file, '/boot/config');
> > +       }
> > +
> > +       if (-R "/proc/config.gz") {
> > +               my $tmp_file = "/tmp/tmpkconf";
> > +               if (system("gunzip < /proc/config.gz > $tmp_file")) {
> > +                       dprint " parse_kernel_config: system(gunzip...) failed\n";
> > +               } else {
> > +                       $page_offset = parse_kernel_config_file($tmp_file);
> > +                       if ($page_offset ne "") {
> > +                               return $page_offset;
> > +                       }
> > +               }
> > +               system("rm -f $tmp_file");
> > +       }
> > +
> > +       foreach my $config_file (@config_files) {
> > +               $page_offset = parse_kernel_config($config_file);
> > +               if ($page_offset ne "") {
> > +                       return $page_offset;
> > +               }
> > +       }
> > +
> > +       printf STDERR "Failed to parse kernel config files\n";
> > +       printf STDERR "Falling back to %s\n", $default_offset;
> > +       return $default_offset;
> > +}
> > +
> > +sub parse_kernel_config_file
> > +{
> > +       my ($file) = @_;
> > +       my $config = 'CONFIG_PAGE_OFFSET';
> > +       my $val = "";
> > +
> > +       open(my $fh, "<", $file) or return "";
> > +       while (my $line = <$fh> ) {
> > +               if ($line =~ /^$config/) {
> > +                       my ($str, $val) = split /=/, $line;
> > +                       chomp($val);
> > +                       last;
> > +               }
> > +       }
> > +
> > +       close $fh;
> > +       return $val;
> > +}
> > +
> > +
> 
> Get_page_offset attempts to build a list of config files, which are
> then passed into the parsing function for further processing.
> This splits up the code to do with the config files between
> get_page_offset() and parse_kernel_config_file().
> May I suggest putting the kernel config file processing code into the
> parse_kernel_config_file() instead, and let the parsing function
> handle the config files and either return the page_offset or an empty
> string.
>
> See below for the proposed implementation.

Nice, this is much better! Thanks.

> Apologies for the absence of indentation.

Re-posting with indentation, comments in line.

> Disclaimer: I did not test-run the code being proposed.

I also did not test my comments ;)

> sub get_page_offset
> {
> 	my $default_offset = "0xc0000000";
> 	my $page_offset;
> 
> 	# Allow --page-offset-32bit to over ride.
> 	if ($page_offset_32bit != 0) {
> 		return $page_offset_32bit;
> 	}
> 
> 	$page_offset = parse_kernel_config_file();
>     	if ($page_offset ne "") {
> 		return $page_offset
> 	}
> 
> 	printf STDERR "Failed to parse kernel config files\n";
> 	printf STDERR "Falling back to %s\n", $default_offset;
> 
> 	return $default_offset;
> }
> 
> sub parse_kernel_config_file
> {
> 	my @config_files;
> 	my $config = 'CONFIG_PAGE_OFFSET';
> 
> 	# Allow --kernel-config-file to over ride.
> 	if ($kernel_config_file != "") {
> 		@config_files = ($kernel_config_file);
> 	} else {
> 		my $config_file = '/boot/config-' . `uname -r`;
> 		@config_files = ($config_file, '/boot/config');
>	}
> 
> 	if (-R "/proc/config.gz") {

perhaps
		my $tmpkconf = '/tmp/tmpkconf';

> 		if (system("gunzip < /proc/config.gz > /tmp/tmpkconf") == 0) {
> 			push @config_files, "/tmp/tmpkconf";
> 		}
> 	}

Won't there only ever be a single config file? So if /proc/config.gz is
readable we could do
		
		@config_files = ($tmpkconf)

> 	foreach my $config_file (@config_files) {
> 		open(my $fh, "<", $config_file) or return "";

 		open(my $fh, "<", $config_file) or next;

> 		while (my $line = <$fh> ) {
>			if ($line =~ /^$config/) {
>				my ($config_name, $page_offset) = split /=/, $line;
>				chomp($page_offset);
>				last;
>			}
>		}
>	}
>	system("rm -f $tmp_file");
>	close $fh;
>
>	return $page_offset;
> }

thanks,
Tobin.

Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Alexander Kapshuk]

On Tue, Nov 28, 2017 at 11:10 PM, Tobin C. Harding <me@tobin.cc> wrote:
> On Tue, Nov 28, 2017 at 03:16:24PM +0200, Alexander Kapshuk wrote:
>> On Tue, Nov 28, 2017 at 8:32 AM, Tobin C. Harding <me@tobin.cc> wrote:
>> > Currently, leaking_addresses.pl only supports scanning 64 bit
>> > architectures. This is due to how the regular expressions are formed. We
>> > can do better than this. 32 architectures can be supported if we take
>> > into consideration the kernel virtual address split.
>> >
>> > Add support for ix86 32 bit architectures.
>> >  - Add command line option for page offset.
>> >  - Add command line option for kernel configuration file.
>> >  - Parse kernel config file for page offset (CONFIG_PAGE_OFFSET).
>> >  - Use page offset when checking for kernel virtual addresses.
>> >
>> > Signed-off-by: Kaiwan N Billimoria <kaiwan.billimoria@gmail.com>
>> > Signed-off-by: Tobin C. Harding <me@tobin.cc>
>> > ---
>> >
>> > As discussed this is a patch based on Kaiwan's previous patch. This
>> > patch represents co development by Kaiwan and Tobin.
>> >
>> > Applies on top of commit 4fbd8d194f06 (Linux 4.15-rc1)
>> >
>> > thanks,
>> > Tobin.
>> >
>> >  scripts/leaking_addresses.pl | 168 +++++++++++++++++++++++++++++++++++++------
>> >  1 file changed, 148 insertions(+), 20 deletions(-)
>> >
>> > diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
>> > index bc5788000018..f03f2f140e0a 100755
>> > --- a/scripts/leaking_addresses.pl
>> > +++ b/scripts/leaking_addresses.pl
>> > @@ -1,9 +1,11 @@
>> >  #!/usr/bin/env perl
>> >  #
>> >  # (c) 2017 Tobin C. Harding <me@tobin.cc>
>> > +# (c) 2017 Kaiwan N Billimoria <kaiwan.billimoria@gmail.com> (ix86 stuff)
>> > +#
>> >  # Licensed under the terms of the GNU GPL License version 2
>> >  #
>> > -# leaking_addresses.pl: Scan 64 bit kernel for potential leaking addresses.
>> > +# leaking_addresses.pl: Scan the kernel for potential leaking addresses.
>> >  #  - Scans dmesg output.
>> >  #  - Walks directory tree and parses each file (for each directory in @DIRS).
>> >  #
>> > @@ -22,6 +24,7 @@ use Cwd 'abs_path';
>> >  use Term::ANSIColor qw(:constants);
>> >  use Getopt::Long qw(:config no_auto_abbrev);
>> >  use Config;
>> > +use feature 'state';
>> >
>> >  my $P = $0;
>> >  my $V = '0.01';
>> > @@ -35,18 +38,19 @@ my $TIMEOUT = 10;
>> >  # Script can only grep for kernel addresses on the following architectures. If
>> >  # your architecture is not listed here and has a grep'able kernel address please
>> >  # consider submitting a patch.
>> > -my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64');
>> > +my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64', 'i[3456]86');
>> >
>> >  # Command line options.
>> >  my $help = 0;
>> >  my $debug = 0;
>> > -my $raw = 0;
>> > -my $output_raw = "";   # Write raw results to file.
>> > -my $input_raw = "";    # Read raw results from file instead of scanning.
>> > -
>> > +my $raw = 0;                   # Show raw output.
>> > +my $output_raw = "";           # Write raw results to file.
>> > +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 $page_offset_32bit = 0;     # 32-bit: value of CONFIG_PAGE_OFFSET
>> > +my $kernel_config_file = "";   # Kernel configuration file.
>> >
>> >  # Do not parse these files (absolute path).
>> >  my @skip_parse_files_abs = ('/proc/kmsg',
>> > @@ -95,14 +99,16 @@ Version: $V
>> >
>> >  Options:
>> >
>> > -       -o, --output-raw=<file>  Save results for future processing.
>> > -       -i, --input-raw=<file>   Read results from file instead of scanning.
>> > -           --raw                Show raw results (default).
>> > -           --suppress-dmesg     Do not show dmesg results.
>> > -           --squash-by-path     Show one result per unique path.
>> > -           --squash-by-filename Show one result per unique filename.
>> > -       -d, --debug              Display debugging output.
>> > -       -h, --help, --version    Display this help and exit.
>> > +       -o, --output-raw=<file>         Save results for future processing.
>> > +       -i, --input-raw=<file>          Read results from file instead of scanning.
>> > +             --raw                       Show raw results (default).
>> > +             --suppress-dmesg            Do not show dmesg results.
>> > +             --squash-by-path            Show one result per unique path.
>> > +             --squash-by-filename        Show one result per unique filename.
>> > +           --page-offset-32bit=<hex>   PAGE_OFFSET value (for 32-bit kernels).
>> > +           --kernel-config-file=<file> Kernel configuration file (e.g /boot/config)
>> > +       -d, --debug                     Display debugging output.
>> > +       -h, --help, --version           Display this help and exit.
>> >
>> >  Examples:
>> >
>> > @@ -115,7 +121,10 @@ Examples:
>> >         # View summary report.
>> >         $0 --input-raw scan.out --squash-by-filename
>> >
>> > -Scans the running (64 bit) kernel for potential leaking addresses.
>> > +       # Scan kernel on a 32-bit system with a 2GB:2GB virtual address split.
>> > +       $0 --page-offset-32bit=0x80000000
>> > +
>> > +Scans the running kernel for potential leaking addresses.
>> >
>> >  EOM
>> >         exit($exitcode);
>> > @@ -131,6 +140,8 @@ GetOptions(
>> >         'squash-by-path'        => \$squash_by_path,
>> >         'squash-by-filename'    => \$squash_by_filename,
>> >         'raw'                   => \$raw,
>> > +       'page-offset-32bit=o'   => \$page_offset_32bit,
>> > +       'kernel-config-file=s'  => \$kernel_config_file,
>> >  ) or help(1);
>> >
>> >  help(0) if ($help);
>> > @@ -146,7 +157,9 @@ if (!$input_raw and ($squash_by_path or $squash_by_filename)) {
>> >         exit(128);
>> >  }
>> >
>> > -if (!is_supported_architecture()) {
>> > +if (is_supported_architecture()) {
>> > +       show_detected_architecture() if $debug;
>> > +} else {
>> >         printf "\nScript does not support your architecture, sorry.\n";
>> >         printf "\nCurrently we support: \n\n";
>> >         foreach(@SUPPORTED_ARCHITECTURES) {
>> > @@ -177,7 +190,7 @@ sub dprint
>> >
>> >  sub is_supported_architecture
>> >  {
>> > -       return (is_x86_64() or is_ppc64());
>> > +       return (is_x86_64() or is_ppc64() or is_ix86_32());
>> >  }
>> >
>> >  sub is_x86_64
>> > @@ -200,10 +213,40 @@ sub is_ppc64
>> >         return 0;
>> >  }
>> >
>> > +sub is_ix86_32
>> > +{
>> > +       my $archname = $Config{archname};
>> > +
>> > +       if ($archname =~ m/i[3456]86-linux/) {
>> > +               return 1;
>> > +       }
>> > +       return 0;
>> > +}
>> > +
>> > +sub show_detected_architecture
>> > +{
>> > +       printf "Detected architecture: ";
>> > +       if (is_ix86_32()) {
>> > +               printf "32 bit x86\n";
>> > +       } elsif (is_x86_64()) {
>> > +               printf "x86_64\n";
>> > +       } elsif (is_ppc64()) {
>> > +               printf "ppc64\n";
>> > +       } else {
>> > +               printf "failed to detect architecture\n"
>> > +       }
>> > +}
>> > +
>> >  sub is_false_positive
>> >  {
>> >         my ($match) = @_;
>> >
>> > +       if (is_ix86_32()) {
>> > +               return is_false_positive_ix86_32($match);
>> > +       }
>> > +
>> > +       # 64 bit architectures
>> > +
>> >         if ($match =~ '\b(0x)?(f|F){16}\b' or
>> >             $match =~ '\b(0x)?0{16}\b') {
>> >                 return 1;
>> > @@ -220,6 +263,87 @@ sub is_false_positive
>> >         return 0;
>> >  }
>> >
>> > +sub is_false_positive_ix86_32
>> > +{
>> > +       my ($match) = @_;
>> > +       state $page_offset = get_page_offset(); # only gets called once
>> > +
>> > +       if ($match =~ '\b(0x)?(f|F){8}\b') {
>> > +               return 1;
>> > +       }
>> > +
>> > +       my $addr32 = eval hex($match);
>> > +       if ($addr32 < $page_offset) {
>> > +               return 1;
>> > +       }
>> > +
>> > +       return 0;
>> > +}
>> > +
>>
>>
>>
>> > +sub get_page_offset
>> > +{
>> > +       my $page_offset;
>> > +       my $default_offset = "0xc0000000";
>> > +       my @config_files;
>> > +
>> > +       # Allow --page-offset-32bit to over ride.
>> > +       if ($page_offset_32bit != 0) {
>> > +               return $page_offset_32bit;
>> > +       }
>> > +
>> > +       # Allow --kernel-config-file to over ride.
>> > +       if ($kernel_config_file != "") {
>> > +               @config_files = ($kernel_config_file);
>> > +       } else {
>> > +               my $config_file = '/boot/config-' . `uname -r`;
>> > +               @config_files = ($config_file, '/boot/config');
>> > +       }
>> > +
>> > +       if (-R "/proc/config.gz") {
>> > +               my $tmp_file = "/tmp/tmpkconf";
>> > +               if (system("gunzip < /proc/config.gz > $tmp_file")) {
>> > +                       dprint " parse_kernel_config: system(gunzip...) failed\n";
>> > +               } else {
>> > +                       $page_offset = parse_kernel_config_file($tmp_file);
>> > +                       if ($page_offset ne "") {
>> > +                               return $page_offset;
>> > +                       }
>> > +               }
>> > +               system("rm -f $tmp_file");
>> > +       }
>> > +
>> > +       foreach my $config_file (@config_files) {
>> > +               $page_offset = parse_kernel_config($config_file);
>> > +               if ($page_offset ne "") {
>> > +                       return $page_offset;
>> > +               }
>> > +       }
>> > +
>> > +       printf STDERR "Failed to parse kernel config files\n";
>> > +       printf STDERR "Falling back to %s\n", $default_offset;
>> > +       return $default_offset;
>> > +}
>> > +
>> > +sub parse_kernel_config_file
>> > +{
>> > +       my ($file) = @_;
>> > +       my $config = 'CONFIG_PAGE_OFFSET';
>> > +       my $val = "";
>> > +
>> > +       open(my $fh, "<", $file) or return "";
>> > +       while (my $line = <$fh> ) {
>> > +               if ($line =~ /^$config/) {
>> > +                       my ($str, $val) = split /=/, $line;
>> > +                       chomp($val);
>> > +                       last;
>> > +               }
>> > +       }
>> > +
>> > +       close $fh;
>> > +       return $val;
>> > +}
>> > +
>> > +
>>
>> Get_page_offset attempts to build a list of config files, which are
>> then passed into the parsing function for further processing.
>> This splits up the code to do with the config files between
>> get_page_offset() and parse_kernel_config_file().
>> May I suggest putting the kernel config file processing code into the
>> parse_kernel_config_file() instead, and let the parsing function
>> handle the config files and either return the page_offset or an empty
>> string.
>>
>> See below for the proposed implementation.
>
> Nice, this is much better! Thanks.
>
>> Apologies for the absence of indentation.
>
> Re-posting with indentation, comments in line.
>
>> Disclaimer: I did not test-run the code being proposed.
>
> I also did not test my comments ;)
>
>> sub get_page_offset
>> {
>>       my $default_offset = "0xc0000000";
>>       my $page_offset;
>>
>>       # Allow --page-offset-32bit to over ride.
>>       if ($page_offset_32bit != 0) {
>>               return $page_offset_32bit;
>>       }
>>
>>       $page_offset = parse_kernel_config_file();
>>       if ($page_offset ne "") {
>>               return $page_offset
>>       }
>>
>>       printf STDERR "Failed to parse kernel config files\n";
>>       printf STDERR "Falling back to %s\n", $default_offset;
>>
>>       return $default_offset;
>> }
>>
>> sub parse_kernel_config_file
>> {
>>       my @config_files;
>>       my $config = 'CONFIG_PAGE_OFFSET';
>>
>>       # Allow --kernel-config-file to over ride.
>>       if ($kernel_config_file != "") {
>>               @config_files = ($kernel_config_file);
>>       } else {
>>               my $config_file = '/boot/config-' . `uname -r`;
>>               @config_files = ($config_file, '/boot/config');
>>       }
>>
>>       if (-R "/proc/config.gz") {
>
> perhaps
>                 my $tmpkconf = '/tmp/tmpkconf';

my $tmpkconf is almost as long as /tmp/tmpkconf. The name of the tmp
file is self explanatory.
Using a variable instead of the filename in this particular context is
a matter of personal preference. If you prefer to use the variable
here, it's your call.

>
>>               if (system("gunzip < /proc/config.gz > /tmp/tmpkconf") == 0) {
>>                       push @config_files, "/tmp/tmpkconf";
>>               }
>>       }
>
> Won't there only ever be a single config file? So if /proc/config.gz is
> readable we could do

The code above builds a list of config files.
Assigning to @config_files as shown below would wipe out the config
files appended to the list so far, would it not?
So $tmpkconf needs appending to the list.

>
>                 @config_files = ($tmpkconf)
>
>>       foreach my $config_file (@config_files) {
>>               open(my $fh, "<", $config_file) or return "";
>
>                 open(my $fh, "<", $config_file) or next;

Good catch. If there's more config files to process we don't want to
return, but process the next one.

>
>>               while (my $line = <$fh> ) {
>>                       if ($line =~ /^$config/) {
>>                               my ($config_name, $page_offset) = split /=/, $line;
>>                               chomp($page_offset);
>>                               last;
>>                       }
>>               }
>>       }
>>       system("rm -f $tmp_file");
>>       close $fh;
>>
>>       return $page_offset;
>> }
>
> thanks,
> Tobin.

Thanks.

Alexander Kapshuk.

[kernel-hardening] Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Alexander Kapshuk]

On Tue, Nov 28, 2017 at 11:10 PM, Tobin C. Harding <me@tobin.cc> wrote:
> On Tue, Nov 28, 2017 at 03:16:24PM +0200, Alexander Kapshuk wrote:
>> On Tue, Nov 28, 2017 at 8:32 AM, Tobin C. Harding <me@tobin.cc> wrote:
>> > Currently, leaking_addresses.pl only supports scanning 64 bit
>> > architectures. This is due to how the regular expressions are formed. We
>> > can do better than this. 32 architectures can be supported if we take
>> > into consideration the kernel virtual address split.
>> >
>> > Add support for ix86 32 bit architectures.
>> >  - Add command line option for page offset.
>> >  - Add command line option for kernel configuration file.
>> >  - Parse kernel config file for page offset (CONFIG_PAGE_OFFSET).
>> >  - Use page offset when checking for kernel virtual addresses.
>> >
>> > Signed-off-by: Kaiwan N Billimoria <kaiwan.billimoria@gmail.com>
>> > Signed-off-by: Tobin C. Harding <me@tobin.cc>
>> > ---
>> >
>> > As discussed this is a patch based on Kaiwan's previous patch. This
>> > patch represents co development by Kaiwan and Tobin.
>> >
>> > Applies on top of commit 4fbd8d194f06 (Linux 4.15-rc1)
>> >
>> > thanks,
>> > Tobin.
>> >
>> >  scripts/leaking_addresses.pl | 168 +++++++++++++++++++++++++++++++++++++------
>> >  1 file changed, 148 insertions(+), 20 deletions(-)
>> >
>> > diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
>> > index bc5788000018..f03f2f140e0a 100755
>> > --- a/scripts/leaking_addresses.pl
>> > +++ b/scripts/leaking_addresses.pl
>> > @@ -1,9 +1,11 @@
>> >  #!/usr/bin/env perl
>> >  #
>> >  # (c) 2017 Tobin C. Harding <me@tobin.cc>
>> > +# (c) 2017 Kaiwan N Billimoria <kaiwan.billimoria@gmail.com> (ix86 stuff)
>> > +#
>> >  # Licensed under the terms of the GNU GPL License version 2
>> >  #
>> > -# leaking_addresses.pl: Scan 64 bit kernel for potential leaking addresses.
>> > +# leaking_addresses.pl: Scan the kernel for potential leaking addresses.
>> >  #  - Scans dmesg output.
>> >  #  - Walks directory tree and parses each file (for each directory in @DIRS).
>> >  #
>> > @@ -22,6 +24,7 @@ use Cwd 'abs_path';
>> >  use Term::ANSIColor qw(:constants);
>> >  use Getopt::Long qw(:config no_auto_abbrev);
>> >  use Config;
>> > +use feature 'state';
>> >
>> >  my $P = $0;
>> >  my $V = '0.01';
>> > @@ -35,18 +38,19 @@ my $TIMEOUT = 10;
>> >  # Script can only grep for kernel addresses on the following architectures. If
>> >  # your architecture is not listed here and has a grep'able kernel address please
>> >  # consider submitting a patch.
>> > -my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64');
>> > +my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64', 'i[3456]86');
>> >
>> >  # Command line options.
>> >  my $help = 0;
>> >  my $debug = 0;
>> > -my $raw = 0;
>> > -my $output_raw = "";   # Write raw results to file.
>> > -my $input_raw = "";    # Read raw results from file instead of scanning.
>> > -
>> > +my $raw = 0;                   # Show raw output.
>> > +my $output_raw = "";           # Write raw results to file.
>> > +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 $page_offset_32bit = 0;     # 32-bit: value of CONFIG_PAGE_OFFSET
>> > +my $kernel_config_file = "";   # Kernel configuration file.
>> >
>> >  # Do not parse these files (absolute path).
>> >  my @skip_parse_files_abs = ('/proc/kmsg',
>> > @@ -95,14 +99,16 @@ Version: $V
>> >
>> >  Options:
>> >
>> > -       -o, --output-raw=<file>  Save results for future processing.
>> > -       -i, --input-raw=<file>   Read results from file instead of scanning.
>> > -           --raw                Show raw results (default).
>> > -           --suppress-dmesg     Do not show dmesg results.
>> > -           --squash-by-path     Show one result per unique path.
>> > -           --squash-by-filename Show one result per unique filename.
>> > -       -d, --debug              Display debugging output.
>> > -       -h, --help, --version    Display this help and exit.
>> > +       -o, --output-raw=<file>         Save results for future processing.
>> > +       -i, --input-raw=<file>          Read results from file instead of scanning.
>> > +             --raw                       Show raw results (default).
>> > +             --suppress-dmesg            Do not show dmesg results.
>> > +             --squash-by-path            Show one result per unique path.
>> > +             --squash-by-filename        Show one result per unique filename.
>> > +           --page-offset-32bit=<hex>   PAGE_OFFSET value (for 32-bit kernels).
>> > +           --kernel-config-file=<file> Kernel configuration file (e.g /boot/config)
>> > +       -d, --debug                     Display debugging output.
>> > +       -h, --help, --version           Display this help and exit.
>> >
>> >  Examples:
>> >
>> > @@ -115,7 +121,10 @@ Examples:
>> >         # View summary report.
>> >         $0 --input-raw scan.out --squash-by-filename
>> >
>> > -Scans the running (64 bit) kernel for potential leaking addresses.
>> > +       # Scan kernel on a 32-bit system with a 2GB:2GB virtual address split.
>> > +       $0 --page-offset-32bit=0x80000000
>> > +
>> > +Scans the running kernel for potential leaking addresses.
>> >
>> >  EOM
>> >         exit($exitcode);
>> > @@ -131,6 +140,8 @@ GetOptions(
>> >         'squash-by-path'        => \$squash_by_path,
>> >         'squash-by-filename'    => \$squash_by_filename,
>> >         'raw'                   => \$raw,
>> > +       'page-offset-32bit=o'   => \$page_offset_32bit,
>> > +       'kernel-config-file=s'  => \$kernel_config_file,
>> >  ) or help(1);
>> >
>> >  help(0) if ($help);
>> > @@ -146,7 +157,9 @@ if (!$input_raw and ($squash_by_path or $squash_by_filename)) {
>> >         exit(128);
>> >  }
>> >
>> > -if (!is_supported_architecture()) {
>> > +if (is_supported_architecture()) {
>> > +       show_detected_architecture() if $debug;
>> > +} else {
>> >         printf "\nScript does not support your architecture, sorry.\n";
>> >         printf "\nCurrently we support: \n\n";
>> >         foreach(@SUPPORTED_ARCHITECTURES) {
>> > @@ -177,7 +190,7 @@ sub dprint
>> >
>> >  sub is_supported_architecture
>> >  {
>> > -       return (is_x86_64() or is_ppc64());
>> > +       return (is_x86_64() or is_ppc64() or is_ix86_32());
>> >  }
>> >
>> >  sub is_x86_64
>> > @@ -200,10 +213,40 @@ sub is_ppc64
>> >         return 0;
>> >  }
>> >
>> > +sub is_ix86_32
>> > +{
>> > +       my $archname = $Config{archname};
>> > +
>> > +       if ($archname =~ m/i[3456]86-linux/) {
>> > +               return 1;
>> > +       }
>> > +       return 0;
>> > +}
>> > +
>> > +sub show_detected_architecture
>> > +{
>> > +       printf "Detected architecture: ";
>> > +       if (is_ix86_32()) {
>> > +               printf "32 bit x86\n";
>> > +       } elsif (is_x86_64()) {
>> > +               printf "x86_64\n";
>> > +       } elsif (is_ppc64()) {
>> > +               printf "ppc64\n";
>> > +       } else {
>> > +               printf "failed to detect architecture\n"
>> > +       }
>> > +}
>> > +
>> >  sub is_false_positive
>> >  {
>> >         my ($match) = @_;
>> >
>> > +       if (is_ix86_32()) {
>> > +               return is_false_positive_ix86_32($match);
>> > +       }
>> > +
>> > +       # 64 bit architectures
>> > +
>> >         if ($match =~ '\b(0x)?(f|F){16}\b' or
>> >             $match =~ '\b(0x)?0{16}\b') {
>> >                 return 1;
>> > @@ -220,6 +263,87 @@ sub is_false_positive
>> >         return 0;
>> >  }
>> >
>> > +sub is_false_positive_ix86_32
>> > +{
>> > +       my ($match) = @_;
>> > +       state $page_offset = get_page_offset(); # only gets called once
>> > +
>> > +       if ($match =~ '\b(0x)?(f|F){8}\b') {
>> > +               return 1;
>> > +       }
>> > +
>> > +       my $addr32 = eval hex($match);
>> > +       if ($addr32 < $page_offset) {
>> > +               return 1;
>> > +       }
>> > +
>> > +       return 0;
>> > +}
>> > +
>>
>>
>>
>> > +sub get_page_offset
>> > +{
>> > +       my $page_offset;
>> > +       my $default_offset = "0xc0000000";
>> > +       my @config_files;
>> > +
>> > +       # Allow --page-offset-32bit to over ride.
>> > +       if ($page_offset_32bit != 0) {
>> > +               return $page_offset_32bit;
>> > +       }
>> > +
>> > +       # Allow --kernel-config-file to over ride.
>> > +       if ($kernel_config_file != "") {
>> > +               @config_files = ($kernel_config_file);
>> > +       } else {
>> > +               my $config_file = '/boot/config-' . `uname -r`;
>> > +               @config_files = ($config_file, '/boot/config');
>> > +       }
>> > +
>> > +       if (-R "/proc/config.gz") {
>> > +               my $tmp_file = "/tmp/tmpkconf";
>> > +               if (system("gunzip < /proc/config.gz > $tmp_file")) {
>> > +                       dprint " parse_kernel_config: system(gunzip...) failed\n";
>> > +               } else {
>> > +                       $page_offset = parse_kernel_config_file($tmp_file);
>> > +                       if ($page_offset ne "") {
>> > +                               return $page_offset;
>> > +                       }
>> > +               }
>> > +               system("rm -f $tmp_file");
>> > +       }
>> > +
>> > +       foreach my $config_file (@config_files) {
>> > +               $page_offset = parse_kernel_config($config_file);
>> > +               if ($page_offset ne "") {
>> > +                       return $page_offset;
>> > +               }
>> > +       }
>> > +
>> > +       printf STDERR "Failed to parse kernel config files\n";
>> > +       printf STDERR "Falling back to %s\n", $default_offset;
>> > +       return $default_offset;
>> > +}
>> > +
>> > +sub parse_kernel_config_file
>> > +{
>> > +       my ($file) = @_;
>> > +       my $config = 'CONFIG_PAGE_OFFSET';
>> > +       my $val = "";
>> > +
>> > +       open(my $fh, "<", $file) or return "";
>> > +       while (my $line = <$fh> ) {
>> > +               if ($line =~ /^$config/) {
>> > +                       my ($str, $val) = split /=/, $line;
>> > +                       chomp($val);
>> > +                       last;
>> > +               }
>> > +       }
>> > +
>> > +       close $fh;
>> > +       return $val;
>> > +}
>> > +
>> > +
>>
>> Get_page_offset attempts to build a list of config files, which are
>> then passed into the parsing function for further processing.
>> This splits up the code to do with the config files between
>> get_page_offset() and parse_kernel_config_file().
>> May I suggest putting the kernel config file processing code into the
>> parse_kernel_config_file() instead, and let the parsing function
>> handle the config files and either return the page_offset or an empty
>> string.
>>
>> See below for the proposed implementation.
>
> Nice, this is much better! Thanks.
>
>> Apologies for the absence of indentation.
>
> Re-posting with indentation, comments in line.
>
>> Disclaimer: I did not test-run the code being proposed.
>
> I also did not test my comments ;)
>
>> sub get_page_offset
>> {
>>       my $default_offset = "0xc0000000";
>>       my $page_offset;
>>
>>       # Allow --page-offset-32bit to over ride.
>>       if ($page_offset_32bit != 0) {
>>               return $page_offset_32bit;
>>       }
>>
>>       $page_offset = parse_kernel_config_file();
>>       if ($page_offset ne "") {
>>               return $page_offset
>>       }
>>
>>       printf STDERR "Failed to parse kernel config files\n";
>>       printf STDERR "Falling back to %s\n", $default_offset;
>>
>>       return $default_offset;
>> }
>>
>> sub parse_kernel_config_file
>> {
>>       my @config_files;
>>       my $config = 'CONFIG_PAGE_OFFSET';
>>
>>       # Allow --kernel-config-file to over ride.
>>       if ($kernel_config_file != "") {
>>               @config_files = ($kernel_config_file);
>>       } else {
>>               my $config_file = '/boot/config-' . `uname -r`;
>>               @config_files = ($config_file, '/boot/config');
>>       }
>>
>>       if (-R "/proc/config.gz") {
>
> perhaps
>                 my $tmpkconf = '/tmp/tmpkconf';

my $tmpkconf is almost as long as /tmp/tmpkconf. The name of the tmp
file is self explanatory.
Using a variable instead of the filename in this particular context is
a matter of personal preference. If you prefer to use the variable
here, it's your call.

>
>>               if (system("gunzip < /proc/config.gz > /tmp/tmpkconf") == 0) {
>>                       push @config_files, "/tmp/tmpkconf";
>>               }
>>       }
>
> Won't there only ever be a single config file? So if /proc/config.gz is
> readable we could do

The code above builds a list of config files.
Assigning to @config_files as shown below would wipe out the config
files appended to the list so far, would it not?
So $tmpkconf needs appending to the list.

>
>                 @config_files = ($tmpkconf)
>
>>       foreach my $config_file (@config_files) {
>>               open(my $fh, "<", $config_file) or return "";
>
>                 open(my $fh, "<", $config_file) or next;

Good catch. If there's more config files to process we don't want to
return, but process the next one.

>
>>               while (my $line = <$fh> ) {
>>                       if ($line =~ /^$config/) {
>>                               my ($config_name, $page_offset) = split /=/, $line;
>>                               chomp($page_offset);
>>                               last;
>>                       }
>>               }
>>       }
>>       system("rm -f $tmp_file");
>>       close $fh;
>>
>>       return $page_offset;
>> }
>
> thanks,
> Tobin.

Thanks.

Alexander Kapshuk.

Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Tobin C. Harding]

On Wed, Nov 29, 2017 at 09:59:59AM +0200, Alexander Kapshuk wrote:
> On Tue, Nov 28, 2017 at 11:10 PM, Tobin C. Harding <me@tobin.cc> wrote:
> > On Tue, Nov 28, 2017 at 03:16:24PM +0200, Alexander Kapshuk wrote:
> >> On Tue, Nov 28, 2017 at 8:32 AM, Tobin C. Harding <me@tobin.cc> wrote:
> >> > Currently, leaking_addresses.pl only supports scanning 64 bit
> >> > architectures. This is due to how the regular expressions are formed. We
> >> > can do better than this. 32 architectures can be supported if we take
> >> > into consideration the kernel virtual address split.
> >> >
> >> > Add support for ix86 32 bit architectures.
> >> >  - Add command line option for page offset.
> >> >  - Add command line option for kernel configuration file.
> >> >  - Parse kernel config file for page offset (CONFIG_PAGE_OFFSET).
> >> >  - Use page offset when checking for kernel virtual addresses.
> >> >
> >> > Signed-off-by: Kaiwan N Billimoria <kaiwan.billimoria@gmail.com>
> >> > Signed-off-by: Tobin C. Harding <me@tobin.cc>
> >> > ---
> >> >
> >> > As discussed this is a patch based on Kaiwan's previous patch. This
> >> > patch represents co development by Kaiwan and Tobin.
> >> >
> >> > Applies on top of commit 4fbd8d194f06 (Linux 4.15-rc1)
> >> >
> >> > thanks,
> >> > Tobin.
> >> >
> >> >  scripts/leaking_addresses.pl | 168 +++++++++++++++++++++++++++++++++++++------
> >> >  1 file changed, 148 insertions(+), 20 deletions(-)
> >> >
> >> > diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> >> > index bc5788000018..f03f2f140e0a 100755
> >> > --- a/scripts/leaking_addresses.pl
> >> > +++ b/scripts/leaking_addresses.pl
> >> > @@ -1,9 +1,11 @@
> >> >  #!/usr/bin/env perl
> >> >  #
> >> >  # (c) 2017 Tobin C. Harding <me@tobin.cc>
> >> > +# (c) 2017 Kaiwan N Billimoria <kaiwan.billimoria@gmail.com> (ix86 stuff)
> >> > +#
> >> >  # Licensed under the terms of the GNU GPL License version 2
> >> >  #
> >> > -# leaking_addresses.pl: Scan 64 bit kernel for potential leaking addresses.
> >> > +# leaking_addresses.pl: Scan the kernel for potential leaking addresses.
> >> >  #  - Scans dmesg output.
> >> >  #  - Walks directory tree and parses each file (for each directory in @DIRS).
> >> >  #
> >> > @@ -22,6 +24,7 @@ use Cwd 'abs_path';
> >> >  use Term::ANSIColor qw(:constants);
> >> >  use Getopt::Long qw(:config no_auto_abbrev);
> >> >  use Config;
> >> > +use feature 'state';
> >> >
> >> >  my $P = $0;
> >> >  my $V = '0.01';
> >> > @@ -35,18 +38,19 @@ my $TIMEOUT = 10;
> >> >  # Script can only grep for kernel addresses on the following architectures. If
> >> >  # your architecture is not listed here and has a grep'able kernel address please
> >> >  # consider submitting a patch.
> >> > -my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64');
> >> > +my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64', 'i[3456]86');
> >> >
> >> >  # Command line options.
> >> >  my $help = 0;
> >> >  my $debug = 0;
> >> > -my $raw = 0;
> >> > -my $output_raw = "";   # Write raw results to file.
> >> > -my $input_raw = "";    # Read raw results from file instead of scanning.
> >> > -
> >> > +my $raw = 0;                   # Show raw output.
> >> > +my $output_raw = "";           # Write raw results to file.
> >> > +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 $page_offset_32bit = 0;     # 32-bit: value of CONFIG_PAGE_OFFSET
> >> > +my $kernel_config_file = "";   # Kernel configuration file.
> >> >
> >> >  # Do not parse these files (absolute path).
> >> >  my @skip_parse_files_abs = ('/proc/kmsg',
> >> > @@ -95,14 +99,16 @@ Version: $V
> >> >
> >> >  Options:
> >> >
> >> > -       -o, --output-raw=<file>  Save results for future processing.
> >> > -       -i, --input-raw=<file>   Read results from file instead of scanning.
> >> > -           --raw                Show raw results (default).
> >> > -           --suppress-dmesg     Do not show dmesg results.
> >> > -           --squash-by-path     Show one result per unique path.
> >> > -           --squash-by-filename Show one result per unique filename.
> >> > -       -d, --debug              Display debugging output.
> >> > -       -h, --help, --version    Display this help and exit.
> >> > +       -o, --output-raw=<file>         Save results for future processing.
> >> > +       -i, --input-raw=<file>          Read results from file instead of scanning.
> >> > +             --raw                       Show raw results (default).
> >> > +             --suppress-dmesg            Do not show dmesg results.
> >> > +             --squash-by-path            Show one result per unique path.
> >> > +             --squash-by-filename        Show one result per unique filename.
> >> > +           --page-offset-32bit=<hex>   PAGE_OFFSET value (for 32-bit kernels).
> >> > +           --kernel-config-file=<file> Kernel configuration file (e.g /boot/config)
> >> > +       -d, --debug                     Display debugging output.
> >> > +       -h, --help, --version           Display this help and exit.
> >> >
> >> >  Examples:
> >> >
> >> > @@ -115,7 +121,10 @@ Examples:
> >> >         # View summary report.
> >> >         $0 --input-raw scan.out --squash-by-filename
> >> >
> >> > -Scans the running (64 bit) kernel for potential leaking addresses.
> >> > +       # Scan kernel on a 32-bit system with a 2GB:2GB virtual address split.
> >> > +       $0 --page-offset-32bit=0x80000000
> >> > +
> >> > +Scans the running kernel for potential leaking addresses.
> >> >
> >> >  EOM
> >> >         exit($exitcode);
> >> > @@ -131,6 +140,8 @@ GetOptions(
> >> >         'squash-by-path'        => \$squash_by_path,
> >> >         'squash-by-filename'    => \$squash_by_filename,
> >> >         'raw'                   => \$raw,
> >> > +       'page-offset-32bit=o'   => \$page_offset_32bit,
> >> > +       'kernel-config-file=s'  => \$kernel_config_file,
> >> >  ) or help(1);
> >> >
> >> >  help(0) if ($help);
> >> > @@ -146,7 +157,9 @@ if (!$input_raw and ($squash_by_path or $squash_by_filename)) {
> >> >         exit(128);
> >> >  }
> >> >
> >> > -if (!is_supported_architecture()) {
> >> > +if (is_supported_architecture()) {
> >> > +       show_detected_architecture() if $debug;
> >> > +} else {
> >> >         printf "\nScript does not support your architecture, sorry.\n";
> >> >         printf "\nCurrently we support: \n\n";
> >> >         foreach(@SUPPORTED_ARCHITECTURES) {
> >> > @@ -177,7 +190,7 @@ sub dprint
> >> >
> >> >  sub is_supported_architecture
> >> >  {
> >> > -       return (is_x86_64() or is_ppc64());
> >> > +       return (is_x86_64() or is_ppc64() or is_ix86_32());
> >> >  }
> >> >
> >> >  sub is_x86_64
> >> > @@ -200,10 +213,40 @@ sub is_ppc64
> >> >         return 0;
> >> >  }
> >> >
> >> > +sub is_ix86_32
> >> > +{
> >> > +       my $archname = $Config{archname};
> >> > +
> >> > +       if ($archname =~ m/i[3456]86-linux/) {
> >> > +               return 1;
> >> > +       }
> >> > +       return 0;
> >> > +}
> >> > +
> >> > +sub show_detected_architecture
> >> > +{
> >> > +       printf "Detected architecture: ";
> >> > +       if (is_ix86_32()) {
> >> > +               printf "32 bit x86\n";
> >> > +       } elsif (is_x86_64()) {
> >> > +               printf "x86_64\n";
> >> > +       } elsif (is_ppc64()) {
> >> > +               printf "ppc64\n";
> >> > +       } else {
> >> > +               printf "failed to detect architecture\n"
> >> > +       }
> >> > +}
> >> > +
> >> >  sub is_false_positive
> >> >  {
> >> >         my ($match) = @_;
> >> >
> >> > +       if (is_ix86_32()) {
> >> > +               return is_false_positive_ix86_32($match);
> >> > +       }
> >> > +
> >> > +       # 64 bit architectures
> >> > +
> >> >         if ($match =~ '\b(0x)?(f|F){16}\b' or
> >> >             $match =~ '\b(0x)?0{16}\b') {
> >> >                 return 1;
> >> > @@ -220,6 +263,87 @@ sub is_false_positive
> >> >         return 0;
> >> >  }
> >> >
> >> > +sub is_false_positive_ix86_32
> >> > +{
> >> > +       my ($match) = @_;
> >> > +       state $page_offset = get_page_offset(); # only gets called once
> >> > +
> >> > +       if ($match =~ '\b(0x)?(f|F){8}\b') {
> >> > +               return 1;
> >> > +       }
> >> > +
> >> > +       my $addr32 = eval hex($match);
> >> > +       if ($addr32 < $page_offset) {
> >> > +               return 1;
> >> > +       }
> >> > +
> >> > +       return 0;
> >> > +}
> >> > +
> >>
> >>
> >>
> >> > +sub get_page_offset
> >> > +{
> >> > +       my $page_offset;
> >> > +       my $default_offset = "0xc0000000";
> >> > +       my @config_files;
> >> > +
> >> > +       # Allow --page-offset-32bit to over ride.
> >> > +       if ($page_offset_32bit != 0) {
> >> > +               return $page_offset_32bit;
> >> > +       }
> >> > +
> >> > +       # Allow --kernel-config-file to over ride.
> >> > +       if ($kernel_config_file != "") {
> >> > +               @config_files = ($kernel_config_file);
> >> > +       } else {
> >> > +               my $config_file = '/boot/config-' . `uname -r`;
> >> > +               @config_files = ($config_file, '/boot/config');
> >> > +       }
> >> > +
> >> > +       if (-R "/proc/config.gz") {
> >> > +               my $tmp_file = "/tmp/tmpkconf";
> >> > +               if (system("gunzip < /proc/config.gz > $tmp_file")) {
> >> > +                       dprint " parse_kernel_config: system(gunzip...) failed\n";
> >> > +               } else {
> >> > +                       $page_offset = parse_kernel_config_file($tmp_file);
> >> > +                       if ($page_offset ne "") {
> >> > +                               return $page_offset;
> >> > +                       }
> >> > +               }
> >> > +               system("rm -f $tmp_file");
> >> > +       }
> >> > +
> >> > +       foreach my $config_file (@config_files) {
> >> > +               $page_offset = parse_kernel_config($config_file);
> >> > +               if ($page_offset ne "") {
> >> > +                       return $page_offset;
> >> > +               }
> >> > +       }
> >> > +
> >> > +       printf STDERR "Failed to parse kernel config files\n";
> >> > +       printf STDERR "Falling back to %s\n", $default_offset;
> >> > +       return $default_offset;
> >> > +}
> >> > +
> >> > +sub parse_kernel_config_file
> >> > +{
> >> > +       my ($file) = @_;
> >> > +       my $config = 'CONFIG_PAGE_OFFSET';
> >> > +       my $val = "";
> >> > +
> >> > +       open(my $fh, "<", $file) or return "";
> >> > +       while (my $line = <$fh> ) {
> >> > +               if ($line =~ /^$config/) {
> >> > +                       my ($str, $val) = split /=/, $line;
> >> > +                       chomp($val);
> >> > +                       last;
> >> > +               }
> >> > +       }
> >> > +
> >> > +       close $fh;
> >> > +       return $val;
> >> > +}
> >> > +
> >> > +
> >>
> >> Get_page_offset attempts to build a list of config files, which are
> >> then passed into the parsing function for further processing.
> >> This splits up the code to do with the config files between
> >> get_page_offset() and parse_kernel_config_file().
> >> May I suggest putting the kernel config file processing code into the
> >> parse_kernel_config_file() instead, and let the parsing function
> >> handle the config files and either return the page_offset or an empty
> >> string.
> >>
> >> See below for the proposed implementation.
> >
> > Nice, this is much better! Thanks.
> >
> >> Apologies for the absence of indentation.
> >
> > Re-posting with indentation, comments in line.
> >
> >> Disclaimer: I did not test-run the code being proposed.
> >
> > I also did not test my comments ;)
> >
> >> sub get_page_offset
> >> {
> >>       my $default_offset = "0xc0000000";
> >>       my $page_offset;
> >>
> >>       # Allow --page-offset-32bit to over ride.
> >>       if ($page_offset_32bit != 0) {
> >>               return $page_offset_32bit;
> >>       }
> >>
> >>       $page_offset = parse_kernel_config_file();
> >>       if ($page_offset ne "") {
> >>               return $page_offset
> >>       }
> >>
> >>       printf STDERR "Failed to parse kernel config files\n";
> >>       printf STDERR "Falling back to %s\n", $default_offset;
> >>
> >>       return $default_offset;
> >> }
> >>
> >> sub parse_kernel_config_file
> >> {
> >>       my @config_files;
> >>       my $config = 'CONFIG_PAGE_OFFSET';
> >>
> >>       # Allow --kernel-config-file to over ride.
> >>       if ($kernel_config_file != "") {
> >>               @config_files = ($kernel_config_file);
> >>       } else {
> >>               my $config_file = '/boot/config-' . `uname -r`;
> >>               @config_files = ($config_file, '/boot/config');
> >>       }
> >>
> >>       if (-R "/proc/config.gz") {
> >
> > perhaps
> >                 my $tmpkconf = '/tmp/tmpkconf';
> 
> my $tmpkconf is almost as long as /tmp/tmpkconf. The name of the tmp
> file is self explanatory.
> Using a variable instead of the filename in this particular context is
> a matter of personal preference. If you prefer to use the variable
> here, it's your call.

I'm a stickler for no const strings or magic numbers but it's Kaiwan's
patch, if he puts it in with const strings I'll apply it as is :)

> >
> >>               if (system("gunzip < /proc/config.gz > /tmp/tmpkconf") == 0) {
> >>                       push @config_files, "/tmp/tmpkconf";
> >>               }
> >>       }
> >
> > Won't there only ever be a single config file? So if /proc/config.gz is
> > readable we could do
> 
> The code above builds a list of config files.
> Assigning to @config_files as shown below would wipe out the config
> files appended to the list so far, would it not?
> So $tmpkconf needs appending to the list.

You are correct, since the beginning of this function that has been the
algorithm. My observation is that if /proc/config.gz is present then we
don't need to parse the other files so it is better to blow them away.

I don't know enough about the whole Linux-sphere to know if this is
correct. But it seems reasonable that even if there is more than one way
to look at the running kernels config file they will all be the same,
the system would be pretty broken if they were different.

So once we have found a readable config file just parse it and be done
with it, no need to loop over any others.

thanks,
Tobin.

[kernel-hardening] Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Tobin C. Harding]

On Wed, Nov 29, 2017 at 09:59:59AM +0200, Alexander Kapshuk wrote:
> On Tue, Nov 28, 2017 at 11:10 PM, Tobin C. Harding <me@tobin.cc> wrote:
> > On Tue, Nov 28, 2017 at 03:16:24PM +0200, Alexander Kapshuk wrote:
> >> On Tue, Nov 28, 2017 at 8:32 AM, Tobin C. Harding <me@tobin.cc> wrote:
> >> > Currently, leaking_addresses.pl only supports scanning 64 bit
> >> > architectures. This is due to how the regular expressions are formed. We
> >> > can do better than this. 32 architectures can be supported if we take
> >> > into consideration the kernel virtual address split.
> >> >
> >> > Add support for ix86 32 bit architectures.
> >> >  - Add command line option for page offset.
> >> >  - Add command line option for kernel configuration file.
> >> >  - Parse kernel config file for page offset (CONFIG_PAGE_OFFSET).
> >> >  - Use page offset when checking for kernel virtual addresses.
> >> >
> >> > Signed-off-by: Kaiwan N Billimoria <kaiwan.billimoria@gmail.com>
> >> > Signed-off-by: Tobin C. Harding <me@tobin.cc>
> >> > ---
> >> >
> >> > As discussed this is a patch based on Kaiwan's previous patch. This
> >> > patch represents co development by Kaiwan and Tobin.
> >> >
> >> > Applies on top of commit 4fbd8d194f06 (Linux 4.15-rc1)
> >> >
> >> > thanks,
> >> > Tobin.
> >> >
> >> >  scripts/leaking_addresses.pl | 168 +++++++++++++++++++++++++++++++++++++------
> >> >  1 file changed, 148 insertions(+), 20 deletions(-)
> >> >
> >> > diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> >> > index bc5788000018..f03f2f140e0a 100755
> >> > --- a/scripts/leaking_addresses.pl
> >> > +++ b/scripts/leaking_addresses.pl
> >> > @@ -1,9 +1,11 @@
> >> >  #!/usr/bin/env perl
> >> >  #
> >> >  # (c) 2017 Tobin C. Harding <me@tobin.cc>
> >> > +# (c) 2017 Kaiwan N Billimoria <kaiwan.billimoria@gmail.com> (ix86 stuff)
> >> > +#
> >> >  # Licensed under the terms of the GNU GPL License version 2
> >> >  #
> >> > -# leaking_addresses.pl: Scan 64 bit kernel for potential leaking addresses.
> >> > +# leaking_addresses.pl: Scan the kernel for potential leaking addresses.
> >> >  #  - Scans dmesg output.
> >> >  #  - Walks directory tree and parses each file (for each directory in @DIRS).
> >> >  #
> >> > @@ -22,6 +24,7 @@ use Cwd 'abs_path';
> >> >  use Term::ANSIColor qw(:constants);
> >> >  use Getopt::Long qw(:config no_auto_abbrev);
> >> >  use Config;
> >> > +use feature 'state';
> >> >
> >> >  my $P = $0;
> >> >  my $V = '0.01';
> >> > @@ -35,18 +38,19 @@ my $TIMEOUT = 10;
> >> >  # Script can only grep for kernel addresses on the following architectures. If
> >> >  # your architecture is not listed here and has a grep'able kernel address please
> >> >  # consider submitting a patch.
> >> > -my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64');
> >> > +my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64', 'i[3456]86');
> >> >
> >> >  # Command line options.
> >> >  my $help = 0;
> >> >  my $debug = 0;
> >> > -my $raw = 0;
> >> > -my $output_raw = "";   # Write raw results to file.
> >> > -my $input_raw = "";    # Read raw results from file instead of scanning.
> >> > -
> >> > +my $raw = 0;                   # Show raw output.
> >> > +my $output_raw = "";           # Write raw results to file.
> >> > +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 $page_offset_32bit = 0;     # 32-bit: value of CONFIG_PAGE_OFFSET
> >> > +my $kernel_config_file = "";   # Kernel configuration file.
> >> >
> >> >  # Do not parse these files (absolute path).
> >> >  my @skip_parse_files_abs = ('/proc/kmsg',
> >> > @@ -95,14 +99,16 @@ Version: $V
> >> >
> >> >  Options:
> >> >
> >> > -       -o, --output-raw=<file>  Save results for future processing.
> >> > -       -i, --input-raw=<file>   Read results from file instead of scanning.
> >> > -           --raw                Show raw results (default).
> >> > -           --suppress-dmesg     Do not show dmesg results.
> >> > -           --squash-by-path     Show one result per unique path.
> >> > -           --squash-by-filename Show one result per unique filename.
> >> > -       -d, --debug              Display debugging output.
> >> > -       -h, --help, --version    Display this help and exit.
> >> > +       -o, --output-raw=<file>         Save results for future processing.
> >> > +       -i, --input-raw=<file>          Read results from file instead of scanning.
> >> > +             --raw                       Show raw results (default).
> >> > +             --suppress-dmesg            Do not show dmesg results.
> >> > +             --squash-by-path            Show one result per unique path.
> >> > +             --squash-by-filename        Show one result per unique filename.
> >> > +           --page-offset-32bit=<hex>   PAGE_OFFSET value (for 32-bit kernels).
> >> > +           --kernel-config-file=<file> Kernel configuration file (e.g /boot/config)
> >> > +       -d, --debug                     Display debugging output.
> >> > +       -h, --help, --version           Display this help and exit.
> >> >
> >> >  Examples:
> >> >
> >> > @@ -115,7 +121,10 @@ Examples:
> >> >         # View summary report.
> >> >         $0 --input-raw scan.out --squash-by-filename
> >> >
> >> > -Scans the running (64 bit) kernel for potential leaking addresses.
> >> > +       # Scan kernel on a 32-bit system with a 2GB:2GB virtual address split.
> >> > +       $0 --page-offset-32bit=0x80000000
> >> > +
> >> > +Scans the running kernel for potential leaking addresses.
> >> >
> >> >  EOM
> >> >         exit($exitcode);
> >> > @@ -131,6 +140,8 @@ GetOptions(
> >> >         'squash-by-path'        => \$squash_by_path,
> >> >         'squash-by-filename'    => \$squash_by_filename,
> >> >         'raw'                   => \$raw,
> >> > +       'page-offset-32bit=o'   => \$page_offset_32bit,
> >> > +       'kernel-config-file=s'  => \$kernel_config_file,
> >> >  ) or help(1);
> >> >
> >> >  help(0) if ($help);
> >> > @@ -146,7 +157,9 @@ if (!$input_raw and ($squash_by_path or $squash_by_filename)) {
> >> >         exit(128);
> >> >  }
> >> >
> >> > -if (!is_supported_architecture()) {
> >> > +if (is_supported_architecture()) {
> >> > +       show_detected_architecture() if $debug;
> >> > +} else {
> >> >         printf "\nScript does not support your architecture, sorry.\n";
> >> >         printf "\nCurrently we support: \n\n";
> >> >         foreach(@SUPPORTED_ARCHITECTURES) {
> >> > @@ -177,7 +190,7 @@ sub dprint
> >> >
> >> >  sub is_supported_architecture
> >> >  {
> >> > -       return (is_x86_64() or is_ppc64());
> >> > +       return (is_x86_64() or is_ppc64() or is_ix86_32());
> >> >  }
> >> >
> >> >  sub is_x86_64
> >> > @@ -200,10 +213,40 @@ sub is_ppc64
> >> >         return 0;
> >> >  }
> >> >
> >> > +sub is_ix86_32
> >> > +{
> >> > +       my $archname = $Config{archname};
> >> > +
> >> > +       if ($archname =~ m/i[3456]86-linux/) {
> >> > +               return 1;
> >> > +       }
> >> > +       return 0;
> >> > +}
> >> > +
> >> > +sub show_detected_architecture
> >> > +{
> >> > +       printf "Detected architecture: ";
> >> > +       if (is_ix86_32()) {
> >> > +               printf "32 bit x86\n";
> >> > +       } elsif (is_x86_64()) {
> >> > +               printf "x86_64\n";
> >> > +       } elsif (is_ppc64()) {
> >> > +               printf "ppc64\n";
> >> > +       } else {
> >> > +               printf "failed to detect architecture\n"
> >> > +       }
> >> > +}
> >> > +
> >> >  sub is_false_positive
> >> >  {
> >> >         my ($match) = @_;
> >> >
> >> > +       if (is_ix86_32()) {
> >> > +               return is_false_positive_ix86_32($match);
> >> > +       }
> >> > +
> >> > +       # 64 bit architectures
> >> > +
> >> >         if ($match =~ '\b(0x)?(f|F){16}\b' or
> >> >             $match =~ '\b(0x)?0{16}\b') {
> >> >                 return 1;
> >> > @@ -220,6 +263,87 @@ sub is_false_positive
> >> >         return 0;
> >> >  }
> >> >
> >> > +sub is_false_positive_ix86_32
> >> > +{
> >> > +       my ($match) = @_;
> >> > +       state $page_offset = get_page_offset(); # only gets called once
> >> > +
> >> > +       if ($match =~ '\b(0x)?(f|F){8}\b') {
> >> > +               return 1;
> >> > +       }
> >> > +
> >> > +       my $addr32 = eval hex($match);
> >> > +       if ($addr32 < $page_offset) {
> >> > +               return 1;
> >> > +       }
> >> > +
> >> > +       return 0;
> >> > +}
> >> > +
> >>
> >>
> >>
> >> > +sub get_page_offset
> >> > +{
> >> > +       my $page_offset;
> >> > +       my $default_offset = "0xc0000000";
> >> > +       my @config_files;
> >> > +
> >> > +       # Allow --page-offset-32bit to over ride.
> >> > +       if ($page_offset_32bit != 0) {
> >> > +               return $page_offset_32bit;
> >> > +       }
> >> > +
> >> > +       # Allow --kernel-config-file to over ride.
> >> > +       if ($kernel_config_file != "") {
> >> > +               @config_files = ($kernel_config_file);
> >> > +       } else {
> >> > +               my $config_file = '/boot/config-' . `uname -r`;
> >> > +               @config_files = ($config_file, '/boot/config');
> >> > +       }
> >> > +
> >> > +       if (-R "/proc/config.gz") {
> >> > +               my $tmp_file = "/tmp/tmpkconf";
> >> > +               if (system("gunzip < /proc/config.gz > $tmp_file")) {
> >> > +                       dprint " parse_kernel_config: system(gunzip...) failed\n";
> >> > +               } else {
> >> > +                       $page_offset = parse_kernel_config_file($tmp_file);
> >> > +                       if ($page_offset ne "") {
> >> > +                               return $page_offset;
> >> > +                       }
> >> > +               }
> >> > +               system("rm -f $tmp_file");
> >> > +       }
> >> > +
> >> > +       foreach my $config_file (@config_files) {
> >> > +               $page_offset = parse_kernel_config($config_file);
> >> > +               if ($page_offset ne "") {
> >> > +                       return $page_offset;
> >> > +               }
> >> > +       }
> >> > +
> >> > +       printf STDERR "Failed to parse kernel config files\n";
> >> > +       printf STDERR "Falling back to %s\n", $default_offset;
> >> > +       return $default_offset;
> >> > +}
> >> > +
> >> > +sub parse_kernel_config_file
> >> > +{
> >> > +       my ($file) = @_;
> >> > +       my $config = 'CONFIG_PAGE_OFFSET';
> >> > +       my $val = "";
> >> > +
> >> > +       open(my $fh, "<", $file) or return "";
> >> > +       while (my $line = <$fh> ) {
> >> > +               if ($line =~ /^$config/) {
> >> > +                       my ($str, $val) = split /=/, $line;
> >> > +                       chomp($val);
> >> > +                       last;
> >> > +               }
> >> > +       }
> >> > +
> >> > +       close $fh;
> >> > +       return $val;
> >> > +}
> >> > +
> >> > +
> >>
> >> Get_page_offset attempts to build a list of config files, which are
> >> then passed into the parsing function for further processing.
> >> This splits up the code to do with the config files between
> >> get_page_offset() and parse_kernel_config_file().
> >> May I suggest putting the kernel config file processing code into the
> >> parse_kernel_config_file() instead, and let the parsing function
> >> handle the config files and either return the page_offset or an empty
> >> string.
> >>
> >> See below for the proposed implementation.
> >
> > Nice, this is much better! Thanks.
> >
> >> Apologies for the absence of indentation.
> >
> > Re-posting with indentation, comments in line.
> >
> >> Disclaimer: I did not test-run the code being proposed.
> >
> > I also did not test my comments ;)
> >
> >> sub get_page_offset
> >> {
> >>       my $default_offset = "0xc0000000";
> >>       my $page_offset;
> >>
> >>       # Allow --page-offset-32bit to over ride.
> >>       if ($page_offset_32bit != 0) {
> >>               return $page_offset_32bit;
> >>       }
> >>
> >>       $page_offset = parse_kernel_config_file();
> >>       if ($page_offset ne "") {
> >>               return $page_offset
> >>       }
> >>
> >>       printf STDERR "Failed to parse kernel config files\n";
> >>       printf STDERR "Falling back to %s\n", $default_offset;
> >>
> >>       return $default_offset;
> >> }
> >>
> >> sub parse_kernel_config_file
> >> {
> >>       my @config_files;
> >>       my $config = 'CONFIG_PAGE_OFFSET';
> >>
> >>       # Allow --kernel-config-file to over ride.
> >>       if ($kernel_config_file != "") {
> >>               @config_files = ($kernel_config_file);
> >>       } else {
> >>               my $config_file = '/boot/config-' . `uname -r`;
> >>               @config_files = ($config_file, '/boot/config');
> >>       }
> >>
> >>       if (-R "/proc/config.gz") {
> >
> > perhaps
> >                 my $tmpkconf = '/tmp/tmpkconf';
> 
> my $tmpkconf is almost as long as /tmp/tmpkconf. The name of the tmp
> file is self explanatory.
> Using a variable instead of the filename in this particular context is
> a matter of personal preference. If you prefer to use the variable
> here, it's your call.

I'm a stickler for no const strings or magic numbers but it's Kaiwan's
patch, if he puts it in with const strings I'll apply it as is :)

> >
> >>               if (system("gunzip < /proc/config.gz > /tmp/tmpkconf") == 0) {
> >>                       push @config_files, "/tmp/tmpkconf";
> >>               }
> >>       }
> >
> > Won't there only ever be a single config file? So if /proc/config.gz is
> > readable we could do
> 
> The code above builds a list of config files.
> Assigning to @config_files as shown below would wipe out the config
> files appended to the list so far, would it not?
> So $tmpkconf needs appending to the list.

You are correct, since the beginning of this function that has been the
algorithm. My observation is that if /proc/config.gz is present then we
don't need to parse the other files so it is better to blow them away.

I don't know enough about the whole Linux-sphere to know if this is
correct. But it seems reasonable that even if there is more than one way
to look at the running kernels config file they will all be the same,
the system would be pretty broken if they were different.

So once we have found a readable config file just parse it and be done
with it, no need to loop over any others.

thanks,
Tobin.

Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Kaiwan N Billimoria]

Hi,

On Wed, Nov 29, 2017 at 3:46 PM, Tobin C. Harding <me@tobin.cc> wrote:
> On Wed, Nov 29, 2017 at 09:59:59AM +0200, Alexander Kapshuk wrote:
>> On Tue, Nov 28, 2017 at 11:10 PM, Tobin C. Harding <me@tobin.cc> wrote:
>> > On Tue, Nov 28, 2017 at 03:16:24PM +0200, Alexander Kapshuk wrote:
>> >> On Tue, Nov 28, 2017 at 8:32 AM, Tobin C. Harding <me@tobin.cc> wrote:
>> >> >  Options:
>> >> >
>> >> > -       -o, --output-raw=<file>  Save results for future processing.
>> >> > -       -i, --input-raw=<file>   Read results from file instead of scanning.
>> >> > -           --raw                Show raw results (default).
>> >> > -           --suppress-dmesg     Do not show dmesg results.
>> >> > -           --squash-by-path     Show one result per unique path.
>> >> > -           --squash-by-filename Show one result per unique filename.
>> >> > -       -d, --debug              Display debugging output.
>> >> > -       -h, --help, --version    Display this help and exit.
>> >> > +       -o, --output-raw=<file>         Save results for future processing.
>> >> > +       -i, --input-raw=<file>          Read results from file instead of scanning.
>> >> > +             --raw                       Show raw results (default).
>> >> > +             --suppress-dmesg            Do not show dmesg results.
>> >> > +             --squash-by-path            Show one result per unique path.
>> >> > +             --squash-by-filename        Show one result per unique filename.
>> >> > +           --page-offset-32bit=<hex>   PAGE_OFFSET value (for 32-bit kernels).
>> >> > +           --kernel-config-file=<file> Kernel configuration file (e.g /boot/config)
>> >> > +       -d, --debug                     Display debugging output.
>> >> > +       -h, --help, --version           Display this help and exit.
>> >> >
Thanks for the whitespace fixes..

>> >>
>> >> Get_page_offset attempts to build a list of config files, which are
>> >> then passed into the parsing function for further processing.
>> >> This splits up the code to do with the config files between
>> >> get_page_offset() and parse_kernel_config_file().
>> >> May I suggest putting the kernel config file processing code into the
>> >> parse_kernel_config_file() instead, and let the parsing function
>> >> handle the config files and either return the page_offset or an empty
>> >> string.
>> >>
>> >> See below for the proposed implementation.

Thanks Alexander..

>> >
>> > Nice, this is much better! Thanks.
>> >
>> >> Apologies for the absence of indentation.
>> >
>> > Re-posting with indentation, comments in line.
>> >
>> >> Disclaimer: I did not test-run the code being proposed.
>> >
>> > I also did not test my comments ;)
>> >
>> >> sub get_page_offset
>> >> {
>> >>       my $default_offset = "0xc0000000";
>> >>       my $page_offset;
>> >>
>> >>       # Allow --page-offset-32bit to over ride.
>> >>       if ($page_offset_32bit != 0) {
>> >>               return $page_offset_32bit;
>> >>       }
>> >>
>> >>       $page_offset = parse_kernel_config_file();
>> >>       if ($page_offset ne "") {
>> >>               return $page_offset
>> >>       }
>> >>
>> >>       printf STDERR "Failed to parse kernel config files\n";
>> >>       printf STDERR "Falling back to %s\n", $default_offset;
>> >>
>> >>       return $default_offset;

This "fallback to 0xc0000000" I don't really agree with.
Obviously, there are platforms out there that do not use a PAGE_OFFSET value of
0xc0000000. So I think that defaulting to this is kinda presumptive;
much better, IMHO,
if we just fail and ask the user to pass the "correct" PAGE_OFFSET value via
the '--page-offset-32bit=' option switch.
What do you say?

>> >> }
>> >>

>> > perhaps
>> >                 my $tmpkconf = '/tmp/tmpkconf';
>>
>> my $tmpkconf is almost as long as /tmp/tmpkconf. The name of the tmp
>> file is self explanatory.
>> Using a variable instead of the filename in this particular context is
>> a matter of personal preference. If you prefer to use the variable
>> here, it's your call.
>
> I'm a stickler for no const strings or magic numbers but it's Kaiwan's
> patch, if he puts it in with const strings I'll apply it as is :)

I'd say in this case it's self-evident; IMO, we could leave it as a
const string..

>> >
>> >>               if (system("gunzip < /proc/config.gz > /tmp/tmpkconf") == 0) {
>> >>                       push @config_files, "/tmp/tmpkconf";
>> >>               }
>> >>       }
>> >
>> > Won't there only ever be a single config file? So if /proc/config.gz is
>> > readable we could do
>>
>> The code above builds a list of config files.
>> Assigning to @config_files as shown below would wipe out the config
>> files appended to the list so far, would it not?
>> So $tmpkconf needs appending to the list.
>
> You are correct, since the beginning of this function that has been the
> algorithm. My observation is that if /proc/config.gz is present then we
> don't need to parse the other files so it is better to blow them away.
>
> I don't know enough about the whole Linux-sphere to know if this is
> correct. But it seems reasonable that even if there is more than one way
> to look at the running kernels config file they will all be the same,
> the system would be pretty broken if they were different.
>
> So once we have found a readable config file just parse it and be done
> with it, no need to loop over any others.

Agreed.

> thanks,
> Tobin.

Tobin, am unsure why but I can't seem to apply your patch (on the
commit you specified: 4.15-rc1).
So have been unable to test so far.. Am copying the patch off the
email, saving and trying:

linux $ git apply --check ../tobin_patch_28nov17.patch
error: patch failed: scripts/leaking_addresses.pl:35
error: scripts/leaking_addresses.pl: patch does not apply
linux $

??

Thanks,
Kaiwan.

[kernel-hardening] Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Kaiwan N Billimoria]

Hi,

On Wed, Nov 29, 2017 at 3:46 PM, Tobin C. Harding <me@tobin.cc> wrote:
> On Wed, Nov 29, 2017 at 09:59:59AM +0200, Alexander Kapshuk wrote:
>> On Tue, Nov 28, 2017 at 11:10 PM, Tobin C. Harding <me@tobin.cc> wrote:
>> > On Tue, Nov 28, 2017 at 03:16:24PM +0200, Alexander Kapshuk wrote:
>> >> On Tue, Nov 28, 2017 at 8:32 AM, Tobin C. Harding <me@tobin.cc> wrote:
>> >> >  Options:
>> >> >
>> >> > -       -o, --output-raw=<file>  Save results for future processing.
>> >> > -       -i, --input-raw=<file>   Read results from file instead of scanning.
>> >> > -           --raw                Show raw results (default).
>> >> > -           --suppress-dmesg     Do not show dmesg results.
>> >> > -           --squash-by-path     Show one result per unique path.
>> >> > -           --squash-by-filename Show one result per unique filename.
>> >> > -       -d, --debug              Display debugging output.
>> >> > -       -h, --help, --version    Display this help and exit.
>> >> > +       -o, --output-raw=<file>         Save results for future processing.
>> >> > +       -i, --input-raw=<file>          Read results from file instead of scanning.
>> >> > +             --raw                       Show raw results (default).
>> >> > +             --suppress-dmesg            Do not show dmesg results.
>> >> > +             --squash-by-path            Show one result per unique path.
>> >> > +             --squash-by-filename        Show one result per unique filename.
>> >> > +           --page-offset-32bit=<hex>   PAGE_OFFSET value (for 32-bit kernels).
>> >> > +           --kernel-config-file=<file> Kernel configuration file (e.g /boot/config)
>> >> > +       -d, --debug                     Display debugging output.
>> >> > +       -h, --help, --version           Display this help and exit.
>> >> >
Thanks for the whitespace fixes..

>> >>
>> >> Get_page_offset attempts to build a list of config files, which are
>> >> then passed into the parsing function for further processing.
>> >> This splits up the code to do with the config files between
>> >> get_page_offset() and parse_kernel_config_file().
>> >> May I suggest putting the kernel config file processing code into the
>> >> parse_kernel_config_file() instead, and let the parsing function
>> >> handle the config files and either return the page_offset or an empty
>> >> string.
>> >>
>> >> See below for the proposed implementation.

Thanks Alexander..

>> >
>> > Nice, this is much better! Thanks.
>> >
>> >> Apologies for the absence of indentation.
>> >
>> > Re-posting with indentation, comments in line.
>> >
>> >> Disclaimer: I did not test-run the code being proposed.
>> >
>> > I also did not test my comments ;)
>> >
>> >> sub get_page_offset
>> >> {
>> >>       my $default_offset = "0xc0000000";
>> >>       my $page_offset;
>> >>
>> >>       # Allow --page-offset-32bit to over ride.
>> >>       if ($page_offset_32bit != 0) {
>> >>               return $page_offset_32bit;
>> >>       }
>> >>
>> >>       $page_offset = parse_kernel_config_file();
>> >>       if ($page_offset ne "") {
>> >>               return $page_offset
>> >>       }
>> >>
>> >>       printf STDERR "Failed to parse kernel config files\n";
>> >>       printf STDERR "Falling back to %s\n", $default_offset;
>> >>
>> >>       return $default_offset;

This "fallback to 0xc0000000" I don't really agree with.
Obviously, there are platforms out there that do not use a PAGE_OFFSET value of
0xc0000000. So I think that defaulting to this is kinda presumptive;
much better, IMHO,
if we just fail and ask the user to pass the "correct" PAGE_OFFSET value via
the '--page-offset-32bit=' option switch.
What do you say?

>> >> }
>> >>

>> > perhaps
>> >                 my $tmpkconf = '/tmp/tmpkconf';
>>
>> my $tmpkconf is almost as long as /tmp/tmpkconf. The name of the tmp
>> file is self explanatory.
>> Using a variable instead of the filename in this particular context is
>> a matter of personal preference. If you prefer to use the variable
>> here, it's your call.
>
> I'm a stickler for no const strings or magic numbers but it's Kaiwan's
> patch, if he puts it in with const strings I'll apply it as is :)

I'd say in this case it's self-evident; IMO, we could leave it as a
const string..

>> >
>> >>               if (system("gunzip < /proc/config.gz > /tmp/tmpkconf") == 0) {
>> >>                       push @config_files, "/tmp/tmpkconf";
>> >>               }
>> >>       }
>> >
>> > Won't there only ever be a single config file? So if /proc/config.gz is
>> > readable we could do
>>
>> The code above builds a list of config files.
>> Assigning to @config_files as shown below would wipe out the config
>> files appended to the list so far, would it not?
>> So $tmpkconf needs appending to the list.
>
> You are correct, since the beginning of this function that has been the
> algorithm. My observation is that if /proc/config.gz is present then we
> don't need to parse the other files so it is better to blow them away.
>
> I don't know enough about the whole Linux-sphere to know if this is
> correct. But it seems reasonable that even if there is more than one way
> to look at the running kernels config file they will all be the same,
> the system would be pretty broken if they were different.
>
> So once we have found a readable config file just parse it and be done
> with it, no need to loop over any others.

Agreed.

> thanks,
> Tobin.

Tobin, am unsure why but I can't seem to apply your patch (on the
commit you specified: 4.15-rc1).
So have been unable to test so far.. Am copying the patch off the
email, saving and trying:

linux $ git apply --check ../tobin_patch_28nov17.patch
error: patch failed: scripts/leaking_addresses.pl:35
error: scripts/leaking_addresses.pl: patch does not apply
linux $

??

Thanks,
Kaiwan.

Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Tobin C. Harding]

On Wed, Nov 29, 2017 at 04:32:54PM +0530, Kaiwan N Billimoria wrote:
> Hi,
> 
> On Wed, Nov 29, 2017 at 3:46 PM, Tobin C. Harding <me@tobin.cc> wrote:
> > On Wed, Nov 29, 2017 at 09:59:59AM +0200, Alexander Kapshuk wrote:
> >> On Tue, Nov 28, 2017 at 11:10 PM, Tobin C. Harding <me@tobin.cc> wrote:
> >> > On Tue, Nov 28, 2017 at 03:16:24PM +0200, Alexander Kapshuk wrote:
> >> >> On Tue, Nov 28, 2017 at 8:32 AM, Tobin C. Harding <me@tobin.cc> wrote:
> >> >> >  Options:
> >> >> >
> >> >> > -       -o, --output-raw=<file>  Save results for future processing.
> >> >> > -       -i, --input-raw=<file>   Read results from file instead of scanning.
> >> >> > -           --raw                Show raw results (default).
> >> >> > -           --suppress-dmesg     Do not show dmesg results.
> >> >> > -           --squash-by-path     Show one result per unique path.
> >> >> > -           --squash-by-filename Show one result per unique filename.
> >> >> > -       -d, --debug              Display debugging output.
> >> >> > -       -h, --help, --version    Display this help and exit.
> >> >> > +       -o, --output-raw=<file>         Save results for future processing.
> >> >> > +       -i, --input-raw=<file>          Read results from file instead of scanning.
> >> >> > +             --raw                       Show raw results (default).
> >> >> > +             --suppress-dmesg            Do not show dmesg results.
> >> >> > +             --squash-by-path            Show one result per unique path.
> >> >> > +             --squash-by-filename        Show one result per unique filename.
> >> >> > +           --page-offset-32bit=<hex>   PAGE_OFFSET value (for 32-bit kernels).
> >> >> > +           --kernel-config-file=<file> Kernel configuration file (e.g /boot/config)
> >> >> > +       -d, --debug                     Display debugging output.
> >> >> > +       -h, --help, --version           Display this help and exit.
> >> >> >
> Thanks for the whitespace fixes..
> 
> >> >>
> >> >> Get_page_offset attempts to build a list of config files, which are
> >> >> then passed into the parsing function for further processing.
> >> >> This splits up the code to do with the config files between
> >> >> get_page_offset() and parse_kernel_config_file().
> >> >> May I suggest putting the kernel config file processing code into the
> >> >> parse_kernel_config_file() instead, and let the parsing function
> >> >> handle the config files and either return the page_offset or an empty
> >> >> string.
> >> >>
> >> >> See below for the proposed implementation.
> 
> Thanks Alexander..
> 
> >> >
> >> > Nice, this is much better! Thanks.
> >> >
> >> >> Apologies for the absence of indentation.
> >> >
> >> > Re-posting with indentation, comments in line.
> >> >
> >> >> Disclaimer: I did not test-run the code being proposed.
> >> >
> >> > I also did not test my comments ;)
> >> >
> >> >> sub get_page_offset
> >> >> {
> >> >>       my $default_offset = "0xc0000000";
> >> >>       my $page_offset;
> >> >>
> >> >>       # Allow --page-offset-32bit to over ride.
> >> >>       if ($page_offset_32bit != 0) {
> >> >>               return $page_offset_32bit;
> >> >>       }
> >> >>
> >> >>       $page_offset = parse_kernel_config_file();
> >> >>       if ($page_offset ne "") {
> >> >>               return $page_offset
> >> >>       }
> >> >>
> >> >>       printf STDERR "Failed to parse kernel config files\n";
> >> >>       printf STDERR "Falling back to %s\n", $default_offset;
> >> >>
> >> >>       return $default_offset;
> 
> This "fallback to 0xc0000000" I don't really agree with.
> Obviously, there are platforms out there that do not use a PAGE_OFFSET value of
> 0xc0000000. So I think that defaulting to this is kinda presumptive;
> much better, IMHO,
> if we just fail and ask the user to pass the "correct" PAGE_OFFSET value via
> the '--page-offset-32bit=' option switch.
> What do you say?

If we fallback to some sane value (it does not have to be 0xc0000000
but that seems the most obvious) then the script has more chance of
running by default. Why do I think it is better to run by default even
with the wrong virtual address spilt, well since the correct value is
basically just eliminating false positives (non-kernel addresses) it
seems more right to run by default with extra false positives than to
fail and place demands on the user. This will be especially useful if we
get the script running in any continuous integration tools.

We should definitely be noisy if we fallback to the default value
though.

> >> >> }
> >> >>
> 
> >> > perhaps
> >> >                 my $tmpkconf = '/tmp/tmpkconf';
> >>
> >> my $tmpkconf is almost as long as /tmp/tmpkconf. The name of the tmp
> >> file is self explanatory.
> >> Using a variable instead of the filename in this particular context is
> >> a matter of personal preference. If you prefer to use the variable
> >> here, it's your call.
> >
> > I'm a stickler for no const strings or magic numbers but it's Kaiwan's
> > patch, if he puts it in with const strings I'll apply it as is :)
> 
> I'd say in this case it's self-evident; IMO, we could leave it as a
> const string..
> 
> >> >
> >> >>               if (system("gunzip < /proc/config.gz > /tmp/tmpkconf") == 0) {
> >> >>                       push @config_files, "/tmp/tmpkconf";
> >> >>               }
> >> >>       }
> >> >
> >> > Won't there only ever be a single config file? So if /proc/config.gz is
> >> > readable we could do
> >>
> >> The code above builds a list of config files.
> >> Assigning to @config_files as shown below would wipe out the config
> >> files appended to the list so far, would it not?
> >> So $tmpkconf needs appending to the list.
> >
> > You are correct, since the beginning of this function that has been the
> > algorithm. My observation is that if /proc/config.gz is present then we
> > don't need to parse the other files so it is better to blow them away.
> >
> > I don't know enough about the whole Linux-sphere to know if this is
> > correct. But it seems reasonable that even if there is more than one way
> > to look at the running kernels config file they will all be the same,
> > the system would be pretty broken if they were different.
> >
> > So once we have found a readable config file just parse it and be done
> > with it, no need to loop over any others.
> 
> Agreed.
> 
> > thanks,
> > Tobin.
> 
> Tobin, am unsure why but I can't seem to apply your patch (on the
> commit you specified: 4.15-rc1).
> So have been unable to test so far.. Am copying the patch off the
> email, saving and trying:
> 
> linux $ git apply --check ../tobin_patch_28nov17.patch
> error: patch failed: scripts/leaking_addresses.pl:35
> error: scripts/leaking_addresses.pl: patch does not apply
> linux $

I just tried to save and apply it on my end and it works. How are you
saving it? What email client are you using? Perhaps try to create a
simple patch yourself, email to yourself, save it and apply it to a
clean branch.

Also, if you want the commit message also you can use

	$ git am < this.patch

Sometimes you need to perform a 3 way merge, pass '-3' to `git am` to do
so.

Let me know how you go.

thanks,
Tobin.

[kernel-hardening] Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Tobin C. Harding]

On Wed, Nov 29, 2017 at 04:32:54PM +0530, Kaiwan N Billimoria wrote:
> Hi,
> 
> On Wed, Nov 29, 2017 at 3:46 PM, Tobin C. Harding <me@tobin.cc> wrote:
> > On Wed, Nov 29, 2017 at 09:59:59AM +0200, Alexander Kapshuk wrote:
> >> On Tue, Nov 28, 2017 at 11:10 PM, Tobin C. Harding <me@tobin.cc> wrote:
> >> > On Tue, Nov 28, 2017 at 03:16:24PM +0200, Alexander Kapshuk wrote:
> >> >> On Tue, Nov 28, 2017 at 8:32 AM, Tobin C. Harding <me@tobin.cc> wrote:
> >> >> >  Options:
> >> >> >
> >> >> > -       -o, --output-raw=<file>  Save results for future processing.
> >> >> > -       -i, --input-raw=<file>   Read results from file instead of scanning.
> >> >> > -           --raw                Show raw results (default).
> >> >> > -           --suppress-dmesg     Do not show dmesg results.
> >> >> > -           --squash-by-path     Show one result per unique path.
> >> >> > -           --squash-by-filename Show one result per unique filename.
> >> >> > -       -d, --debug              Display debugging output.
> >> >> > -       -h, --help, --version    Display this help and exit.
> >> >> > +       -o, --output-raw=<file>         Save results for future processing.
> >> >> > +       -i, --input-raw=<file>          Read results from file instead of scanning.
> >> >> > +             --raw                       Show raw results (default).
> >> >> > +             --suppress-dmesg            Do not show dmesg results.
> >> >> > +             --squash-by-path            Show one result per unique path.
> >> >> > +             --squash-by-filename        Show one result per unique filename.
> >> >> > +           --page-offset-32bit=<hex>   PAGE_OFFSET value (for 32-bit kernels).
> >> >> > +           --kernel-config-file=<file> Kernel configuration file (e.g /boot/config)
> >> >> > +       -d, --debug                     Display debugging output.
> >> >> > +       -h, --help, --version           Display this help and exit.
> >> >> >
> Thanks for the whitespace fixes..
> 
> >> >>
> >> >> Get_page_offset attempts to build a list of config files, which are
> >> >> then passed into the parsing function for further processing.
> >> >> This splits up the code to do with the config files between
> >> >> get_page_offset() and parse_kernel_config_file().
> >> >> May I suggest putting the kernel config file processing code into the
> >> >> parse_kernel_config_file() instead, and let the parsing function
> >> >> handle the config files and either return the page_offset or an empty
> >> >> string.
> >> >>
> >> >> See below for the proposed implementation.
> 
> Thanks Alexander..
> 
> >> >
> >> > Nice, this is much better! Thanks.
> >> >
> >> >> Apologies for the absence of indentation.
> >> >
> >> > Re-posting with indentation, comments in line.
> >> >
> >> >> Disclaimer: I did not test-run the code being proposed.
> >> >
> >> > I also did not test my comments ;)
> >> >
> >> >> sub get_page_offset
> >> >> {
> >> >>       my $default_offset = "0xc0000000";
> >> >>       my $page_offset;
> >> >>
> >> >>       # Allow --page-offset-32bit to over ride.
> >> >>       if ($page_offset_32bit != 0) {
> >> >>               return $page_offset_32bit;
> >> >>       }
> >> >>
> >> >>       $page_offset = parse_kernel_config_file();
> >> >>       if ($page_offset ne "") {
> >> >>               return $page_offset
> >> >>       }
> >> >>
> >> >>       printf STDERR "Failed to parse kernel config files\n";
> >> >>       printf STDERR "Falling back to %s\n", $default_offset;
> >> >>
> >> >>       return $default_offset;
> 
> This "fallback to 0xc0000000" I don't really agree with.
> Obviously, there are platforms out there that do not use a PAGE_OFFSET value of
> 0xc0000000. So I think that defaulting to this is kinda presumptive;
> much better, IMHO,
> if we just fail and ask the user to pass the "correct" PAGE_OFFSET value via
> the '--page-offset-32bit=' option switch.
> What do you say?

If we fallback to some sane value (it does not have to be 0xc0000000
but that seems the most obvious) then the script has more chance of
running by default. Why do I think it is better to run by default even
with the wrong virtual address spilt, well since the correct value is
basically just eliminating false positives (non-kernel addresses) it
seems more right to run by default with extra false positives than to
fail and place demands on the user. This will be especially useful if we
get the script running in any continuous integration tools.

We should definitely be noisy if we fallback to the default value
though.

> >> >> }
> >> >>
> 
> >> > perhaps
> >> >                 my $tmpkconf = '/tmp/tmpkconf';
> >>
> >> my $tmpkconf is almost as long as /tmp/tmpkconf. The name of the tmp
> >> file is self explanatory.
> >> Using a variable instead of the filename in this particular context is
> >> a matter of personal preference. If you prefer to use the variable
> >> here, it's your call.
> >
> > I'm a stickler for no const strings or magic numbers but it's Kaiwan's
> > patch, if he puts it in with const strings I'll apply it as is :)
> 
> I'd say in this case it's self-evident; IMO, we could leave it as a
> const string..
> 
> >> >
> >> >>               if (system("gunzip < /proc/config.gz > /tmp/tmpkconf") == 0) {
> >> >>                       push @config_files, "/tmp/tmpkconf";
> >> >>               }
> >> >>       }
> >> >
> >> > Won't there only ever be a single config file? So if /proc/config.gz is
> >> > readable we could do
> >>
> >> The code above builds a list of config files.
> >> Assigning to @config_files as shown below would wipe out the config
> >> files appended to the list so far, would it not?
> >> So $tmpkconf needs appending to the list.
> >
> > You are correct, since the beginning of this function that has been the
> > algorithm. My observation is that if /proc/config.gz is present then we
> > don't need to parse the other files so it is better to blow them away.
> >
> > I don't know enough about the whole Linux-sphere to know if this is
> > correct. But it seems reasonable that even if there is more than one way
> > to look at the running kernels config file they will all be the same,
> > the system would be pretty broken if they were different.
> >
> > So once we have found a readable config file just parse it and be done
> > with it, no need to loop over any others.
> 
> Agreed.
> 
> > thanks,
> > Tobin.
> 
> Tobin, am unsure why but I can't seem to apply your patch (on the
> commit you specified: 4.15-rc1).
> So have been unable to test so far.. Am copying the patch off the
> email, saving and trying:
> 
> linux $ git apply --check ../tobin_patch_28nov17.patch
> error: patch failed: scripts/leaking_addresses.pl:35
> error: scripts/leaking_addresses.pl: patch does not apply
> linux $

I just tried to save and apply it on my end and it works. How are you
saving it? What email client are you using? Perhaps try to create a
simple patch yourself, email to yourself, save it and apply it to a
clean branch.

Also, if you want the commit message also you can use

	$ git am < this.patch

Sometimes you need to perform a 3 way merge, pass '-3' to `git am` to do
so.

Let me know how you go.

thanks,
Tobin.

Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Kaiwan N Billimoria]

Hi,

Pl see my re inline below..
Will also follow up this mail with a patch with (minor) fixes for the
last one Tobin sent, and,
hopefully, that should mostly have the whole thing done (for now at least!)..

Thanks,
Kaiwan.

On Thu, Nov 30, 2017 at 2:18 AM, Tobin C. Harding <me@tobin.cc> wrote:
> On Wed, Nov 29, 2017 at 04:32:54PM +0530, Kaiwan N Billimoria wrote:

>> This "fallback to 0xc0000000" I don't really agree with.
>> Obviously, there are platforms out there that do not use a PAGE_OFFSET value of
>> 0xc0000000. So I think that defaulting to this is kinda presumptive;
>> much better, IMHO,
>> if we just fail and ask the user to pass the "correct" PAGE_OFFSET value via
>> the '--page-offset-32bit=' option switch.
>> What do you say?
>
> If we fallback to some sane value (it does not have to be 0xc0000000
> but that seems the most obvious) then the script has more chance of
> running by default. Why do I think it is better to run by default even
> with the wrong virtual address spilt, well since the correct value is
> basically just eliminating false positives (non-kernel addresses) it
> seems more right to run by default with extra false positives than to
> fail and place demands on the user. This will be especially useful if we
> get the script running in any continuous integration tools.
>
> We should definitely be noisy if we fallback to the default value
> though.

Yes, that's a valid argument. Will go with this..

> I just tried to save and apply it on my end and it works. How are you
> saving it? What email client are you using? Perhaps try to create a
> simple patch yourself, email to yourself, save it and apply it to a
> clean branch.
Huh.. wierd issues on my end, I guess.. will sort it out, thanks.

[kernel-hardening] Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Kaiwan N Billimoria]

Hi,

Pl see my re inline below..
Will also follow up this mail with a patch with (minor) fixes for the
last one Tobin sent, and,
hopefully, that should mostly have the whole thing done (for now at least!)..

Thanks,
Kaiwan.

On Thu, Nov 30, 2017 at 2:18 AM, Tobin C. Harding <me@tobin.cc> wrote:
> On Wed, Nov 29, 2017 at 04:32:54PM +0530, Kaiwan N Billimoria wrote:

>> This "fallback to 0xc0000000" I don't really agree with.
>> Obviously, there are platforms out there that do not use a PAGE_OFFSET value of
>> 0xc0000000. So I think that defaulting to this is kinda presumptive;
>> much better, IMHO,
>> if we just fail and ask the user to pass the "correct" PAGE_OFFSET value via
>> the '--page-offset-32bit=' option switch.
>> What do you say?
>
> If we fallback to some sane value (it does not have to be 0xc0000000
> but that seems the most obvious) then the script has more chance of
> running by default. Why do I think it is better to run by default even
> with the wrong virtual address spilt, well since the correct value is
> basically just eliminating false positives (non-kernel addresses) it
> seems more right to run by default with extra false positives than to
> fail and place demands on the user. This will be especially useful if we
> get the script running in any continuous integration tools.
>
> We should definitely be noisy if we fallback to the default value
> though.

Yes, that's a valid argument. Will go with this..

> I just tried to save and apply it on my end and it works. How are you
> saving it? What email client are you using? Perhaps try to create a
> simple patch yourself, email to yourself, save it and apply it to a
> clean branch.
Huh.. wierd issues on my end, I guess.. will sort it out, thanks.

Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[kaiwan.billimoria]

Hi,

Applies upon the previous one in this thread.
Found and fixed some minor issues with light testing on a 32-bit x86.
(I realize this isn't an ideal description, forgive me!).

Have also emitted a 'noisy' warning on PAGE_OFFSET fallback to 0xc00000000.

Signed-off-by: Kaiwan N Billimoria <kaiwan.billimoria@gmail.com>
---
 scripts/leaking_addresses.pl | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
index fcf1ebe0f043..3a8691a642c8 100755
--- a/scripts/leaking_addresses.pl
+++ b/scripts/leaking_addresses.pl
@@ -160,7 +160,6 @@ if (!$input_raw and ($squash_by_path or $squash_by_filename)) {
 
 if (!is_supported_architecture()) {
 	show_detected_architecture() if $debug;
-} else {
 	printf "\nScript does not support your architecture, sorry.\n";
 	printf "\nCurrently we support: \n\n";
 	foreach(@SUPPORTED_ARCHITECTURES) {
@@ -267,7 +266,7 @@ sub is_false_positive
 sub is_false_positive_ix86_32
 {
 	my ($match) = @_;
-	state $page_offset = get_page_offset(); # only gets called once
+	state $page_offset = eval get_page_offset(); # only gets called once
 
 	if ($match =~ '\b(0x)?(f|F){8}\b') {
 		return 1;
@@ -293,7 +292,7 @@ sub get_page_offset
 	}
 
 	# Allow --kernel-config-file to override.
-	if ($kernel_config_file != "") {
+	if ($kernel_config_file ne "") {
 		@config_files = ($kernel_config_file);
 	} else {
 		my $config_file = '/boot/config-' . `uname -r`;
@@ -314,14 +313,16 @@ sub get_page_offset
 	}
 
 	foreach my $config_file (@config_files) {
-		$page_offset = parse_kernel_config($config_file);
+		$page_offset = parse_kernel_config_file($config_file);
 		if ($page_offset ne "") {
 			return $page_offset;
 		}
 	}
 
-	printf STDERR "Failed to parse kernel config files\n";
-	printf STDERR "Falling back to %s\n", $default_offset;
+	printf STDERR "\nFailed to parse kernel config files\n";
+	printf STDERR "*** NOTE ***\n";
+	printf STDERR "Falling back to PAGE_OFFSET = %s\n\n", $default_offset;
+
 	return $default_offset;
 }
 
@@ -329,11 +330,13 @@ sub parse_kernel_config_file
 {
 	my ($file) = @_;
 	my $config = 'CONFIG_PAGE_OFFSET';
+	my $str = "";
+	my $val = "";
 
 	open(my $fh, "<", $file) or return "";
 	while (my $line = <$fh> ) {
 		if ($line =~ /^$config/) {
-			my ($str, $val) = split /=/, $line;
+			($str, $val) = split /=/, $line;
 			chomp($val);
 			last;
 		}
-- 
2.14.3

On Thu, 2017-11-30 at 07:48 +1100, Tobin C. Harding wrote:
> On Wed, Nov 29, 2017 at 04:32:54PM +0530, Kaiwan N Billimoria wrote:
> > Hi,
> > 
> > On Wed, Nov 29, 2017 at 3:46 PM, Tobin C. Harding <me@tobin.cc> wrote:
> > > On Wed, Nov 29, 2017 at 09:59:59AM +0200, Alexander Kapshuk wrote:
> > > > On Tue, Nov 28, 2017 at 11:10 PM, Tobin C. Harding <me@tobin.cc> wrote:
> > > > > On Tue, Nov 28, 2017 at 03:16:24PM +0200, Alexander Kapshuk wrote:
> > > > > > On Tue, Nov 28, 2017 at 8:32 AM, Tobin C. Harding <me@tobin.cc> wrote:
> > > > > > >  Options:
> > > > > > > 
> > > > > > > -       -o, --output-raw=<file>  Save results for future processing.
> > > > > > > -       -i, --input-raw=<file>   Read results from file instead of scanning.
> > > > > > > -           --raw                Show raw results (default).
> > > > > > > -           --suppress-dmesg     Do not show dmesg results.
> > > > > > > -           --squash-by-path     Show one result per unique path.
> > > > > > > -           --squash-by-filename Show one result per unique filename.
> > > > > > > -       -d, --debug              Display debugging output.
> > > > > > > -       -h, --help, --version    Display this help and exit.
> > > > > > > +       -o, --output-raw=<file>         Save results for future processing.
> > > > > > > +       -i, --input-raw=<file>          Read results from file instead of scanning.
> > > > > > > +             --raw                       Show raw results (default).
> > > > > > > +             --suppress-dmesg            Do not show dmesg results.
> > > > > > > +             --squash-by-path            Show one result per unique path.
> > > > > > > +             --squash-by-filename        Show one result per unique filename.
> > > > > > > +           --page-offset-32bit=<hex>   PAGE_OFFSET value (for 32-bit kernels).
> > > > > > > +           --kernel-config-file=<file> Kernel configuration file (e.g /boot/config)
> > > > > > > +       -d, --debug                     Display debugging output.
> > > > > > > +       -h, --help, --version           Display this help and exit.
> > > > > > > 
> > 
> > Thanks for the whitespace fixes..
> > 
> > > > > > 
> > > > > > Get_page_offset attempts to build a list of config files, which are
> > > > > > then passed into the parsing function for further processing.
> > > > > > This splits up the code to do with the config files between
> > > > > > get_page_offset() and parse_kernel_config_file().
> > > > > > May I suggest putting the kernel config file processing code into the
> > > > > > parse_kernel_config_file() instead, and let the parsing function
> > > > > > handle the config files and either return the page_offset or an empty
> > > > > > string.
> > > > > > 
> > > > > > See below for the proposed implementation.
> > 
> > Thanks Alexander..
> > 
> > > > > 
> > > > > Nice, this is much better! Thanks.
> > > > > 
> > > > > > Apologies for the absence of indentation.
> > > > > 
> > > > > Re-posting with indentation, comments in line.
> > > > > 
> > > > > > Disclaimer: I did not test-run the code being proposed.
> > > > > 
> > > > > I also did not test my comments ;)
> > > > > 
> > > > > > sub get_page_offset
> > > > > > {
> > > > > >       my $default_offset = "0xc0000000";
> > > > > >       my $page_offset;
> > > > > > 
> > > > > >       # Allow --page-offset-32bit to over ride.
> > > > > >       if ($page_offset_32bit != 0) {
> > > > > >               return $page_offset_32bit;
> > > > > >       }
> > > > > > 
> > > > > >       $page_offset = parse_kernel_config_file();
> > > > > >       if ($page_offset ne "") {
> > > > > >               return $page_offset
> > > > > >       }
> > > > > > 
> > > > > >       printf STDERR "Failed to parse kernel config files\n";
> > > > > >       printf STDERR "Falling back to %s\n", $default_offset;
> > > > > > 
> > > > > >       return $default_offset;
> > 
> > This "fallback to 0xc0000000" I don't really agree with.
> > Obviously, there are platforms out there that do not use a PAGE_OFFSET value of
> > 0xc0000000. So I think that defaulting to this is kinda presumptive;
> > much better, IMHO,
> > if we just fail and ask the user to pass the "correct" PAGE_OFFSET value via
> > the '--page-offset-32bit=' option switch.
> > What do you say?
> 
> If we fallback to some sane value (it does not have to be 0xc0000000
> but that seems the most obvious) then the script has more chance of
> running by default. Why do I think it is better to run by default even
> with the wrong virtual address spilt, well since the correct value is
> basically just eliminating false positives (non-kernel addresses) it
> seems more right to run by default with extra false positives than to
> fail and place demands on the user. This will be especially useful if we
> get the script running in any continuous integration tools.
> 
> We should definitely be noisy if we fallback to the default value
> though.
> 
> > > > > > }
> > > > > > 
> > > > > perhaps
> > > > >                 my $tmpkconf = '/tmp/tmpkconf';
> > > > 
> > > > my $tmpkconf is almost as long as /tmp/tmpkconf. The name of the tmp
> > > > file is self explanatory.
> > > > Using a variable instead of the filename in this particular context is
> > > > a matter of personal preference. If you prefer to use the variable
> > > > here, it's your call.
> > > 
> > > I'm a stickler for no const strings or magic numbers but it's Kaiwan's
> > > patch, if he puts it in with const strings I'll apply it as is :)
> > 
> > I'd say in this case it's self-evident; IMO, we could leave it as a
> > const string..
> > 
> > > > > 
> > > > > >               if (system("gunzip < /proc/config.gz > /tmp/tmpkconf") == 0) {
> > > > > >                       push @config_files, "/tmp/tmpkconf";
> > > > > >               }
> > > > > >       }
> > > > > 
> > > > > Won't there only ever be a single config file? So if /proc/config.gz is
> > > > > readable we could do
> > > > 
> > > > The code above builds a list of config files.
> > > > Assigning to @config_files as shown below would wipe out the config
> > > > files appended to the list so far, would it not?
> > > > So $tmpkconf needs appending to the list.
> > > 
> > > You are correct, since the beginning of this function that has been the
> > > algorithm. My observation is that if /proc/config.gz is present then we
> > > don't need to parse the other files so it is better to blow them away.
> > > 
> > > I don't know enough about the whole Linux-sphere to know if this is
> > > correct. But it seems reasonable that even if there is more than one way
> > > to look at the running kernels config file they will all be the same,
> > > the system would be pretty broken if they were different.
> > > 
> > > So once we have found a readable config file just parse it and be done
> > > with it, no need to loop over any others.
> > 
> > Agreed.
> > 
> > > thanks,
> > > Tobin.
> > 
> > Tobin, am unsure why but I can't seem to apply your patch (on the
> > commit you specified: 4.15-rc1).
> > So have been unable to test so far.. Am copying the patch off the
> > email, saving and trying:
> > 
> > linux $ git apply --check ../tobin_patch_28nov17.patch
> > error: patch failed: scripts/leaking_addresses.pl:35
> > error: scripts/leaking_addresses.pl: patch does not apply
> > linux $
> 
> I just tried to save and apply it on my end and it works. How are you
> saving it? What email client are you using? Perhaps try to create a
> simple patch yourself, email to yourself, save it and apply it to a
> clean branch.
> 
> Also, if you want the commit message also you can use
> 
> 	$ git am < this.patch
> 
> Sometimes you need to perform a 3 way merge, pass '-3' to `git am` to do
> so.
> 
> Let me know how you go.
> 
> thanks,
> Tobin.

[kernel-hardening] Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[kaiwan.billimoria]

Hi,

Applies upon the previous one in this thread.
Found and fixed some minor issues with light testing on a 32-bit x86.
(I realize this isn't an ideal description, forgive me!).

Have also emitted a 'noisy' warning on PAGE_OFFSET fallback to 0xc00000000.

Signed-off-by: Kaiwan N Billimoria <kaiwan.billimoria@gmail.com>
---
 scripts/leaking_addresses.pl | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
index fcf1ebe0f043..3a8691a642c8 100755
--- a/scripts/leaking_addresses.pl
+++ b/scripts/leaking_addresses.pl
@@ -160,7 +160,6 @@ if (!$input_raw and ($squash_by_path or $squash_by_filename)) {
 
 if (!is_supported_architecture()) {
 	show_detected_architecture() if $debug;
-} else {
 	printf "\nScript does not support your architecture, sorry.\n";
 	printf "\nCurrently we support: \n\n";
 	foreach(@SUPPORTED_ARCHITECTURES) {
@@ -267,7 +266,7 @@ sub is_false_positive
 sub is_false_positive_ix86_32
 {
 	my ($match) = @_;
-	state $page_offset = get_page_offset(); # only gets called once
+	state $page_offset = eval get_page_offset(); # only gets called once
 
 	if ($match =~ '\b(0x)?(f|F){8}\b') {
 		return 1;
@@ -293,7 +292,7 @@ sub get_page_offset
 	}
 
 	# Allow --kernel-config-file to override.
-	if ($kernel_config_file != "") {
+	if ($kernel_config_file ne "") {
 		@config_files = ($kernel_config_file);
 	} else {
 		my $config_file = '/boot/config-' . `uname -r`;
@@ -314,14 +313,16 @@ sub get_page_offset
 	}
 
 	foreach my $config_file (@config_files) {
-		$page_offset = parse_kernel_config($config_file);
+		$page_offset = parse_kernel_config_file($config_file);
 		if ($page_offset ne "") {
 			return $page_offset;
 		}
 	}
 
-	printf STDERR "Failed to parse kernel config files\n";
-	printf STDERR "Falling back to %s\n", $default_offset;
+	printf STDERR "\nFailed to parse kernel config files\n";
+	printf STDERR "*** NOTE ***\n";
+	printf STDERR "Falling back to PAGE_OFFSET = %s\n\n", $default_offset;
+
 	return $default_offset;
 }
 
@@ -329,11 +330,13 @@ sub parse_kernel_config_file
 {
 	my ($file) = @_;
 	my $config = 'CONFIG_PAGE_OFFSET';
+	my $str = "";
+	my $val = "";
 
 	open(my $fh, "<", $file) or return "";
 	while (my $line = <$fh> ) {
 		if ($line =~ /^$config/) {
-			my ($str, $val) = split /=/, $line;
+			($str, $val) = split /=/, $line;
 			chomp($val);
 			last;
 		}
-- 
2.14.3

On Thu, 2017-11-30 at 07:48 +1100, Tobin C. Harding wrote:
> On Wed, Nov 29, 2017 at 04:32:54PM +0530, Kaiwan N Billimoria wrote:
> > Hi,
> > 
> > On Wed, Nov 29, 2017 at 3:46 PM, Tobin C. Harding <me@tobin.cc> wrote:
> > > On Wed, Nov 29, 2017 at 09:59:59AM +0200, Alexander Kapshuk wrote:
> > > > On Tue, Nov 28, 2017 at 11:10 PM, Tobin C. Harding <me@tobin.cc> wrote:
> > > > > On Tue, Nov 28, 2017 at 03:16:24PM +0200, Alexander Kapshuk wrote:
> > > > > > On Tue, Nov 28, 2017 at 8:32 AM, Tobin C. Harding <me@tobin.cc> wrote:
> > > > > > >  Options:
> > > > > > > 
> > > > > > > -       -o, --output-raw=<file>  Save results for future processing.
> > > > > > > -       -i, --input-raw=<file>   Read results from file instead of scanning.
> > > > > > > -           --raw                Show raw results (default).
> > > > > > > -           --suppress-dmesg     Do not show dmesg results.
> > > > > > > -           --squash-by-path     Show one result per unique path.
> > > > > > > -           --squash-by-filename Show one result per unique filename.
> > > > > > > -       -d, --debug              Display debugging output.
> > > > > > > -       -h, --help, --version    Display this help and exit.
> > > > > > > +       -o, --output-raw=<file>         Save results for future processing.
> > > > > > > +       -i, --input-raw=<file>          Read results from file instead of scanning.
> > > > > > > +             --raw                       Show raw results (default).
> > > > > > > +             --suppress-dmesg            Do not show dmesg results.
> > > > > > > +             --squash-by-path            Show one result per unique path.
> > > > > > > +             --squash-by-filename        Show one result per unique filename.
> > > > > > > +           --page-offset-32bit=<hex>   PAGE_OFFSET value (for 32-bit kernels).
> > > > > > > +           --kernel-config-file=<file> Kernel configuration file (e.g /boot/config)
> > > > > > > +       -d, --debug                     Display debugging output.
> > > > > > > +       -h, --help, --version           Display this help and exit.
> > > > > > > 
> > 
> > Thanks for the whitespace fixes..
> > 
> > > > > > 
> > > > > > Get_page_offset attempts to build a list of config files, which are
> > > > > > then passed into the parsing function for further processing.
> > > > > > This splits up the code to do with the config files between
> > > > > > get_page_offset() and parse_kernel_config_file().
> > > > > > May I suggest putting the kernel config file processing code into the
> > > > > > parse_kernel_config_file() instead, and let the parsing function
> > > > > > handle the config files and either return the page_offset or an empty
> > > > > > string.
> > > > > > 
> > > > > > See below for the proposed implementation.
> > 
> > Thanks Alexander..
> > 
> > > > > 
> > > > > Nice, this is much better! Thanks.
> > > > > 
> > > > > > Apologies for the absence of indentation.
> > > > > 
> > > > > Re-posting with indentation, comments in line.
> > > > > 
> > > > > > Disclaimer: I did not test-run the code being proposed.
> > > > > 
> > > > > I also did not test my comments ;)
> > > > > 
> > > > > > sub get_page_offset
> > > > > > {
> > > > > >       my $default_offset = "0xc0000000";
> > > > > >       my $page_offset;
> > > > > > 
> > > > > >       # Allow --page-offset-32bit to over ride.
> > > > > >       if ($page_offset_32bit != 0) {
> > > > > >               return $page_offset_32bit;
> > > > > >       }
> > > > > > 
> > > > > >       $page_offset = parse_kernel_config_file();
> > > > > >       if ($page_offset ne "") {
> > > > > >               return $page_offset
> > > > > >       }
> > > > > > 
> > > > > >       printf STDERR "Failed to parse kernel config files\n";
> > > > > >       printf STDERR "Falling back to %s\n", $default_offset;
> > > > > > 
> > > > > >       return $default_offset;
> > 
> > This "fallback to 0xc0000000" I don't really agree with.
> > Obviously, there are platforms out there that do not use a PAGE_OFFSET value of
> > 0xc0000000. So I think that defaulting to this is kinda presumptive;
> > much better, IMHO,
> > if we just fail and ask the user to pass the "correct" PAGE_OFFSET value via
> > the '--page-offset-32bit=' option switch.
> > What do you say?
> 
> If we fallback to some sane value (it does not have to be 0xc0000000
> but that seems the most obvious) then the script has more chance of
> running by default. Why do I think it is better to run by default even
> with the wrong virtual address spilt, well since the correct value is
> basically just eliminating false positives (non-kernel addresses) it
> seems more right to run by default with extra false positives than to
> fail and place demands on the user. This will be especially useful if we
> get the script running in any continuous integration tools.
> 
> We should definitely be noisy if we fallback to the default value
> though.
> 
> > > > > > }
> > > > > > 
> > > > > perhaps
> > > > >                 my $tmpkconf = '/tmp/tmpkconf';
> > > > 
> > > > my $tmpkconf is almost as long as /tmp/tmpkconf. The name of the tmp
> > > > file is self explanatory.
> > > > Using a variable instead of the filename in this particular context is
> > > > a matter of personal preference. If you prefer to use the variable
> > > > here, it's your call.
> > > 
> > > I'm a stickler for no const strings or magic numbers but it's Kaiwan's
> > > patch, if he puts it in with const strings I'll apply it as is :)
> > 
> > I'd say in this case it's self-evident; IMO, we could leave it as a
> > const string..
> > 
> > > > > 
> > > > > >               if (system("gunzip < /proc/config.gz > /tmp/tmpkconf") == 0) {
> > > > > >                       push @config_files, "/tmp/tmpkconf";
> > > > > >               }
> > > > > >       }
> > > > > 
> > > > > Won't there only ever be a single config file? So if /proc/config.gz is
> > > > > readable we could do
> > > > 
> > > > The code above builds a list of config files.
> > > > Assigning to @config_files as shown below would wipe out the config
> > > > files appended to the list so far, would it not?
> > > > So $tmpkconf needs appending to the list.
> > > 
> > > You are correct, since the beginning of this function that has been the
> > > algorithm. My observation is that if /proc/config.gz is present then we
> > > don't need to parse the other files so it is better to blow them away.
> > > 
> > > I don't know enough about the whole Linux-sphere to know if this is
> > > correct. But it seems reasonable that even if there is more than one way
> > > to look at the running kernels config file they will all be the same,
> > > the system would be pretty broken if they were different.
> > > 
> > > So once we have found a readable config file just parse it and be done
> > > with it, no need to loop over any others.
> > 
> > Agreed.
> > 
> > > thanks,
> > > Tobin.
> > 
> > Tobin, am unsure why but I can't seem to apply your patch (on the
> > commit you specified: 4.15-rc1).
> > So have been unable to test so far.. Am copying the patch off the
> > email, saving and trying:
> > 
> > linux $ git apply --check ../tobin_patch_28nov17.patch
> > error: patch failed: scripts/leaking_addresses.pl:35
> > error: scripts/leaking_addresses.pl: patch does not apply
> > linux $
> 
> I just tried to save and apply it on my end and it works. How are you
> saving it? What email client are you using? Perhaps try to create a
> simple patch yourself, email to yourself, save it and apply it to a
> clean branch.
> 
> Also, if you want the commit message also you can use
> 
> 	$ git am < this.patch
> 
> Sometimes you need to perform a 3 way merge, pass '-3' to `git am` to do
> so.
> 
> Let me know how you go.
> 
> thanks,
> Tobin.

Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Tobin C. Harding]

On Fri, Dec 01, 2017 at 06:39:07PM +0530, kaiwan.billimoria@gmail.com wrote:
> Hi,
> 
> Applies upon the previous one in this thread.
> Found and fixed some minor issues with light testing on a 32-bit x86.
> (I realize this isn't an ideal description, forgive me!).
> 
> Have also emitted a 'noisy' warning on PAGE_OFFSET fallback to 0xc00000000.
> 
> Signed-off-by: Kaiwan N Billimoria <kaiwan.billimoria@gmail.com>
> ---
>  scripts/leaking_addresses.pl | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> index fcf1ebe0f043..3a8691a642c8 100755
> --- a/scripts/leaking_addresses.pl
> +++ b/scripts/leaking_addresses.pl
> @@ -160,7 +160,6 @@ if (!$input_raw and ($squash_by_path or $squash_by_filename)) {
>  
>  if (!is_supported_architecture()) {
>  	show_detected_architecture() if $debug;
> -} else {
>  	printf "\nScript does not support your architecture, sorry.\n";
>  	printf "\nCurrently we support: \n\n";
>  	foreach(@SUPPORTED_ARCHITECTURES) {
> @@ -267,7 +266,7 @@ sub is_false_positive
>  sub is_false_positive_ix86_32
>  {
>  	my ($match) = @_;
> -	state $page_offset = get_page_offset(); # only gets called once
> +	state $page_offset = eval get_page_offset(); # only gets called once

Why do you use 'eval' here?

thanks,
Tobin.

[kernel-hardening] Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Tobin C. Harding]

On Fri, Dec 01, 2017 at 06:39:07PM +0530, kaiwan.billimoria@gmail.com wrote:
> Hi,
> 
> Applies upon the previous one in this thread.
> Found and fixed some minor issues with light testing on a 32-bit x86.
> (I realize this isn't an ideal description, forgive me!).
> 
> Have also emitted a 'noisy' warning on PAGE_OFFSET fallback to 0xc00000000.
> 
> Signed-off-by: Kaiwan N Billimoria <kaiwan.billimoria@gmail.com>
> ---
>  scripts/leaking_addresses.pl | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> index fcf1ebe0f043..3a8691a642c8 100755
> --- a/scripts/leaking_addresses.pl
> +++ b/scripts/leaking_addresses.pl
> @@ -160,7 +160,6 @@ if (!$input_raw and ($squash_by_path or $squash_by_filename)) {
>  
>  if (!is_supported_architecture()) {
>  	show_detected_architecture() if $debug;
> -} else {
>  	printf "\nScript does not support your architecture, sorry.\n";
>  	printf "\nCurrently we support: \n\n";
>  	foreach(@SUPPORTED_ARCHITECTURES) {
> @@ -267,7 +266,7 @@ sub is_false_positive
>  sub is_false_positive_ix86_32
>  {
>  	my ($match) = @_;
> -	state $page_offset = get_page_offset(); # only gets called once
> +	state $page_offset = eval get_page_offset(); # only gets called once

Why do you use 'eval' here?

thanks,
Tobin.

Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[kaiwan.billimoria]

On Mon, 2017-12-04 at 11:11 +1100, Tobin C. Harding wrote:
> On Fri, Dec 01, 2017 at 06:39:07PM +0530, kaiwan.billimoria@gmail.com wrote:

> > @@ -267,7 +266,7 @@ sub is_false_positive
> >  sub is_false_positive_ix86_32
> >  {
> >  	my ($match) = @_;
> > -	state $page_offset = get_page_offset(); # only gets called once
> > +	state $page_offset = eval get_page_offset(); # only gets called once
> 
> Why do you use 'eval' here?
> 
Without the eval:
i.e.
  state $page_offset = get_page_offset(); # only gets called once

$ ./leaking_addresses.pl  |head -200
Argument "0x80000000" isn't numeric in numeric lt (<) at ./leaking_addresses.pl line 277.
...

With the 'eval', no warning, it's fine.


Additional Comments:

a) When running in debug mode, print the arch we're currently running on
b) Also, while checking, I found another bug; requires the fix below (strip the filename of LF).

Patch follows:

---
diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
index 3a8691a642c8..9906dcf8b807 100755
--- a/scripts/leaking_addresses.pl
+++ b/scripts/leaking_addresses.pl
@@ -158,8 +158,8 @@ if (!$input_raw and ($squash_by_path or $squash_by_filename)) {
        exit(128);
 }
 
+show_detected_architecture() if $debug;
 if (!is_supported_architecture()) {
-       show_detected_architecture() if $debug;
        printf "\nScript does not support your architecture, sorry.\n";
        printf "\nCurrently we support: \n\n";
        foreach(@SUPPORTED_ARCHITECTURES) {
@@ -313,6 +313,7 @@ sub get_page_offset
        }
 
        foreach my $config_file (@config_files) {
+               $config_file =~ s/\R*//g;
                $page_offset = parse_kernel_config_file($config_file);
                if ($page_offset ne "") {
                        return $page_offset;



Thanks,
Kaiwan.


> thanks,
> Tobin.

[kernel-hardening] Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[kaiwan.billimoria]

On Mon, 2017-12-04 at 11:11 +1100, Tobin C. Harding wrote:
> On Fri, Dec 01, 2017 at 06:39:07PM +0530, kaiwan.billimoria@gmail.com wrote:

> > @@ -267,7 +266,7 @@ sub is_false_positive
> >  sub is_false_positive_ix86_32
> >  {
> >  	my ($match) = @_;
> > -	state $page_offset = get_page_offset(); # only gets called once
> > +	state $page_offset = eval get_page_offset(); # only gets called once
> 
> Why do you use 'eval' here?
> 
Without the eval:
i.e.
  state $page_offset = get_page_offset(); # only gets called once

$ ./leaking_addresses.pl  |head -200
Argument "0x80000000" isn't numeric in numeric lt (<) at ./leaking_addresses.pl line 277.
...

With the 'eval', no warning, it's fine.


Additional Comments:

a) When running in debug mode, print the arch we're currently running on
b) Also, while checking, I found another bug; requires the fix below (strip the filename of LF).

Patch follows:

---
diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
index 3a8691a642c8..9906dcf8b807 100755
--- a/scripts/leaking_addresses.pl
+++ b/scripts/leaking_addresses.pl
@@ -158,8 +158,8 @@ if (!$input_raw and ($squash_by_path or $squash_by_filename)) {
        exit(128);
 }
 
+show_detected_architecture() if $debug;
 if (!is_supported_architecture()) {
-       show_detected_architecture() if $debug;
        printf "\nScript does not support your architecture, sorry.\n";
        printf "\nCurrently we support: \n\n";
        foreach(@SUPPORTED_ARCHITECTURES) {
@@ -313,6 +313,7 @@ sub get_page_offset
        }
 
        foreach my $config_file (@config_files) {
+               $config_file =~ s/\R*//g;
                $page_offset = parse_kernel_config_file($config_file);
                if ($page_offset ne "") {
                        return $page_offset;



Thanks,
Kaiwan.


> thanks,
> Tobin.

Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Tobin C. Harding]

On Mon, Dec 04, 2017 at 10:11:21AM +0530, kaiwan.billimoria@gmail.com wrote:
> On Mon, 2017-12-04 at 11:11 +1100, Tobin C. Harding wrote:
> > On Fri, Dec 01, 2017 at 06:39:07PM +0530, kaiwan.billimoria@gmail.com wrote:
> 
> > > @@ -267,7 +266,7 @@ sub is_false_positive
> > >  sub is_false_positive_ix86_32
> > >  {
> > >  	my ($match) = @_;
> > > -	state $page_offset = get_page_offset(); # only gets called once
> > > +	state $page_offset = eval get_page_offset(); # only gets called once
> > 
> > Why do you use 'eval' here?
> > 
> Without the eval:
> i.e.
>   state $page_offset = get_page_offset(); # only gets called once
> 
> $ ./leaking_addresses.pl  |head -200
> Argument "0x80000000" isn't numeric in numeric lt (<) at ./leaking_addresses.pl line 277.
> ...
> 
> With the 'eval', no warning, it's fine.

Why not use hex()?

> Additional Comments:
> 
> a) When running in debug mode, print the arch we're currently running on
> b) Also, while checking, I found another bug; requires the fix below (strip the filename of LF).
> 
> Patch follows:
> 
> ---
> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> index 3a8691a642c8..9906dcf8b807 100755
> --- a/scripts/leaking_addresses.pl
> +++ b/scripts/leaking_addresses.pl
> @@ -158,8 +158,8 @@ if (!$input_raw and ($squash_by_path or $squash_by_filename)) {
>         exit(128);
>  }
>  
> +show_detected_architecture() if $debug;
>  if (!is_supported_architecture()) {
> -       show_detected_architecture() if $debug;
>         printf "\nScript does not support your architecture, sorry.\n";
>         printf "\nCurrently we support: \n\n";
>         foreach(@SUPPORTED_ARCHITECTURES) {
> @@ -313,6 +313,7 @@ sub get_page_offset
>         }
>  
>         foreach my $config_file (@config_files) {
> +               $config_file =~ s/\R*//g;

Is there some reason you don't use chomp()?

thanks,
Tobin.

[kernel-hardening] Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Tobin C. Harding]

On Mon, Dec 04, 2017 at 10:11:21AM +0530, kaiwan.billimoria@gmail.com wrote:
> On Mon, 2017-12-04 at 11:11 +1100, Tobin C. Harding wrote:
> > On Fri, Dec 01, 2017 at 06:39:07PM +0530, kaiwan.billimoria@gmail.com wrote:
> 
> > > @@ -267,7 +266,7 @@ sub is_false_positive
> > >  sub is_false_positive_ix86_32
> > >  {
> > >  	my ($match) = @_;
> > > -	state $page_offset = get_page_offset(); # only gets called once
> > > +	state $page_offset = eval get_page_offset(); # only gets called once
> > 
> > Why do you use 'eval' here?
> > 
> Without the eval:
> i.e.
>   state $page_offset = get_page_offset(); # only gets called once
> 
> $ ./leaking_addresses.pl  |head -200
> Argument "0x80000000" isn't numeric in numeric lt (<) at ./leaking_addresses.pl line 277.
> ...
> 
> With the 'eval', no warning, it's fine.

Why not use hex()?

> Additional Comments:
> 
> a) When running in debug mode, print the arch we're currently running on
> b) Also, while checking, I found another bug; requires the fix below (strip the filename of LF).
> 
> Patch follows:
> 
> ---
> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> index 3a8691a642c8..9906dcf8b807 100755
> --- a/scripts/leaking_addresses.pl
> +++ b/scripts/leaking_addresses.pl
> @@ -158,8 +158,8 @@ if (!$input_raw and ($squash_by_path or $squash_by_filename)) {
>         exit(128);
>  }
>  
> +show_detected_architecture() if $debug;
>  if (!is_supported_architecture()) {
> -       show_detected_architecture() if $debug;
>         printf "\nScript does not support your architecture, sorry.\n";
>         printf "\nCurrently we support: \n\n";
>         foreach(@SUPPORTED_ARCHITECTURES) {
> @@ -313,6 +313,7 @@ sub get_page_offset
>         }
>  
>         foreach my $config_file (@config_files) {
> +               $config_file =~ s/\R*//g;

Is there some reason you don't use chomp()?

thanks,
Tobin.

Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Kaiwan N Billimoria]

On Mon, Dec 4, 2017 at 10:25 AM, Tobin C. Harding <me@tobin.cc> wrote:
>
> > With the 'eval', no warning, it's fine.
>
> Why not use hex()?

> >
> >         foreach my $config_file (@config_files) {
> > +               $config_file =~ s/\R*//g;
>
> Is there some reason you don't use chomp()?

Tobin, now you can see that I'm indeed a Perl newbie noob! :-)
Will follow your suggestions.. indeed, feel free to go ahead with Perl-stuff
that's more appropriate than my (very amateur) perl!

thanks, Kaiwan.

> thanks,
> Tobin.

[kernel-hardening] Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Kaiwan N Billimoria]

On Mon, Dec 4, 2017 at 10:25 AM, Tobin C. Harding <me@tobin.cc> wrote:
>
> > With the 'eval', no warning, it's fine.
>
> Why not use hex()?

> >
> >         foreach my $config_file (@config_files) {
> > +               $config_file =~ s/\R*//g;
>
> Is there some reason you don't use chomp()?

Tobin, now you can see that I'm indeed a Perl newbie noob! :-)
Will follow your suggestions.. indeed, feel free to go ahead with Perl-stuff
that's more appropriate than my (very amateur) perl!

thanks, Kaiwan.

> thanks,
> Tobin.

Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Kaiwan N Billimoria]

> On Mon, Dec 4, 2017 at 10:25 AM, Tobin C. Harding <me@tobin.cc> wrote:
>>
>> > With the 'eval', no warning, it's fine.
>>
>> Why not use hex()?
>
>> >
>> >         foreach my $config_file (@config_files) {
>> > +               $config_file =~ s/\R*//g;
>>
>> Is there some reason you don't use chomp()?
>

Wrt your suggestions:

---
diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
index 9906dcf8b807..260b52e456f1 100755
--- a/scripts/leaking_addresses.pl
+++ b/scripts/leaking_addresses.pl
@@ -266,7 +266,7 @@ sub is_false_positive
 sub is_false_positive_ix86_32
 {
        my ($match) = @_;
-       state $page_offset = eval get_page_offset(); # only gets called once
+       state $page_offset = hex get_page_offset(); # only gets called once

        if ($match =~ '\b(0x)?(f|F){8}\b') {
                return 1;
@@ -313,7 +313,7 @@ sub get_page_offset
        }

        foreach my $config_file (@config_files) {
-               $config_file =~ s/\R*//g;
+               chomp $config_file;
                $page_offset = parse_kernel_config_file($config_file);
                if ($page_offset ne "") {
                        return $page_offset;


Thanks & Regards,
Kaiwan.

[kernel-hardening] Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Kaiwan N Billimoria]

> On Mon, Dec 4, 2017 at 10:25 AM, Tobin C. Harding <me@tobin.cc> wrote:
>>
>> > With the 'eval', no warning, it's fine.
>>
>> Why not use hex()?
>
>> >
>> >         foreach my $config_file (@config_files) {
>> > +               $config_file =~ s/\R*//g;
>>
>> Is there some reason you don't use chomp()?
>

Wrt your suggestions:

---
diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
index 9906dcf8b807..260b52e456f1 100755
--- a/scripts/leaking_addresses.pl
+++ b/scripts/leaking_addresses.pl
@@ -266,7 +266,7 @@ sub is_false_positive
 sub is_false_positive_ix86_32
 {
        my ($match) = @_;
-       state $page_offset = eval get_page_offset(); # only gets called once
+       state $page_offset = hex get_page_offset(); # only gets called once

        if ($match =~ '\b(0x)?(f|F){8}\b') {
                return 1;
@@ -313,7 +313,7 @@ sub get_page_offset
        }

        foreach my $config_file (@config_files) {
-               $config_file =~ s/\R*//g;
+               chomp $config_file;
                $page_offset = parse_kernel_config_file($config_file);
                if ($page_offset ne "") {
                        return $page_offset;


Thanks & Regards,
Kaiwan.

Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Tobin C. Harding]

On Mon, Dec 04, 2017 at 10:51:53AM +0530, Kaiwan N Billimoria wrote:
> > On Mon, Dec 4, 2017 at 10:25 AM, Tobin C. Harding <me@tobin.cc> wrote:
> >>
> >> > With the 'eval', no warning, it's fine.
> >>
> >> Why not use hex()?
> >
> >> >
> >> >         foreach my $config_file (@config_files) {
> >> > +               $config_file =~ s/\R*//g;
> >>
> >> Is there some reason you don't use chomp()?
> >
> 
> Wrt your suggestions:
> 
> ---
> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> index 9906dcf8b807..260b52e456f1 100755
> --- a/scripts/leaking_addresses.pl
> +++ b/scripts/leaking_addresses.pl
> @@ -266,7 +266,7 @@ sub is_false_positive
>  sub is_false_positive_ix86_32
>  {
>         my ($match) = @_;
> -       state $page_offset = eval get_page_offset(); # only gets called once
> +       state $page_offset = hex get_page_offset(); # only gets called once

I don't think this is valid ;) I meant use hex() to convert the string
to an int so it doesn't throw the warning (inside get_page_offset()).

thanks,
Tobin.

[kernel-hardening] Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Tobin C. Harding]

On Mon, Dec 04, 2017 at 10:51:53AM +0530, Kaiwan N Billimoria wrote:
> > On Mon, Dec 4, 2017 at 10:25 AM, Tobin C. Harding <me@tobin.cc> wrote:
> >>
> >> > With the 'eval', no warning, it's fine.
> >>
> >> Why not use hex()?
> >
> >> >
> >> >         foreach my $config_file (@config_files) {
> >> > +               $config_file =~ s/\R*//g;
> >>
> >> Is there some reason you don't use chomp()?
> >
> 
> Wrt your suggestions:
> 
> ---
> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> index 9906dcf8b807..260b52e456f1 100755
> --- a/scripts/leaking_addresses.pl
> +++ b/scripts/leaking_addresses.pl
> @@ -266,7 +266,7 @@ sub is_false_positive
>  sub is_false_positive_ix86_32
>  {
>         my ($match) = @_;
> -       state $page_offset = eval get_page_offset(); # only gets called once
> +       state $page_offset = hex get_page_offset(); # only gets called once

I don't think this is valid ;) I meant use hex() to convert the string
to an int so it doesn't throw the warning (inside get_page_offset()).

thanks,
Tobin.

Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[kaiwan.billimoria]

On Mon, 2017-12-04 at 19:21 +1100, Tobin C. Harding wrote:
> On Mon, Dec 04, 2017 at 10:51:53AM +0530, Kaiwan N Billimoria wrote:
> > > ---
> > diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> > index 9906dcf8b807..260b52e456f1 100755
> > --- a/scripts/leaking_addresses.pl
> > +++ b/scripts/leaking_addresses.pl
> > @@ -266,7 +266,7 @@ sub is_false_positive
> >  sub is_false_positive_ix86_32
> >  {
> >         my ($match) = @_;
> > -       state $page_offset = eval get_page_offset(); # only gets called once
> > +       state $page_offset = hex get_page_offset(); # only gets called once
> 
> I don't think this is valid ;) I meant use hex() to convert the string
> to an int so it doesn't throw the warning (inside get_page_offset()).

Yup, got it, thanks   :-p
Combined patch below:


---
 scripts/leaking_addresses.pl | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
index 9906dcf8b807..a595a2c66b12 100755
--- a/scripts/leaking_addresses.pl
+++ b/scripts/leaking_addresses.pl
@@ -266,8 +266,7 @@ sub is_false_positive
 sub is_false_positive_ix86_32
 {
 	my ($match) = @_;
-	state $page_offset = eval get_page_offset(); # only gets called once
-
+	state $page_offset = get_page_offset(); # only gets called once
 	if ($match =~ '\b(0x)?(f|F){8}\b') {
 		return 1;
 	}
@@ -283,7 +282,7 @@ sub is_false_positive_ix86_32
 sub get_page_offset
 {
 	my $page_offset;
-	my $default_offset = "0xc0000000";
+	my $default_offset = hex("0xc0000000");
 	my @config_files;
 
 	# Allow --page-offset-32bit to override.
@@ -306,23 +305,23 @@ sub get_page_offset
 		} else {
 			$page_offset = parse_kernel_config_file($tmp_file);
 			if ($page_offset ne "") {
-				return $page_offset;
+				return hex($page_offset);
 			}
 		}
 		system("rm -f $tmp_file");
 	}
 
 	foreach my $config_file (@config_files) {
-		$config_file =~ s/\R*//g;
+		chomp $config_file;
 		$page_offset = parse_kernel_config_file($config_file);
 		if ($page_offset ne "") {
-			return $page_offset;
+			return hex($page_offset);
 		}
 	}
 
 	printf STDERR "\nFailed to parse kernel config files\n";
 	printf STDERR "*** NOTE ***\n";
-	printf STDERR "Falling back to PAGE_OFFSET = %s\n\n", $default_offset;
+	printf STDERR "Falling back to PAGE_OFFSET = 0x%x\n\n", $default_offset;
 
 	return $default_offset;
 }
-- 
2.14.3

Thanks,
Kaiwan.

> thanks,
> Tobin.

[kernel-hardening] Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[kaiwan.billimoria]

On Mon, 2017-12-04 at 19:21 +1100, Tobin C. Harding wrote:
> On Mon, Dec 04, 2017 at 10:51:53AM +0530, Kaiwan N Billimoria wrote:
> > > ---
> > diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> > index 9906dcf8b807..260b52e456f1 100755
> > --- a/scripts/leaking_addresses.pl
> > +++ b/scripts/leaking_addresses.pl
> > @@ -266,7 +266,7 @@ sub is_false_positive
> >  sub is_false_positive_ix86_32
> >  {
> >         my ($match) = @_;
> > -       state $page_offset = eval get_page_offset(); # only gets called once
> > +       state $page_offset = hex get_page_offset(); # only gets called once
> 
> I don't think this is valid ;) I meant use hex() to convert the string
> to an int so it doesn't throw the warning (inside get_page_offset()).

Yup, got it, thanks   :-p
Combined patch below:


---
 scripts/leaking_addresses.pl | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
index 9906dcf8b807..a595a2c66b12 100755
--- a/scripts/leaking_addresses.pl
+++ b/scripts/leaking_addresses.pl
@@ -266,8 +266,7 @@ sub is_false_positive
 sub is_false_positive_ix86_32
 {
 	my ($match) = @_;
-	state $page_offset = eval get_page_offset(); # only gets called once
-
+	state $page_offset = get_page_offset(); # only gets called once
 	if ($match =~ '\b(0x)?(f|F){8}\b') {
 		return 1;
 	}
@@ -283,7 +282,7 @@ sub is_false_positive_ix86_32
 sub get_page_offset
 {
 	my $page_offset;
-	my $default_offset = "0xc0000000";
+	my $default_offset = hex("0xc0000000");
 	my @config_files;
 
 	# Allow --page-offset-32bit to override.
@@ -306,23 +305,23 @@ sub get_page_offset
 		} else {
 			$page_offset = parse_kernel_config_file($tmp_file);
 			if ($page_offset ne "") {
-				return $page_offset;
+				return hex($page_offset);
 			}
 		}
 		system("rm -f $tmp_file");
 	}
 
 	foreach my $config_file (@config_files) {
-		$config_file =~ s/\R*//g;
+		chomp $config_file;
 		$page_offset = parse_kernel_config_file($config_file);
 		if ($page_offset ne "") {
-			return $page_offset;
+			return hex($page_offset);
 		}
 	}
 
 	printf STDERR "\nFailed to parse kernel config files\n";
 	printf STDERR "*** NOTE ***\n";
-	printf STDERR "Falling back to PAGE_OFFSET = %s\n\n", $default_offset;
+	printf STDERR "Falling back to PAGE_OFFSET = 0x%x\n\n", $default_offset;
 
 	return $default_offset;
 }
-- 
2.14.3

Thanks,
Kaiwan.

> thanks,
> Tobin.

Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Alexander Kapshuk]

On Mon, Dec 4, 2017 at 12:20 PM,  <kaiwan.billimoria@gmail.com> wrote:
> On Mon, 2017-12-04 at 19:21 +1100, Tobin C. Harding wrote:
>> On Mon, Dec 04, 2017 at 10:51:53AM +0530, Kaiwan N Billimoria wrote:
>> > > ---
>> > diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
>> > index 9906dcf8b807..260b52e456f1 100755
>> > --- a/scripts/leaking_addresses.pl
>> > +++ b/scripts/leaking_addresses.pl
>> > @@ -266,7 +266,7 @@ sub is_false_positive
>> >  sub is_false_positive_ix86_32
>> >  {
>> >         my ($match) = @_;
>> > -       state $page_offset = eval get_page_offset(); # only gets called once
>> > +       state $page_offset = hex get_page_offset(); # only gets called once
>>
>> I don't think this is valid ;) I meant use hex() to convert the string
>> to an int so it doesn't throw the warning (inside get_page_offset()).
>
> Yup, got it, thanks   :-p
> Combined patch below:
>
>
> ---
>  scripts/leaking_addresses.pl | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> index 9906dcf8b807..a595a2c66b12 100755
> --- a/scripts/leaking_addresses.pl
> +++ b/scripts/leaking_addresses.pl
> @@ -266,8 +266,7 @@ sub is_false_positive
>  sub is_false_positive_ix86_32
>  {
>         my ($match) = @_;
> -       state $page_offset = eval get_page_offset(); # only gets called once
> -
> +       state $page_offset = get_page_offset(); # only gets called once
>         if ($match =~ '\b(0x)?(f|F){8}\b') {
>                 return 1;
>         }
> @@ -283,7 +282,7 @@ sub is_false_positive_ix86_32
>  sub get_page_offset
>  {
>         my $page_offset;
> -       my $default_offset = "0xc0000000";
> +       my $default_offset = hex("0xc0000000");
>         my @config_files;
>
>         # Allow --page-offset-32bit to override.
> @@ -306,23 +305,23 @@ sub get_page_offset
>                 } else {
>                         $page_offset = parse_kernel_config_file($tmp_file);
>                         if ($page_offset ne "") {
> -                               return $page_offset;
> +                               return hex($page_offset);
>                         }
>                 }
>                 system("rm -f $tmp_file");
>         }
>
>         foreach my $config_file (@config_files) {
> -               $config_file =~ s/\R*//g;
> +               chomp $config_file;
>                 $page_offset = parse_kernel_config_file($config_file);
>                 if ($page_offset ne "") {
> -                       return $page_offset;
> +                       return hex($page_offset);
>                 }
>         }
>
>         printf STDERR "\nFailed to parse kernel config files\n";
>         printf STDERR "*** NOTE ***\n";
> -       printf STDERR "Falling back to PAGE_OFFSET = %s\n\n", $default_offset;
> +       printf STDERR "Falling back to PAGE_OFFSET = 0x%x\n\n", $default_offset;

Better use the '#' flag with the 'x' conversion specifier:
perl -e 'my $default_offset = hex("0xc0000000");printf "%#x\n", $default_offset'
0xc0000000

>
>         return $default_offset;
>  }
> --
> 2.14.3
>
> Thanks,
> Kaiwan.
>
>> thanks,
>> Tobin.

[kernel-hardening] Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Alexander Kapshuk]

On Mon, Dec 4, 2017 at 12:20 PM,  <kaiwan.billimoria@gmail.com> wrote:
> On Mon, 2017-12-04 at 19:21 +1100, Tobin C. Harding wrote:
>> On Mon, Dec 04, 2017 at 10:51:53AM +0530, Kaiwan N Billimoria wrote:
>> > > ---
>> > diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
>> > index 9906dcf8b807..260b52e456f1 100755
>> > --- a/scripts/leaking_addresses.pl
>> > +++ b/scripts/leaking_addresses.pl
>> > @@ -266,7 +266,7 @@ sub is_false_positive
>> >  sub is_false_positive_ix86_32
>> >  {
>> >         my ($match) = @_;
>> > -       state $page_offset = eval get_page_offset(); # only gets called once
>> > +       state $page_offset = hex get_page_offset(); # only gets called once
>>
>> I don't think this is valid ;) I meant use hex() to convert the string
>> to an int so it doesn't throw the warning (inside get_page_offset()).
>
> Yup, got it, thanks   :-p
> Combined patch below:
>
>
> ---
>  scripts/leaking_addresses.pl | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> index 9906dcf8b807..a595a2c66b12 100755
> --- a/scripts/leaking_addresses.pl
> +++ b/scripts/leaking_addresses.pl
> @@ -266,8 +266,7 @@ sub is_false_positive
>  sub is_false_positive_ix86_32
>  {
>         my ($match) = @_;
> -       state $page_offset = eval get_page_offset(); # only gets called once
> -
> +       state $page_offset = get_page_offset(); # only gets called once
>         if ($match =~ '\b(0x)?(f|F){8}\b') {
>                 return 1;
>         }
> @@ -283,7 +282,7 @@ sub is_false_positive_ix86_32
>  sub get_page_offset
>  {
>         my $page_offset;
> -       my $default_offset = "0xc0000000";
> +       my $default_offset = hex("0xc0000000");
>         my @config_files;
>
>         # Allow --page-offset-32bit to override.
> @@ -306,23 +305,23 @@ sub get_page_offset
>                 } else {
>                         $page_offset = parse_kernel_config_file($tmp_file);
>                         if ($page_offset ne "") {
> -                               return $page_offset;
> +                               return hex($page_offset);
>                         }
>                 }
>                 system("rm -f $tmp_file");
>         }
>
>         foreach my $config_file (@config_files) {
> -               $config_file =~ s/\R*//g;
> +               chomp $config_file;
>                 $page_offset = parse_kernel_config_file($config_file);
>                 if ($page_offset ne "") {
> -                       return $page_offset;
> +                       return hex($page_offset);
>                 }
>         }
>
>         printf STDERR "\nFailed to parse kernel config files\n";
>         printf STDERR "*** NOTE ***\n";
> -       printf STDERR "Falling back to PAGE_OFFSET = %s\n\n", $default_offset;
> +       printf STDERR "Falling back to PAGE_OFFSET = 0x%x\n\n", $default_offset;

Better use the '#' flag with the 'x' conversion specifier:
perl -e 'my $default_offset = hex("0xc0000000");printf "%#x\n", $default_offset'
0xc0000000

>
>         return $default_offset;
>  }
> --
> 2.14.3
>
> Thanks,
> Kaiwan.
>
>> thanks,
>> Tobin.

[kernel-hardening] Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Kaiwan N Billimoria]

[-- Attachment #1: Type: text/plain, Size: 3588 bytes --]

Sure, thanks Alexander..
Tobin, request you to pl make the change while merging, thanks..


Thanks & Regards,
Kaiwan.

On Mon, Dec 4, 2017 at 6:07 PM, Alexander Kapshuk <
alexander.kapshuk@gmail.com> wrote:

> On Mon, Dec 4, 2017 at 12:20 PM,  <kaiwan.billimoria@gmail.com> wrote:
> > On Mon, 2017-12-04 at 19:21 +1100, Tobin C. Harding wrote:
> >> On Mon, Dec 04, 2017 at 10:51:53AM +0530, Kaiwan N Billimoria wrote:
> >> > > ---
> >> > diff --git a/scripts/leaking_addresses.pl b/scripts/
> leaking_addresses.pl
> >> > index 9906dcf8b807..260b52e456f1 100755
> >> > --- a/scripts/leaking_addresses.pl
> >> > +++ b/scripts/leaking_addresses.pl
> >> > @@ -266,7 +266,7 @@ sub is_false_positive
> >> >  sub is_false_positive_ix86_32
> >> >  {
> >> >         my ($match) = @_;
> >> > -       state $page_offset = eval get_page_offset(); # only gets
> called once
> >> > +       state $page_offset = hex get_page_offset(); # only gets
> called once
> >>
> >> I don't think this is valid ;) I meant use hex() to convert the string
> >> to an int so it doesn't throw the warning (inside get_page_offset()).
> >
> > Yup, got it, thanks   :-p
> > Combined patch below:
> >
> >
> > ---
> >  scripts/leaking_addresses.pl | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> > index 9906dcf8b807..a595a2c66b12 100755
> > --- a/scripts/leaking_addresses.pl
> > +++ b/scripts/leaking_addresses.pl
> > @@ -266,8 +266,7 @@ sub is_false_positive
> >  sub is_false_positive_ix86_32
> >  {
> >         my ($match) = @_;
> > -       state $page_offset = eval get_page_offset(); # only gets called
> once
> > -
> > +       state $page_offset = get_page_offset(); # only gets called once
> >         if ($match =~ '\b(0x)?(f|F){8}\b') {
> >                 return 1;
> >         }
> > @@ -283,7 +282,7 @@ sub is_false_positive_ix86_32
> >  sub get_page_offset
> >  {
> >         my $page_offset;
> > -       my $default_offset = "0xc0000000";
> > +       my $default_offset = hex("0xc0000000");
> >         my @config_files;
> >
> >         # Allow --page-offset-32bit to override.
> > @@ -306,23 +305,23 @@ sub get_page_offset
> >                 } else {
> >                         $page_offset = parse_kernel_config_file($tmp_
> file);
> >                         if ($page_offset ne "") {
> > -                               return $page_offset;
> > +                               return hex($page_offset);
> >                         }
> >                 }
> >                 system("rm -f $tmp_file");
> >         }
> >
> >         foreach my $config_file (@config_files) {
> > -               $config_file =~ s/\R*//g;
> > +               chomp $config_file;
> >                 $page_offset = parse_kernel_config_file($config_file);
> >                 if ($page_offset ne "") {
> > -                       return $page_offset;
> > +                       return hex($page_offset);
> >                 }
> >         }
> >
> >         printf STDERR "\nFailed to parse kernel config files\n";
> >         printf STDERR "*** NOTE ***\n";
> > -       printf STDERR "Falling back to PAGE_OFFSET = %s\n\n",
> $default_offset;
> > +       printf STDERR "Falling back to PAGE_OFFSET = 0x%x\n\n",
> $default_offset;
>
> Better use the '#' flag with the 'x' conversion specifier:
> perl -e 'my $default_offset = hex("0xc0000000");printf "%#x\n",
> $default_offset'
> 0xc0000000
>
> >
> >         return $default_offset;
> >  }
> > --
> > 2.14.3
> >
> > Thanks,
> > Kaiwan.
> >
> >> thanks,
> >> Tobin.
>

[-- Attachment #2: Type: text/html, Size: 6145 bytes --]

Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Kaiwan N Billimoria]

Sure, thanks Alexander..
Tobin, request you to pl make the change while merging, thanks..


Thanks & Regards,
Kaiwan.


On Mon, Dec 4, 2017 at 6:07 PM, Alexander Kapshuk
<alexander.kapshuk@gmail.com> wrote:
> On Mon, Dec 4, 2017 at 12:20 PM,  <kaiwan.billimoria@gmail.com> wrote:
>> On Mon, 2017-12-04 at 19:21 +1100, Tobin C. Harding wrote:
>>> On Mon, Dec 04, 2017 at 10:51:53AM +0530, Kaiwan N Billimoria wrote:
>>> > > ---
>>> > diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
>>> > index 9906dcf8b807..260b52e456f1 100755
>>> > --- a/scripts/leaking_addresses.pl
>>> > +++ b/scripts/leaking_addresses.pl
>>> > @@ -266,7 +266,7 @@ sub is_false_positive
>>> >  sub is_false_positive_ix86_32
>>> >  {
>>> >         my ($match) = @_;
>>> > -       state $page_offset = eval get_page_offset(); # only gets called once
>>> > +       state $page_offset = hex get_page_offset(); # only gets called once
>>>
>>> I don't think this is valid ;) I meant use hex() to convert the string
>>> to an int so it doesn't throw the warning (inside get_page_offset()).
>>
>> Yup, got it, thanks   :-p
>> Combined patch below:
>>
>>
>> ---
>>  scripts/leaking_addresses.pl | 13 ++++++-------
>>  1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
>> index 9906dcf8b807..a595a2c66b12 100755
>> --- a/scripts/leaking_addresses.pl
>> +++ b/scripts/leaking_addresses.pl
>> @@ -266,8 +266,7 @@ sub is_false_positive
>>  sub is_false_positive_ix86_32
>>  {
>>         my ($match) = @_;
>> -       state $page_offset = eval get_page_offset(); # only gets called once
>> -
>> +       state $page_offset = get_page_offset(); # only gets called once
>>         if ($match =~ '\b(0x)?(f|F){8}\b') {
>>                 return 1;
>>         }
>> @@ -283,7 +282,7 @@ sub is_false_positive_ix86_32
>>  sub get_page_offset
>>  {
>>         my $page_offset;
>> -       my $default_offset = "0xc0000000";
>> +       my $default_offset = hex("0xc0000000");
>>         my @config_files;
>>
>>         # Allow --page-offset-32bit to override.
>> @@ -306,23 +305,23 @@ sub get_page_offset
>>                 } else {
>>                         $page_offset = parse_kernel_config_file($tmp_file);
>>                         if ($page_offset ne "") {
>> -                               return $page_offset;
>> +                               return hex($page_offset);
>>                         }
>>                 }
>>                 system("rm -f $tmp_file");
>>         }
>>
>>         foreach my $config_file (@config_files) {
>> -               $config_file =~ s/\R*//g;
>> +               chomp $config_file;
>>                 $page_offset = parse_kernel_config_file($config_file);
>>                 if ($page_offset ne "") {
>> -                       return $page_offset;
>> +                       return hex($page_offset);
>>                 }
>>         }
>>
>>         printf STDERR "\nFailed to parse kernel config files\n";
>>         printf STDERR "*** NOTE ***\n";
>> -       printf STDERR "Falling back to PAGE_OFFSET = %s\n\n", $default_offset;
>> +       printf STDERR "Falling back to PAGE_OFFSET = 0x%x\n\n", $default_offset;
>
> Better use the '#' flag with the 'x' conversion specifier:
> perl -e 'my $default_offset = hex("0xc0000000");printf "%#x\n", $default_offset'
> 0xc0000000
>
>>
>>         return $default_offset;
>>  }
>> --
>> 2.14.3
>>
>> Thanks,
>> Kaiwan.
>>
>>> thanks,
>>> Tobin.

[kernel-hardening] Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Kaiwan N Billimoria]

Sure, thanks Alexander..
Tobin, request you to pl make the change while merging, thanks..


Thanks & Regards,
Kaiwan.


On Mon, Dec 4, 2017 at 6:07 PM, Alexander Kapshuk
<alexander.kapshuk@gmail.com> wrote:
> On Mon, Dec 4, 2017 at 12:20 PM,  <kaiwan.billimoria@gmail.com> wrote:
>> On Mon, 2017-12-04 at 19:21 +1100, Tobin C. Harding wrote:
>>> On Mon, Dec 04, 2017 at 10:51:53AM +0530, Kaiwan N Billimoria wrote:
>>> > > ---
>>> > diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
>>> > index 9906dcf8b807..260b52e456f1 100755
>>> > --- a/scripts/leaking_addresses.pl
>>> > +++ b/scripts/leaking_addresses.pl
>>> > @@ -266,7 +266,7 @@ sub is_false_positive
>>> >  sub is_false_positive_ix86_32
>>> >  {
>>> >         my ($match) = @_;
>>> > -       state $page_offset = eval get_page_offset(); # only gets called once
>>> > +       state $page_offset = hex get_page_offset(); # only gets called once
>>>
>>> I don't think this is valid ;) I meant use hex() to convert the string
>>> to an int so it doesn't throw the warning (inside get_page_offset()).
>>
>> Yup, got it, thanks   :-p
>> Combined patch below:
>>
>>
>> ---
>>  scripts/leaking_addresses.pl | 13 ++++++-------
>>  1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
>> index 9906dcf8b807..a595a2c66b12 100755
>> --- a/scripts/leaking_addresses.pl
>> +++ b/scripts/leaking_addresses.pl
>> @@ -266,8 +266,7 @@ sub is_false_positive
>>  sub is_false_positive_ix86_32
>>  {
>>         my ($match) = @_;
>> -       state $page_offset = eval get_page_offset(); # only gets called once
>> -
>> +       state $page_offset = get_page_offset(); # only gets called once
>>         if ($match =~ '\b(0x)?(f|F){8}\b') {
>>                 return 1;
>>         }
>> @@ -283,7 +282,7 @@ sub is_false_positive_ix86_32
>>  sub get_page_offset
>>  {
>>         my $page_offset;
>> -       my $default_offset = "0xc0000000";
>> +       my $default_offset = hex("0xc0000000");
>>         my @config_files;
>>
>>         # Allow --page-offset-32bit to override.
>> @@ -306,23 +305,23 @@ sub get_page_offset
>>                 } else {
>>                         $page_offset = parse_kernel_config_file($tmp_file);
>>                         if ($page_offset ne "") {
>> -                               return $page_offset;
>> +                               return hex($page_offset);
>>                         }
>>                 }
>>                 system("rm -f $tmp_file");
>>         }
>>
>>         foreach my $config_file (@config_files) {
>> -               $config_file =~ s/\R*//g;
>> +               chomp $config_file;
>>                 $page_offset = parse_kernel_config_file($config_file);
>>                 if ($page_offset ne "") {
>> -                       return $page_offset;
>> +                       return hex($page_offset);
>>                 }
>>         }
>>
>>         printf STDERR "\nFailed to parse kernel config files\n";
>>         printf STDERR "*** NOTE ***\n";
>> -       printf STDERR "Falling back to PAGE_OFFSET = %s\n\n", $default_offset;
>> +       printf STDERR "Falling back to PAGE_OFFSET = 0x%x\n\n", $default_offset;
>
> Better use the '#' flag with the 'x' conversion specifier:
> perl -e 'my $default_offset = hex("0xc0000000");printf "%#x\n", $default_offset'
> 0xc0000000
>
>>
>>         return $default_offset;
>>  }
>> --
>> 2.14.3
>>
>> Thanks,
>> Kaiwan.
>>
>>> thanks,
>>> Tobin.

Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Tobin C. Harding]

On Mon, Dec 04, 2017 at 03:50:41PM +0530, kaiwan.billimoria@gmail.com wrote:
> On Mon, 2017-12-04 at 19:21 +1100, Tobin C. Harding wrote:
> > On Mon, Dec 04, 2017 at 10:51:53AM +0530, Kaiwan N Billimoria wrote:
> > > > ---
> > > diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> > > index 9906dcf8b807..260b52e456f1 100755
> > > --- a/scripts/leaking_addresses.pl
> > > +++ b/scripts/leaking_addresses.pl
> > > @@ -266,7 +266,7 @@ sub is_false_positive
> > >  sub is_false_positive_ix86_32
> > >  {
> > >         my ($match) = @_;
> > > -       state $page_offset = eval get_page_offset(); # only gets called once
> > > +       state $page_offset = hex get_page_offset(); # only gets called once
> > 
> > I don't think this is valid ;) I meant use hex() to convert the string
> > to an int so it doesn't throw the warning (inside get_page_offset()).
> 
> Yup, got it, thanks   :-p
> Combined patch below:
> 
> 
> ---
>  scripts/leaking_addresses.pl | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> index 9906dcf8b807..a595a2c66b12 100755
> --- a/scripts/leaking_addresses.pl
> +++ b/scripts/leaking_addresses.pl
> @@ -266,8 +266,7 @@ sub is_false_positive
>  sub is_false_positive_ix86_32
>  {
>  	my ($match) = @_;
> -	state $page_offset = eval get_page_offset(); # only gets called once
> -
> +	state $page_offset = get_page_offset(); # only gets called once
>  	if ($match =~ '\b(0x)?(f|F){8}\b') {
>  		return 1;
>  	}
> @@ -283,7 +282,7 @@ sub is_false_positive_ix86_32
>  sub get_page_offset
>  {
>  	my $page_offset;
> -	my $default_offset = "0xc0000000";
> +	my $default_offset = hex("0xc0000000");

This is not what I meant. When you put together the whole patch just do
what ever you have to do to make sure none of the functions emit
warnings. My point wast that there is no need to use 'eval' to suppress
warnings.

I'm getting a bit lost with all these small patches in each email. Can
you put together a patch with all the changes to date that you are
making including the suggestions by Alexanda so we can all see where we
are up to?

FYI, it should apply cleanly on top of the 'leaks' branch of my tree.

thanks,
Tobin

[kernel-hardening] Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Tobin C. Harding]

On Mon, Dec 04, 2017 at 03:50:41PM +0530, kaiwan.billimoria@gmail.com wrote:
> On Mon, 2017-12-04 at 19:21 +1100, Tobin C. Harding wrote:
> > On Mon, Dec 04, 2017 at 10:51:53AM +0530, Kaiwan N Billimoria wrote:
> > > > ---
> > > diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> > > index 9906dcf8b807..260b52e456f1 100755
> > > --- a/scripts/leaking_addresses.pl
> > > +++ b/scripts/leaking_addresses.pl
> > > @@ -266,7 +266,7 @@ sub is_false_positive
> > >  sub is_false_positive_ix86_32
> > >  {
> > >         my ($match) = @_;
> > > -       state $page_offset = eval get_page_offset(); # only gets called once
> > > +       state $page_offset = hex get_page_offset(); # only gets called once
> > 
> > I don't think this is valid ;) I meant use hex() to convert the string
> > to an int so it doesn't throw the warning (inside get_page_offset()).
> 
> Yup, got it, thanks   :-p
> Combined patch below:
> 
> 
> ---
>  scripts/leaking_addresses.pl | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> index 9906dcf8b807..a595a2c66b12 100755
> --- a/scripts/leaking_addresses.pl
> +++ b/scripts/leaking_addresses.pl
> @@ -266,8 +266,7 @@ sub is_false_positive
>  sub is_false_positive_ix86_32
>  {
>  	my ($match) = @_;
> -	state $page_offset = eval get_page_offset(); # only gets called once
> -
> +	state $page_offset = get_page_offset(); # only gets called once
>  	if ($match =~ '\b(0x)?(f|F){8}\b') {
>  		return 1;
>  	}
> @@ -283,7 +282,7 @@ sub is_false_positive_ix86_32
>  sub get_page_offset
>  {
>  	my $page_offset;
> -	my $default_offset = "0xc0000000";
> +	my $default_offset = hex("0xc0000000");

This is not what I meant. When you put together the whole patch just do
what ever you have to do to make sure none of the functions emit
warnings. My point wast that there is no need to use 'eval' to suppress
warnings.

I'm getting a bit lost with all these small patches in each email. Can
you put together a patch with all the changes to date that you are
making including the suggestions by Alexanda so we can all see where we
are up to?

FYI, it should apply cleanly on top of the 'leaks' branch of my tree.

thanks,
Tobin

Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Alexander Kapshuk]

On Wed, Nov 29, 2017 at 12:16 PM, Tobin C. Harding <me@tobin.cc> wrote:
> On Wed, Nov 29, 2017 at 09:59:59AM +0200, Alexander Kapshuk wrote:
>> On Tue, Nov 28, 2017 at 11:10 PM, Tobin C. Harding <me@tobin.cc> wrote:
>> > On Tue, Nov 28, 2017 at 03:16:24PM +0200, Alexander Kapshuk wrote:
>> >> On Tue, Nov 28, 2017 at 8:32 AM, Tobin C. Harding <me@tobin.cc> wrote:
>> >> > Currently, leaking_addresses.pl only supports scanning 64 bit
>> >> > architectures. This is due to how the regular expressions are formed. We
>> >> > can do better than this. 32 architectures can be supported if we take
>> >> > into consideration the kernel virtual address split.
>> >> >
>> >> > Add support for ix86 32 bit architectures.
>> >> >  - Add command line option for page offset.
>> >> >  - Add command line option for kernel configuration file.
>> >> >  - Parse kernel config file for page offset (CONFIG_PAGE_OFFSET).
>> >> >  - Use page offset when checking for kernel virtual addresses.
>> >> >
>> >> > Signed-off-by: Kaiwan N Billimoria <kaiwan.billimoria@gmail.com>
>> >> > Signed-off-by: Tobin C. Harding <me@tobin.cc>
>> >> > ---
>> >> >
>> >> > As discussed this is a patch based on Kaiwan's previous patch. This
>> >> > patch represents co development by Kaiwan and Tobin.
>> >> >
>> >> > Applies on top of commit 4fbd8d194f06 (Linux 4.15-rc1)
>> >> >
>> >> > thanks,
>> >> > Tobin.
>> >> >
>> >> >  scripts/leaking_addresses.pl | 168 +++++++++++++++++++++++++++++++++++++------
>> >> >  1 file changed, 148 insertions(+), 20 deletions(-)
>> >> >
>> >> > diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
>> >> > index bc5788000018..f03f2f140e0a 100755
>> >> > --- a/scripts/leaking_addresses.pl
>> >> > +++ b/scripts/leaking_addresses.pl
>> >> > @@ -1,9 +1,11 @@
>> >> >  #!/usr/bin/env perl
>> >> >  #
>> >> >  # (c) 2017 Tobin C. Harding <me@tobin.cc>
>> >> > +# (c) 2017 Kaiwan N Billimoria <kaiwan.billimoria@gmail.com> (ix86 stuff)
>> >> > +#
>> >> >  # Licensed under the terms of the GNU GPL License version 2
>> >> >  #
>> >> > -# leaking_addresses.pl: Scan 64 bit kernel for potential leaking addresses.
>> >> > +# leaking_addresses.pl: Scan the kernel for potential leaking addresses.
>> >> >  #  - Scans dmesg output.
>> >> >  #  - Walks directory tree and parses each file (for each directory in @DIRS).
>> >> >  #
>> >> > @@ -22,6 +24,7 @@ use Cwd 'abs_path';
>> >> >  use Term::ANSIColor qw(:constants);
>> >> >  use Getopt::Long qw(:config no_auto_abbrev);
>> >> >  use Config;
>> >> > +use feature 'state';
>> >> >
>> >> >  my $P = $0;
>> >> >  my $V = '0.01';
>> >> > @@ -35,18 +38,19 @@ my $TIMEOUT = 10;
>> >> >  # Script can only grep for kernel addresses on the following architectures. If
>> >> >  # your architecture is not listed here and has a grep'able kernel address please
>> >> >  # consider submitting a patch.
>> >> > -my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64');
>> >> > +my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64', 'i[3456]86');
>> >> >
>> >> >  # Command line options.
>> >> >  my $help = 0;
>> >> >  my $debug = 0;
>> >> > -my $raw = 0;
>> >> > -my $output_raw = "";   # Write raw results to file.
>> >> > -my $input_raw = "";    # Read raw results from file instead of scanning.
>> >> > -
>> >> > +my $raw = 0;                   # Show raw output.
>> >> > +my $output_raw = "";           # Write raw results to file.
>> >> > +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 $page_offset_32bit = 0;     # 32-bit: value of CONFIG_PAGE_OFFSET
>> >> > +my $kernel_config_file = "";   # Kernel configuration file.
>> >> >
>> >> >  # Do not parse these files (absolute path).
>> >> >  my @skip_parse_files_abs = ('/proc/kmsg',
>> >> > @@ -95,14 +99,16 @@ Version: $V
>> >> >
>> >> >  Options:
>> >> >
>> >> > -       -o, --output-raw=<file>  Save results for future processing.
>> >> > -       -i, --input-raw=<file>   Read results from file instead of scanning.
>> >> > -           --raw                Show raw results (default).
>> >> > -           --suppress-dmesg     Do not show dmesg results.
>> >> > -           --squash-by-path     Show one result per unique path.
>> >> > -           --squash-by-filename Show one result per unique filename.
>> >> > -       -d, --debug              Display debugging output.
>> >> > -       -h, --help, --version    Display this help and exit.
>> >> > +       -o, --output-raw=<file>         Save results for future processing.
>> >> > +       -i, --input-raw=<file>          Read results from file instead of scanning.
>> >> > +             --raw                       Show raw results (default).
>> >> > +             --suppress-dmesg            Do not show dmesg results.
>> >> > +             --squash-by-path            Show one result per unique path.
>> >> > +             --squash-by-filename        Show one result per unique filename.
>> >> > +           --page-offset-32bit=<hex>   PAGE_OFFSET value (for 32-bit kernels).
>> >> > +           --kernel-config-file=<file> Kernel configuration file (e.g /boot/config)
>> >> > +       -d, --debug                     Display debugging output.
>> >> > +       -h, --help, --version           Display this help and exit.
>> >> >
>> >> >  Examples:
>> >> >
>> >> > @@ -115,7 +121,10 @@ Examples:
>> >> >         # View summary report.
>> >> >         $0 --input-raw scan.out --squash-by-filename
>> >> >
>> >> > -Scans the running (64 bit) kernel for potential leaking addresses.
>> >> > +       # Scan kernel on a 32-bit system with a 2GB:2GB virtual address split.
>> >> > +       $0 --page-offset-32bit=0x80000000
>> >> > +
>> >> > +Scans the running kernel for potential leaking addresses.
>> >> >
>> >> >  EOM
>> >> >         exit($exitcode);
>> >> > @@ -131,6 +140,8 @@ GetOptions(
>> >> >         'squash-by-path'        => \$squash_by_path,
>> >> >         'squash-by-filename'    => \$squash_by_filename,
>> >> >         'raw'                   => \$raw,
>> >> > +       'page-offset-32bit=o'   => \$page_offset_32bit,
>> >> > +       'kernel-config-file=s'  => \$kernel_config_file,
>> >> >  ) or help(1);
>> >> >
>> >> >  help(0) if ($help);
>> >> > @@ -146,7 +157,9 @@ if (!$input_raw and ($squash_by_path or $squash_by_filename)) {
>> >> >         exit(128);
>> >> >  }
>> >> >
>> >> > -if (!is_supported_architecture()) {
>> >> > +if (is_supported_architecture()) {
>> >> > +       show_detected_architecture() if $debug;
>> >> > +} else {
>> >> >         printf "\nScript does not support your architecture, sorry.\n";
>> >> >         printf "\nCurrently we support: \n\n";
>> >> >         foreach(@SUPPORTED_ARCHITECTURES) {
>> >> > @@ -177,7 +190,7 @@ sub dprint
>> >> >
>> >> >  sub is_supported_architecture
>> >> >  {
>> >> > -       return (is_x86_64() or is_ppc64());
>> >> > +       return (is_x86_64() or is_ppc64() or is_ix86_32());
>> >> >  }
>> >> >
>> >> >  sub is_x86_64
>> >> > @@ -200,10 +213,40 @@ sub is_ppc64
>> >> >         return 0;
>> >> >  }
>> >> >
>> >> > +sub is_ix86_32
>> >> > +{
>> >> > +       my $archname = $Config{archname};
>> >> > +
>> >> > +       if ($archname =~ m/i[3456]86-linux/) {
>> >> > +               return 1;
>> >> > +       }
>> >> > +       return 0;
>> >> > +}
>> >> > +
>> >> > +sub show_detected_architecture
>> >> > +{
>> >> > +       printf "Detected architecture: ";
>> >> > +       if (is_ix86_32()) {
>> >> > +               printf "32 bit x86\n";
>> >> > +       } elsif (is_x86_64()) {
>> >> > +               printf "x86_64\n";
>> >> > +       } elsif (is_ppc64()) {
>> >> > +               printf "ppc64\n";
>> >> > +       } else {
>> >> > +               printf "failed to detect architecture\n"
>> >> > +       }
>> >> > +}
>> >> > +
>> >> >  sub is_false_positive
>> >> >  {
>> >> >         my ($match) = @_;
>> >> >
>> >> > +       if (is_ix86_32()) {
>> >> > +               return is_false_positive_ix86_32($match);
>> >> > +       }
>> >> > +
>> >> > +       # 64 bit architectures
>> >> > +
>> >> >         if ($match =~ '\b(0x)?(f|F){16}\b' or
>> >> >             $match =~ '\b(0x)?0{16}\b') {
>> >> >                 return 1;
>> >> > @@ -220,6 +263,87 @@ sub is_false_positive
>> >> >         return 0;
>> >> >  }
>> >> >
>> >> > +sub is_false_positive_ix86_32
>> >> > +{
>> >> > +       my ($match) = @_;
>> >> > +       state $page_offset = get_page_offset(); # only gets called once
>> >> > +
>> >> > +       if ($match =~ '\b(0x)?(f|F){8}\b') {
>> >> > +               return 1;
>> >> > +       }
>> >> > +
>> >> > +       my $addr32 = eval hex($match);
>> >> > +       if ($addr32 < $page_offset) {
>> >> > +               return 1;
>> >> > +       }
>> >> > +
>> >> > +       return 0;
>> >> > +}
>> >> > +
>> >>
>> >>
>> >>
>> >> > +sub get_page_offset
>> >> > +{
>> >> > +       my $page_offset;
>> >> > +       my $default_offset = "0xc0000000";
>> >> > +       my @config_files;
>> >> > +
>> >> > +       # Allow --page-offset-32bit to over ride.
>> >> > +       if ($page_offset_32bit != 0) {
>> >> > +               return $page_offset_32bit;
>> >> > +       }
>> >> > +
>> >> > +       # Allow --kernel-config-file to over ride.
>> >> > +       if ($kernel_config_file != "") {
>> >> > +               @config_files = ($kernel_config_file);
>> >> > +       } else {
>> >> > +               my $config_file = '/boot/config-' . `uname -r`;
>> >> > +               @config_files = ($config_file, '/boot/config');
>> >> > +       }
>> >> > +
>> >> > +       if (-R "/proc/config.gz") {
>> >> > +               my $tmp_file = "/tmp/tmpkconf";
>> >> > +               if (system("gunzip < /proc/config.gz > $tmp_file")) {
>> >> > +                       dprint " parse_kernel_config: system(gunzip...) failed\n";
>> >> > +               } else {
>> >> > +                       $page_offset = parse_kernel_config_file($tmp_file);
>> >> > +                       if ($page_offset ne "") {
>> >> > +                               return $page_offset;
>> >> > +                       }
>> >> > +               }
>> >> > +               system("rm -f $tmp_file");
>> >> > +       }
>> >> > +
>> >> > +       foreach my $config_file (@config_files) {
>> >> > +               $page_offset = parse_kernel_config($config_file);
>> >> > +               if ($page_offset ne "") {
>> >> > +                       return $page_offset;
>> >> > +               }
>> >> > +       }
>> >> > +
>> >> > +       printf STDERR "Failed to parse kernel config files\n";
>> >> > +       printf STDERR "Falling back to %s\n", $default_offset;
>> >> > +       return $default_offset;
>> >> > +}
>> >> > +
>> >> > +sub parse_kernel_config_file
>> >> > +{
>> >> > +       my ($file) = @_;
>> >> > +       my $config = 'CONFIG_PAGE_OFFSET';
>> >> > +       my $val = "";
>> >> > +
>> >> > +       open(my $fh, "<", $file) or return "";
>> >> > +       while (my $line = <$fh> ) {
>> >> > +               if ($line =~ /^$config/) {
>> >> > +                       my ($str, $val) = split /=/, $line;
>> >> > +                       chomp($val);
>> >> > +                       last;
>> >> > +               }
>> >> > +       }
>> >> > +
>> >> > +       close $fh;
>> >> > +       return $val;
>> >> > +}
>> >> > +
>> >> > +
>> >>
>> >> Get_page_offset attempts to build a list of config files, which are
>> >> then passed into the parsing function for further processing.
>> >> This splits up the code to do with the config files between
>> >> get_page_offset() and parse_kernel_config_file().
>> >> May I suggest putting the kernel config file processing code into the
>> >> parse_kernel_config_file() instead, and let the parsing function
>> >> handle the config files and either return the page_offset or an empty
>> >> string.
>> >>
>> >> See below for the proposed implementation.
>> >
>> > Nice, this is much better! Thanks.
>> >
>> >> Apologies for the absence of indentation.
>> >
>> > Re-posting with indentation, comments in line.
>> >
>> >> Disclaimer: I did not test-run the code being proposed.
>> >
>> > I also did not test my comments ;)
>> >
>> >> sub get_page_offset
>> >> {
>> >>       my $default_offset = "0xc0000000";
>> >>       my $page_offset;
>> >>
>> >>       # Allow --page-offset-32bit to over ride.
>> >>       if ($page_offset_32bit != 0) {
>> >>               return $page_offset_32bit;
>> >>       }
>> >>
>> >>       $page_offset = parse_kernel_config_file();
>> >>       if ($page_offset ne "") {
>> >>               return $page_offset
>> >>       }
>> >>
>> >>       printf STDERR "Failed to parse kernel config files\n";
>> >>       printf STDERR "Falling back to %s\n", $default_offset;
>> >>
>> >>       return $default_offset;
>> >> }
>> >>
>> >> sub parse_kernel_config_file
>> >> {
>> >>       my @config_files;
>> >>       my $config = 'CONFIG_PAGE_OFFSET';
>> >>
>> >>       # Allow --kernel-config-file to over ride.
>> >>       if ($kernel_config_file != "") {
>> >>               @config_files = ($kernel_config_file);
>> >>       } else {
>> >>               my $config_file = '/boot/config-' . `uname -r`;
>> >>               @config_files = ($config_file, '/boot/config');
>> >>       }
>> >>
>> >>       if (-R "/proc/config.gz") {
>> >
>> > perhaps
>> >                 my $tmpkconf = '/tmp/tmpkconf';
>>
>> my $tmpkconf is almost as long as /tmp/tmpkconf. The name of the tmp
>> file is self explanatory.
>> Using a variable instead of the filename in this particular context is
>> a matter of personal preference. If you prefer to use the variable
>> here, it's your call.
>
> I'm a stickler for no const strings or magic numbers but it's Kaiwan's
> patch, if he puts it in with const strings I'll apply it as is :)

Fair enough.

>
>> >
>> >>               if (system("gunzip < /proc/config.gz > /tmp/tmpkconf") == 0) {
>> >>                       push @config_files, "/tmp/tmpkconf";
>> >>               }
>> >>       }
>> >
>> > Won't there only ever be a single config file? So if /proc/config.gz is
>> > readable we could do
>>
>> The code above builds a list of config files.
>> Assigning to @config_files as shown below would wipe out the config
>> files appended to the list so far, would it not?
>> So $tmpkconf needs appending to the list.
>
> You are correct, since the beginning of this function that has been the
> algorithm. My observation is that if /proc/config.gz is present then we
> don't need to parse the other files so it is better to blow them away.
>
> I don't know enough about the whole Linux-sphere to know if this is
> correct. But it seems reasonable that even if there is more than one way
> to look at the running kernels config file they will all be the same,
> the system would be pretty broken if they were different.
>
> So once we have found a readable config file just parse it and be done
> with it, no need to loop over any others.

Makes sense.
Thanks.

>
> thanks,
> Tobin.

[kernel-hardening] Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses

[Alexander Kapshuk]

On Wed, Nov 29, 2017 at 12:16 PM, Tobin C. Harding <me@tobin.cc> wrote:
> On Wed, Nov 29, 2017 at 09:59:59AM +0200, Alexander Kapshuk wrote:
>> On Tue, Nov 28, 2017 at 11:10 PM, Tobin C. Harding <me@tobin.cc> wrote:
>> > On Tue, Nov 28, 2017 at 03:16:24PM +0200, Alexander Kapshuk wrote:
>> >> On Tue, Nov 28, 2017 at 8:32 AM, Tobin C. Harding <me@tobin.cc> wrote:
>> >> > Currently, leaking_addresses.pl only supports scanning 64 bit
>> >> > architectures. This is due to how the regular expressions are formed. We
>> >> > can do better than this. 32 architectures can be supported if we take
>> >> > into consideration the kernel virtual address split.
>> >> >
>> >> > Add support for ix86 32 bit architectures.
>> >> >  - Add command line option for page offset.
>> >> >  - Add command line option for kernel configuration file.
>> >> >  - Parse kernel config file for page offset (CONFIG_PAGE_OFFSET).
>> >> >  - Use page offset when checking for kernel virtual addresses.
>> >> >
>> >> > Signed-off-by: Kaiwan N Billimoria <kaiwan.billimoria@gmail.com>
>> >> > Signed-off-by: Tobin C. Harding <me@tobin.cc>
>> >> > ---
>> >> >
>> >> > As discussed this is a patch based on Kaiwan's previous patch. This
>> >> > patch represents co development by Kaiwan and Tobin.
>> >> >
>> >> > Applies on top of commit 4fbd8d194f06 (Linux 4.15-rc1)
>> >> >
>> >> > thanks,
>> >> > Tobin.
>> >> >
>> >> >  scripts/leaking_addresses.pl | 168 +++++++++++++++++++++++++++++++++++++------
>> >> >  1 file changed, 148 insertions(+), 20 deletions(-)
>> >> >
>> >> > diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
>> >> > index bc5788000018..f03f2f140e0a 100755
>> >> > --- a/scripts/leaking_addresses.pl
>> >> > +++ b/scripts/leaking_addresses.pl
>> >> > @@ -1,9 +1,11 @@
>> >> >  #!/usr/bin/env perl
>> >> >  #
>> >> >  # (c) 2017 Tobin C. Harding <me@tobin.cc>
>> >> > +# (c) 2017 Kaiwan N Billimoria <kaiwan.billimoria@gmail.com> (ix86 stuff)
>> >> > +#
>> >> >  # Licensed under the terms of the GNU GPL License version 2
>> >> >  #
>> >> > -# leaking_addresses.pl: Scan 64 bit kernel for potential leaking addresses.
>> >> > +# leaking_addresses.pl: Scan the kernel for potential leaking addresses.
>> >> >  #  - Scans dmesg output.
>> >> >  #  - Walks directory tree and parses each file (for each directory in @DIRS).
>> >> >  #
>> >> > @@ -22,6 +24,7 @@ use Cwd 'abs_path';
>> >> >  use Term::ANSIColor qw(:constants);
>> >> >  use Getopt::Long qw(:config no_auto_abbrev);
>> >> >  use Config;
>> >> > +use feature 'state';
>> >> >
>> >> >  my $P = $0;
>> >> >  my $V = '0.01';
>> >> > @@ -35,18 +38,19 @@ my $TIMEOUT = 10;
>> >> >  # Script can only grep for kernel addresses on the following architectures. If
>> >> >  # your architecture is not listed here and has a grep'able kernel address please
>> >> >  # consider submitting a patch.
>> >> > -my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64');
>> >> > +my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64', 'i[3456]86');
>> >> >
>> >> >  # Command line options.
>> >> >  my $help = 0;
>> >> >  my $debug = 0;
>> >> > -my $raw = 0;
>> >> > -my $output_raw = "";   # Write raw results to file.
>> >> > -my $input_raw = "";    # Read raw results from file instead of scanning.
>> >> > -
>> >> > +my $raw = 0;                   # Show raw output.
>> >> > +my $output_raw = "";           # Write raw results to file.
>> >> > +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 $page_offset_32bit = 0;     # 32-bit: value of CONFIG_PAGE_OFFSET
>> >> > +my $kernel_config_file = "";   # Kernel configuration file.
>> >> >
>> >> >  # Do not parse these files (absolute path).
>> >> >  my @skip_parse_files_abs = ('/proc/kmsg',
>> >> > @@ -95,14 +99,16 @@ Version: $V
>> >> >
>> >> >  Options:
>> >> >
>> >> > -       -o, --output-raw=<file>  Save results for future processing.
>> >> > -       -i, --input-raw=<file>   Read results from file instead of scanning.
>> >> > -           --raw                Show raw results (default).
>> >> > -           --suppress-dmesg     Do not show dmesg results.
>> >> > -           --squash-by-path     Show one result per unique path.
>> >> > -           --squash-by-filename Show one result per unique filename.
>> >> > -       -d, --debug              Display debugging output.
>> >> > -       -h, --help, --version    Display this help and exit.
>> >> > +       -o, --output-raw=<file>         Save results for future processing.
>> >> > +       -i, --input-raw=<file>          Read results from file instead of scanning.
>> >> > +             --raw                       Show raw results (default).
>> >> > +             --suppress-dmesg            Do not show dmesg results.
>> >> > +             --squash-by-path            Show one result per unique path.
>> >> > +             --squash-by-filename        Show one result per unique filename.
>> >> > +           --page-offset-32bit=<hex>   PAGE_OFFSET value (for 32-bit kernels).
>> >> > +           --kernel-config-file=<file> Kernel configuration file (e.g /boot/config)
>> >> > +       -d, --debug                     Display debugging output.
>> >> > +       -h, --help, --version           Display this help and exit.
>> >> >
>> >> >  Examples:
>> >> >
>> >> > @@ -115,7 +121,10 @@ Examples:
>> >> >         # View summary report.
>> >> >         $0 --input-raw scan.out --squash-by-filename
>> >> >
>> >> > -Scans the running (64 bit) kernel for potential leaking addresses.
>> >> > +       # Scan kernel on a 32-bit system with a 2GB:2GB virtual address split.
>> >> > +       $0 --page-offset-32bit=0x80000000
>> >> > +
>> >> > +Scans the running kernel for potential leaking addresses.
>> >> >
>> >> >  EOM
>> >> >         exit($exitcode);
>> >> > @@ -131,6 +140,8 @@ GetOptions(
>> >> >         'squash-by-path'        => \$squash_by_path,
>> >> >         'squash-by-filename'    => \$squash_by_filename,
>> >> >         'raw'                   => \$raw,
>> >> > +       'page-offset-32bit=o'   => \$page_offset_32bit,
>> >> > +       'kernel-config-file=s'  => \$kernel_config_file,
>> >> >  ) or help(1);
>> >> >
>> >> >  help(0) if ($help);
>> >> > @@ -146,7 +157,9 @@ if (!$input_raw and ($squash_by_path or $squash_by_filename)) {
>> >> >         exit(128);
>> >> >  }
>> >> >
>> >> > -if (!is_supported_architecture()) {
>> >> > +if (is_supported_architecture()) {
>> >> > +       show_detected_architecture() if $debug;
>> >> > +} else {
>> >> >         printf "\nScript does not support your architecture, sorry.\n";
>> >> >         printf "\nCurrently we support: \n\n";
>> >> >         foreach(@SUPPORTED_ARCHITECTURES) {
>> >> > @@ -177,7 +190,7 @@ sub dprint
>> >> >
>> >> >  sub is_supported_architecture
>> >> >  {
>> >> > -       return (is_x86_64() or is_ppc64());
>> >> > +       return (is_x86_64() or is_ppc64() or is_ix86_32());
>> >> >  }
>> >> >
>> >> >  sub is_x86_64
>> >> > @@ -200,10 +213,40 @@ sub is_ppc64
>> >> >         return 0;
>> >> >  }
>> >> >
>> >> > +sub is_ix86_32
>> >> > +{
>> >> > +       my $archname = $Config{archname};
>> >> > +
>> >> > +       if ($archname =~ m/i[3456]86-linux/) {
>> >> > +               return 1;
>> >> > +       }
>> >> > +       return 0;
>> >> > +}
>> >> > +
>> >> > +sub show_detected_architecture
>> >> > +{
>> >> > +       printf "Detected architecture: ";
>> >> > +       if (is_ix86_32()) {
>> >> > +               printf "32 bit x86\n";
>> >> > +       } elsif (is_x86_64()) {
>> >> > +               printf "x86_64\n";
>> >> > +       } elsif (is_ppc64()) {
>> >> > +               printf "ppc64\n";
>> >> > +       } else {
>> >> > +               printf "failed to detect architecture\n"
>> >> > +       }
>> >> > +}
>> >> > +
>> >> >  sub is_false_positive
>> >> >  {
>> >> >         my ($match) = @_;
>> >> >
>> >> > +       if (is_ix86_32()) {
>> >> > +               return is_false_positive_ix86_32($match);
>> >> > +       }
>> >> > +
>> >> > +       # 64 bit architectures
>> >> > +
>> >> >         if ($match =~ '\b(0x)?(f|F){16}\b' or
>> >> >             $match =~ '\b(0x)?0{16}\b') {
>> >> >                 return 1;
>> >> > @@ -220,6 +263,87 @@ sub is_false_positive
>> >> >         return 0;
>> >> >  }
>> >> >
>> >> > +sub is_false_positive_ix86_32
>> >> > +{
>> >> > +       my ($match) = @_;
>> >> > +       state $page_offset = get_page_offset(); # only gets called once
>> >> > +
>> >> > +       if ($match =~ '\b(0x)?(f|F){8}\b') {
>> >> > +               return 1;
>> >> > +       }
>> >> > +
>> >> > +       my $addr32 = eval hex($match);
>> >> > +       if ($addr32 < $page_offset) {
>> >> > +               return 1;
>> >> > +       }
>> >> > +
>> >> > +       return 0;
>> >> > +}
>> >> > +
>> >>
>> >>
>> >>
>> >> > +sub get_page_offset
>> >> > +{
>> >> > +       my $page_offset;
>> >> > +       my $default_offset = "0xc0000000";
>> >> > +       my @config_files;
>> >> > +
>> >> > +       # Allow --page-offset-32bit to over ride.
>> >> > +       if ($page_offset_32bit != 0) {
>> >> > +               return $page_offset_32bit;
>> >> > +       }
>> >> > +
>> >> > +       # Allow --kernel-config-file to over ride.
>> >> > +       if ($kernel_config_file != "") {
>> >> > +               @config_files = ($kernel_config_file);
>> >> > +       } else {
>> >> > +               my $config_file = '/boot/config-' . `uname -r`;
>> >> > +               @config_files = ($config_file, '/boot/config');
>> >> > +       }
>> >> > +
>> >> > +       if (-R "/proc/config.gz") {
>> >> > +               my $tmp_file = "/tmp/tmpkconf";
>> >> > +               if (system("gunzip < /proc/config.gz > $tmp_file")) {
>> >> > +                       dprint " parse_kernel_config: system(gunzip...) failed\n";
>> >> > +               } else {
>> >> > +                       $page_offset = parse_kernel_config_file($tmp_file);
>> >> > +                       if ($page_offset ne "") {
>> >> > +                               return $page_offset;
>> >> > +                       }
>> >> > +               }
>> >> > +               system("rm -f $tmp_file");
>> >> > +       }
>> >> > +
>> >> > +       foreach my $config_file (@config_files) {
>> >> > +               $page_offset = parse_kernel_config($config_file);
>> >> > +               if ($page_offset ne "") {
>> >> > +                       return $page_offset;
>> >> > +               }
>> >> > +       }
>> >> > +
>> >> > +       printf STDERR "Failed to parse kernel config files\n";
>> >> > +       printf STDERR "Falling back to %s\n", $default_offset;
>> >> > +       return $default_offset;
>> >> > +}
>> >> > +
>> >> > +sub parse_kernel_config_file
>> >> > +{
>> >> > +       my ($file) = @_;
>> >> > +       my $config = 'CONFIG_PAGE_OFFSET';
>> >> > +       my $val = "";
>> >> > +
>> >> > +       open(my $fh, "<", $file) or return "";
>> >> > +       while (my $line = <$fh> ) {
>> >> > +               if ($line =~ /^$config/) {
>> >> > +                       my ($str, $val) = split /=/, $line;
>> >> > +                       chomp($val);
>> >> > +                       last;
>> >> > +               }
>> >> > +       }
>> >> > +
>> >> > +       close $fh;
>> >> > +       return $val;
>> >> > +}
>> >> > +
>> >> > +
>> >>
>> >> Get_page_offset attempts to build a list of config files, which are
>> >> then passed into the parsing function for further processing.
>> >> This splits up the code to do with the config files between
>> >> get_page_offset() and parse_kernel_config_file().
>> >> May I suggest putting the kernel config file processing code into the
>> >> parse_kernel_config_file() instead, and let the parsing function
>> >> handle the config files and either return the page_offset or an empty
>> >> string.
>> >>
>> >> See below for the proposed implementation.
>> >
>> > Nice, this is much better! Thanks.
>> >
>> >> Apologies for the absence of indentation.
>> >
>> > Re-posting with indentation, comments in line.
>> >
>> >> Disclaimer: I did not test-run the code being proposed.
>> >
>> > I also did not test my comments ;)
>> >
>> >> sub get_page_offset
>> >> {
>> >>       my $default_offset = "0xc0000000";
>> >>       my $page_offset;
>> >>
>> >>       # Allow --page-offset-32bit to over ride.
>> >>       if ($page_offset_32bit != 0) {
>> >>               return $page_offset_32bit;
>> >>       }
>> >>
>> >>       $page_offset = parse_kernel_config_file();
>> >>       if ($page_offset ne "") {
>> >>               return $page_offset
>> >>       }
>> >>
>> >>       printf STDERR "Failed to parse kernel config files\n";
>> >>       printf STDERR "Falling back to %s\n", $default_offset;
>> >>
>> >>       return $default_offset;
>> >> }
>> >>
>> >> sub parse_kernel_config_file
>> >> {
>> >>       my @config_files;
>> >>       my $config = 'CONFIG_PAGE_OFFSET';
>> >>
>> >>       # Allow --kernel-config-file to over ride.
>> >>       if ($kernel_config_file != "") {
>> >>               @config_files = ($kernel_config_file);
>> >>       } else {
>> >>               my $config_file = '/boot/config-' . `uname -r`;
>> >>               @config_files = ($config_file, '/boot/config');
>> >>       }
>> >>
>> >>       if (-R "/proc/config.gz") {
>> >
>> > perhaps
>> >                 my $tmpkconf = '/tmp/tmpkconf';
>>
>> my $tmpkconf is almost as long as /tmp/tmpkconf. The name of the tmp
>> file is self explanatory.
>> Using a variable instead of the filename in this particular context is
>> a matter of personal preference. If you prefer to use the variable
>> here, it's your call.
>
> I'm a stickler for no const strings or magic numbers but it's Kaiwan's
> patch, if he puts it in with const strings I'll apply it as is :)

Fair enough.

>
>> >
>> >>               if (system("gunzip < /proc/config.gz > /tmp/tmpkconf") == 0) {
>> >>                       push @config_files, "/tmp/tmpkconf";
>> >>               }
>> >>       }
>> >
>> > Won't there only ever be a single config file? So if /proc/config.gz is
>> > readable we could do
>>
>> The code above builds a list of config files.
>> Assigning to @config_files as shown below would wipe out the config
>> files appended to the list so far, would it not?
>> So $tmpkconf needs appending to the list.
>
> You are correct, since the beginning of this function that has been the
> algorithm. My observation is that if /proc/config.gz is present then we
> don't need to parse the other files so it is better to blow them away.
>
> I don't know enough about the whole Linux-sphere to know if this is
> correct. But it seems reasonable that even if there is more than one way
> to look at the running kernels config file they will all be the same,
> the system would be pretty broken if they were different.
>
> So once we have found a readable config file just parse it and be done
> with it, no need to loop over any others.

Makes sense.
Thanks.

>
> thanks,
> Tobin.