| Message ID | 1511850724-2381-1-git-send-email-me@tobin.cc (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; }
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.
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.
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.
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.
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.
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.
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.
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) {