Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752240AbdLFMyD (ORCPT ); Wed, 6 Dec 2017 07:54:03 -0500 Received: from mail-pf0-f193.google.com ([209.85.192.193]:37308 "EHLO mail-pf0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751709AbdLFMx6 (ORCPT ); Wed, 6 Dec 2017 07:53:58 -0500 X-Google-Smtp-Source: AGs4zMYSsIf06dVGNBzhL7roHDzI4tZrV/0GyhajR3sxMI8wd4GrGBhW3oycPy9ixWOHEWbzxKMWwQ== Message-ID: <1512564831.17323.41.camel@gmail.com> Subject: Re: [PATCH v3] scripts: leaking_addresses: add support for 32-bit kernel addresses From: kaiwan.billimoria@gmail.com To: "Tobin C. Harding" Cc: Alexander Kapshuk , Linux Kernel Mailing List , "kernel-hardening@lists.openwall.com" Date: Wed, 06 Dec 2017 18:23:51 +0530 In-Reply-To: <1512561090.17323.32.camel@gmail.com> References: <1512455204.17323.20.camel@gmail.com> <20171206040437.GB11835@eros> <1512561090.17323.32.camel@gmail.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.26.2 (3.26.2-1.fc27) Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 12621 Lines: 359 On Wed, 2017-12-06 at 17:21 +0530, kaiwan.billimoria@gmail.com wrote: > On Wed, 2017-12-06 at 15:04 +1100, Tobin C. Harding wrote: > > On Tue, Dec 05, 2017 at 11:56:44AM +0530, kaiwan.billimoria@gmail.com 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 (via the PAGE_OFFSET > > > kernel configurable). > > > > > > 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 > > > --- > > > > Right, this is starting to look awesome. > > Great! > > > > Note- This patch represents co development by Tobin and Kaiwan (plus suggestions from > > > Alexander Kapshuk). Applies on Tobin's tree 'leaks' branch on top of commit 680db1ef560f > > > (leaking_addresses: fix typo function not called). > > > > > > > > > scripts/leaking_addresses.pl | 169 +++++++++++++++++++++++++++++++++++++------ > > > 1 file changed, 148 insertions(+), 21 deletions(-) > > > > > > diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl > > > index 2d5336b3e1ea..6b015980d117 100755 > > > --- a/scripts/leaking_addresses.pl > > > +++ b/scripts/leaking_addresses.pl > > > @@ -24,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'; > > > @@ -37,18 +38,20 @@ 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 $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', > > > @@ -97,14 +100,16 @@ Version: $V > > > > > > Options: > > > > > > - -o, --output-raw= Save results for future processing. > > > - -i, --input-raw= 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= Save results for future processing. > > > + -i, --input-raw= 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= PAGE_OFFSET value (for 32-bit kernels). > > > + --kernel-config-file= Kernel configuration file (e.g /boot/config) > > > + -d, --debug Display debugging output. > > > + -h, --help, --version Display this help and exit. > > > > > > Examples: > > > > > > @@ -117,7 +122,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); > > > @@ -133,6 +141,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); > > > @@ -148,6 +158,7 @@ if (!$input_raw and ($squash_by_path or $squash_by_filename)) { > > > exit(128); > > > } > > > > > > +show_detected_architecture() if $debug; > > > if (!is_supported_architecture()) { > > > printf "\nScript does not support your architecture, sorry.\n"; > > > printf "\nCurrently we support: \n\n"; > > > @@ -179,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 > > > @@ -202,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; > > > @@ -222,6 +263,89 @@ sub is_false_positive > > > return 0; > > > } > > > > > > +sub is_false_positive_ix86_32 > > > +{ > > > + my ($match) = @_; > > > + state $page_offset = get_page_offset(); # only gets called once > > > > nit: new line here > > Will do > > > + 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 = hex("0xc0000000"); > > > + my @config_files; > > > > my $tmp_file = ""; > > > > See comments below for reasoning. > > > > > + # Allow --page-offset-32bit to override. > > > + if ($page_offset_32bit != 0) { > > > + return $page_offset_32bit; > > > + } > > > + > > > + # Allow --kernel-config-file to override. > > > + if ($kernel_config_file ne "") { > > > + @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"; > > > > $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 hex($page_offset); > > > + } > > > + } > > > + system("rm -f $tmp_file"); > > > + } > > > > The logic is a bit broken here. sub returns without rm'ing tmp file. > > Caught! :-) Thanks.. > > > I > > believe we discussed using > > > > @config_files = ($tmp_file); > > > > Then continuing to iterate @config_files as done. > > I thought, why not just do this: > > 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 "") { > system("rm -f $tmp_file"); > return hex($page_offset); > } > system("rm -f $tmp_file"); > } > } > > Also, this way, the '$tmp_file' var remains localized to the handling of > the /proc/config.gz file 'if' statement scope. > > > > + > > > + foreach my $config_file (@config_files) { > > > + chomp $config_file; > > > + $page_offset = parse_kernel_config_file($config_file); > > > + if ($page_offset ne "") { > > > + return hex($page_offset); > > > + } > > > + } > > > > We may need to use 'last' instead of returning so we can check for > > > > if ($tmp_file ne "") { > > system("rm -f $tmp_file"); > > } > > Not required, if we use the manner I propose above.. > > > > And one final (particularly trivial) nitpick > > > > Can you use the brief commit log with prefix > > > > leaking_addresses: > > > > please. That prefix is what is currently used. Using 'scripts:' makes it > > hard to fit a descriptive message within 52 characters. > > > > Understood, sorry for the current patch series not using this style. > > > I know we have changed it already, but perhaps it should mention x86 not > > just 32 bit (since it is not 32 bit generic). > > > > I realized while reviewing your code that there is no reason for this to > > be x86 specific, if we can get a config file with CONFIG_PAGE_OFFSET > > then we can scan the kernel like this irrespective of architecture. Perl > > doesn't manage to correctly identify the RaspberryPi I tried it on as 32 > > bit so we may not be able to do it how we currently are. > > Interesting.. > > > > I'm mentioning this because I don't want you to go to all this work and > > then remove a bunch of your code immediately while making it 32 bit > > generic. If you want to work on a generic version then I'm happy to work > > with you on it. > > Sure, lets try for a generic ver! Thanks for your help on this.. > As your experience woth the R Pi shows, we may have to just resort to building a > generic framework of sorts, letting folks "plugin" appropriate "truth values" > for their particular platform; this way, we support as much as we can for now > and, going forward, it's generic. > As of right now though, am unsure what this "generic framework" is.. > A way forward, perhaps: uname -m Looks promising. The output on a few test platforms: +--------------+------------------+ Platform/CPU | `uname -m` | +--------------+------------------+ x86_64 | x86_64 | +--------------+------------------+ x86-32 | i686 | +--------------+------------------+ ARM-32 (yocto | armv5tejl | qemuarm32) | | +--------------+------------------+ ARM64 (yocto | aarch64 | qemuarm64) | | +--------------+------------------+ MIPS64 (yocto | mips64 | qemumips64) | | +--------------+------------------+ ARM32 (qemu | armv7l | IMX6 Sabrelite)| | +--------------+------------------+ > > If you would prefer to just get this done and merged > > then we can do that too. > > > > As I've said before I'm new to the maintainer role so still learning how > > best to approach things. Thanks for your patience. > > IMO, you're doing just great (me, am not so sure :) ). > > Thanks, > Kaiwan. > > Hope this helps, > > Tobin.