From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Google-Smtp-Source: ACJfBovKoVoY0rAP+LWQ9GEAHpwq369ynh0Foa6g+fCaniZrG2TEVDK2smNyAgHNbTh8IADzbKc6 ARC-Seal: i=1; a=rsa-sha256; t=1515840960; cv=none; d=google.com; s=arc-20160816; b=aB2I0Fe9VCIghpM9l7Xwia6b0B+GD82nzN2+0EJA6GH+Z4aqnAHv2yCgHU4ebbqSZa jHvsiuGc4kfEv6uwIj9FDrgeddeLzFQkDegn7Md3g8D5ia+2P1NVBfMqgY87hPWmn4i0 aH021PvHR+G9JMgJbfrDOkew2Lwzd7SW62GOtcUuETvnW/orRxolKLhXIlGtNsHfdhgI 8RB69acGFx0GoixaM9O7o6YznuNuRbOSj6Togok+d4K7KAyZ9Yuqv6x7r+g6DQf23o4c nDDdL1D4cC+nj3R6+3Ekp/4NlpMJiv9DWGQRrroREOTLqU1+juYxZfk6zjxC8KKl3HvM AgfA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=subject:cc:to:message-id:date:from:in-reply-to:references :mime-version:dkim-signature:delivered-to:list-id:list-subscribe :list-unsubscribe:list-help:list-post:precedence:mailing-list :arc-authentication-results; bh=DEhdBq9+f5urniwVec8wXn9TcD9qXIzL7U9LB2wxeTc=; b=HuYzWFSU3cOhYiicnRVg9CzrnRaaBb82GDczfIBpkHBHAvkd5Q69+qioj0EYU9NfWf fdv40zQPO2AJKAUUWV6SGwx++dD3OtpLLDc42CeLzgR+qkizEawPC998M/Ls3EshujeP yTtTDqLKYbZrh5r/dNvAWN0MGWfGU7slMJ5dxT0Xzku8u9xRl/TJ/dTCTaAH3mRx9vEP epqkeP6R12SSJmrjNqc9orO8aWmMhfKc9oJgVa6uR1KlCbVS7w4brx0W9+CUR04UP/Yi L41JZ6I8CbX783qsWZo83OAWNxd6vR9BO6kl4qF/NhGtHFsYlCVa8XLOXnFqLEp4gKRI BYtA== ARC-Authentication-Results: i=1; mx.google.com; dkim=fail header.i=@gmail.com header.s=20161025 header.b=d8AR29Ig; spf=pass (google.com: domain of kernel-hardening-return-11246-gregkh=linuxfoundation.org@lists.openwall.com designates 195.42.179.200 as permitted sender) smtp.mailfrom=kernel-hardening-return-11246-gregkh=linuxfoundation.org@lists.openwall.com; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=gmail.com Authentication-Results: mx.google.com; dkim=fail header.i=@gmail.com header.s=20161025 header.b=d8AR29Ig; spf=pass (google.com: domain of kernel-hardening-return-11246-gregkh=linuxfoundation.org@lists.openwall.com designates 195.42.179.200 as permitted sender) smtp.mailfrom=kernel-hardening-return-11246-gregkh=linuxfoundation.org@lists.openwall.com; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=gmail.com Mailing-List: contact kernel-hardening-help@lists.openwall.com; run by ezmlm List-Post: List-Help: List-Unsubscribe: List-Subscribe: MIME-Version: 1.0 References: <1515068485.3034.5.camel@gmail.com> <20180105221228.GB5445@eros> In-Reply-To: <20180105221228.GB5445@eros> From: Kaiwan N Billimoria Date: Sat, 13 Jan 2018 10:55:26 +0000 Message-ID: To: "Tobin C. Harding" Cc: linux-kernel@vger.kernel.org, kernel-hardening@lists.openwall.com Content-Type: multipart/alternative; boundary="001a113de47e92119a0562a6388c" Subject: [kernel-hardening] Re: [PATCH v5] leaking_addresses: add generic 32-bit support X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: =?utf-8?q?1588664481466659022?= X-GMAIL-MSGID: =?utf-8?q?1589474451170664237?= X-Mailing-List: linux-kernel@vger.kernel.org List-ID: --001a113de47e92119a0562a6388c Content-Type: text/plain; charset="UTF-8" Hi Tobin, Thanks very much for your detailed review. Just wanted to say that am up to my neck in work (an exceptionally busy time), hence will take a while to work on this - around another 3 weeks perhaps. I'd like to continue, but if you feel it's too long please move ahead. On Sat, 6 Jan 2018, 3:42 am Tobin C. Harding, wrote: > Hi Kaiwan, > > Thanks for the patch > > On Thu, Jan 04, 2018 at 05:51:25PM +0530, kaiwan.billimoria@gmail.com > wrote: > > The script now attempts to detect the architecture it's running upon; as > of now, > > we explicitly support x86_64, PPC64, ARM64, MIPS64 and x86_32. > > This is incorrect. We do not currently support ARM64, MIPS64 and x86_32. > > > If it's one of them, we proceed "normally". If we fail to detect the > arch, > > we fallback to 64-bit scanning, _unless_ the user has passed either of > these > > option switches: "--opt-32bit" and/or "--page-offset-32bit=". > > > > If so, we switch to scanning for leaked addresses based on the value of > > PAGE_OFFSET (via an auto-detected or fallback mechanism). > > > > Currently, code (or "rules") to detect special cases for x86_64, PPC64 > and Aarch64 > > (in the get_address_re sub) is present. Also, we now have also builtin > "stubs", > > for lack of a better term, where additional rules for other 64-bit > arch's can be > > plugged into the code, in future, as applicable. > > > > This patch adds support for ix86 and generic 32 bit architectures. > > - Add command line option for generic 32-bit checking. > > - 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. > > This is all pretty confusing. It is hard to discern what is the > description of the problem being fixed and what is the description of > the new behaviour implemented by the patch. Also this patch does not add > kernel configuration file option. > > > The script has been lightly tested on the following systems: > > x86_64 > > x86_32 (on a i686 running Debian 7 to be precise) > > ARM32 (on a Yocto-based qemuarm32 ARM Versatile platform (ARM926EJ-S > cpu)) > > > > In the first two cases, one just has to run the script - no parameters > required. > > In the ARM-32 case, it will, by design, fail to detect the architecture; > > (re)running it with the '--32-bit' and/or the > '--page-offset-32bit=' > > This does not match the code. In the code you have defined the option as > '--opt-32bit'? '--32-bit' is better ;) > > > option switch(es) has the desired effect: it now detects 32-bit leaking > kernel > > addresses, if any. > > > > Request more testing on the above and other platforms. > > > > > > Signed-off-by: Kaiwan N Billimoria > > > > --- > > scripts/leaking_addresses.pl | 190 > +++++++++++++++++++++++++++++++++++-------- > > 1 file changed, 156 insertions(+), 34 deletions(-) > > > > diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl > > index a29e13e577a7..b0807b3a3c7c 100755 > > --- a/scripts/leaking_addresses.pl > > +++ b/scripts/leaking_addresses.pl > > @@ -1,10 +1,10 @@ > > #!/usr/bin/env perl > > # > > # (c) 2017 Tobin C. Harding > > - > > Please make only the minimum number of changes required. > > > +# (c) 2017 Kaiwan N Billimoria > > # 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 kernel for potential leaking addresses. > > # - Scans dmesg output. > > # - Walks directory tree and parses each file (for each directory in > @DIRS). > > # > > @@ -32,11 +32,6 @@ my @DIRS = ('/proc', '/sys'); > > # Timer for parsing each file, in seconds. > > 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'); > > - > > # Command line options. > > my $help = 0; > > my $debug = 0; > > @@ -48,7 +43,9 @@ 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 $kernel_config_file = ""; # Kernel configuration file. > > No need for this to be removed and re-added (if you line up the comments). > > > +my $opt_32_bit = 0; # Detect (only) 32-bit kernel leaking > addresses. > > As previously discussed, please be consistent in naming: $opt_32bit > > > +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', > > @@ -104,10 +101,12 @@ Options: > > --squash-by-path Show one result per unique path. > > --squash-by-filename Show one result per unique > filename. > > --kernel-config-file= Kernel configuration file (e.g > /boot/config) > > + --opt-32bit Detect (only) 32-bit kernel > leaking addresses. > > --32-bit Scan 32-bit kernel. > > > + --page-offset-32bit= PAGE_OFFSET value (for 32-bit > kernels). > > -d, --debug Display debugging output. > > - -h, --help, --versionq Display this help and exit. > > + -h, --help, --version Display this help and exit. > > > > -Scans the running (64 bit) kernel for potential leaking addresses. > > +Scans the running kernel for potential leaking addresses. > > > > EOM > > exit($exitcode); > > @@ -123,7 +122,9 @@ GetOptions( > > 'squash-by-path' => \$squash_by_path, > > 'squash-by-filename' => \$squash_by_filename, > > 'raw' => \$raw, > > - 'kernel-config-file=s' => \$kernel_config_file, > > + 'opt-32bit' => \$opt_32_bit, > > + 'page-offset-32bit=o' => \$page_offset_32bit, > > + 'kernel-config-file=s' => \$kernel_config_file, > > ) or help(1); > > > > help(0) if ($help); > > @@ -139,16 +140,15 @@ if (!$input_raw and ($squash_by_path or > $squash_by_filename)) { > > exit(128); > > } > > > > -if (!is_supported_architecture()) { > > - printf "\nScript does not support your architecture, sorry.\n"; > > - printf "\nCurrently we support: \n\n"; > > - foreach(@SUPPORTED_ARCHITECTURES) { > > - printf "\t%s\n", $_; > > - } > > +show_detected_architecture() if $debug; > > > > - my $archname = $Config{archname}; > > - printf "\n\$ perl -MConfig -e \'print > \"\$Config{archname}\\n\"\'\n"; > > - printf "%s\n", $archname; > > +if (!is_known_architecture()) { > > + printf STDERR "\nFATAL: Script does not recognize your > architecture\n"; > > + > > + my $arch = `uname -m`; > > + chomp $arch; > > + printf "\n\$ uname -m\n"; > > + printf "%s\n", $arch; > > > > exit(129); > > } > > I am going to add a patch to use `uname -m` instead of `perl -MConfig > ...` on powerpc. Please rebase your next version. > > > @@ -168,21 +168,45 @@ sub dprint > > printf(STDERR @_) if $debug; > > } > > > > -sub is_supported_architecture > > +sub is_known_architecture > > Please don't change this. > > > { > > - return (is_x86_64() or is_ppc64()); > > + return (is_64bit() or is_32bit()); > > } > > > > -sub is_x86_64 > > +sub is_32bit > > { > > - my $archname = $Config{archname}; > > + # 32-bit actual case > > + if (is_ix86_32()) { > > + return 1; > > + } > > > > - if ($archname =~ m/x86_64/) { > > + # 32-bit "forced" case (for any arch other than IA-32) > > + if ($opt_32_bit or $page_offset_32bit) { > > return 1; > > } > > return 0; > > } > > Perhaps > > > sub is_32bit > { > # Allow --32-bit and/or --page-offset-32bit to override. > if ($opt_32_bit or $page_offset_32bit) { > return 1; > } > > if (is_ix86_32()) { > return 1; > } > > return 0; > } > > > > +sub is_64bit > > +{ > > + return (is_x86_64() or is_ppc64() or is_arm64() or is_mips64()); > > +} > > + > > +sub is_x86_64 > > +{ > > + is_arch("x86_64"); > > +} > > + > > +sub is_arm64 > > +{ > > + is_arch("aarch64"); > > +} > > + > > +sub is_mips64 > > +{ > > + is_arch("mips64"); > > +} > > + > > We cannot add these last two. There has been no talk (to my knowledge) of > the > address format for mips64 or aarch64. If you have tested for these please > add as a separate patch. > > > sub is_ppc64 > > { > > my $archname = $Config{archname}; > > @@ -193,6 +217,47 @@ sub is_ppc64 > > return 0; > > } > > > > +sub is_ix86_32 > > +{ > > + my $arch = `uname -m`; > > + > > + chomp $arch; > > + if ($arch =~ m/i[3456]86/) { > > + return 1; > > + } > > + return 0; > > +} > > + > > +sub is_arch > > +{ > > + my ($desc) = @_; > > + my $arch = `uname -m`; > > + > > + chomp $arch; > > + if ($arch eq $desc) { > > + return 1; > > + } > > + return 0; > > +} > > + > > +sub show_detected_architecture > > +{ > > + printf "Detected architecture: "; > > + if (is_32bit()) { > > + printf "32 bit\n"; > > is_32bit includes the command line options so this is not _totally_ > correct. Prefer > > if (is_x64_32()) { > ... > > > + } elsif (is_x86_64()) { > > + printf "x86_64\n"; > > + } elsif (is_ppc64()) { > > + printf "PPC64\n"; > > + } elsif (is_arm64()) { > > + printf "ARM64\n"; > > + } elsif (is_mips64()) { > > + printf "MIPS64\n"; > > + } else { > > + printf "failed to detect architecture\n" > > + } > > +} > > + > > # gets config option value from kernel config file > > sub get_kernel_config_option > > { > > @@ -212,7 +277,6 @@ sub get_kernel_config_option > > } else { > > @config_files = ($tmp_file); > > } > > - > > Please don't do white space changes in the middle or another patch. > > > } else { > > my $file = '/boot/config-' . `uname -r`; > > chomp $file; > > @@ -220,7 +284,6 @@ sub get_kernel_config_option > > } > > > > foreach my $file (@config_files) { > > - dprint("parsing config file: %s\n", $file); > > This should be a separate patch. 'One thing per patch' is the kernel mantra > > > $value = option_from_file($option, $file); > > if ($value ne "") { > > last; > > @@ -258,6 +321,14 @@ sub is_false_positive > > { > > my ($match) = @_; > > > > + # 32 bit architectures, actual or forced > > + > > No need for this comment. > > > + if (is_32bit()) { > > + return is_false_positive_32bit($match); > > + } > > + > > + # 64 bit architectures > > + > > Or this one. > > > if ($match =~ '\b(0x)?(f|F){16}\b' or > > $match =~ '\b(0x)?0{16}\b') { > > return 1; > > @@ -281,6 +352,58 @@ sub is_in_vsyscall_memory_region > > return ($hex >= $region_min and $hex <= $region_max); > > } > > > > +sub is_false_positive_32bit > > +{ > > + my ($match) = @_; > > + state $page_offset = get_page_offset(); # only gets called once > > + > > + if ($match =~ '\b(0x)?(f|F){8}\b') { > > + return 1; > > + } > > + > > + my $addr32 = hex($match); > > + if ($addr32 < $page_offset) { > > + return 1; > > + } > > + > > + return 0; > > +} > > The return value of this sub routine is confusing. Either it should be > commented as returning a hex value or it should return a string. I > prefer a string since the rest of the subs in the file return strings, > less cognitive load to keep everything the same. Unless there is some > clear advantage to returning a hex value (which I cannot see). > > > +sub get_page_offset > > +{ > > + my $page_offset; > > + my $default_offset = hex("0xc0000000"); > > + my @config_files; > > + > > + # Allow --page-offset-32bit to override. > > + if ($page_offset_32bit != 0) { > > + return $page_offset_32bit; > > + } > > + > > + $page_offset = get_kernel_config_option('CONFIG_PAGE_OFFSET'); > > + return $default_offset; > > +} > > This is wrong. > > > + > > +sub parse_kernel_config_file > > +{ > > + my ($file) = @_; > > + my $config = 'CONFIG_PAGE_OFFSET'; > > + my $str = ""; > > + my $val = ""; > > + > > + open(my $fh, "<", $file) or return ""; > > + while (my $line = <$fh> ) { > > + if ($line =~ /^$config/) { > > + ($str, $val) = split /=/, $line; > > + chomp($val); > > + last; > > + } > > + } > > + > > + close $fh; > > + return $val; > > +} > > + > > When do you call this subroutine? > > > # True if argument potentially contains a kernel address. > > sub may_leak_address > > { > > @@ -300,8 +423,6 @@ sub may_leak_address > > } > > > > $address_re = get_address_re(); > > - dprint("Kernel address regular expression: %s\n", $address_re); > > - > > Please see comments above on 'one thing per patch'. > > > while (/($address_re)/g) { > > if (!is_false_positive($1)) { > > return 1; > > @@ -313,16 +434,17 @@ sub may_leak_address > > > > sub get_address_re > > { > > - my $re; > > + my $re = ""; > > > > if (is_x86_64()) { > > $re = get_x86_64_re(); > > } elsif (is_ppc64()) { > > $re = '\b(0x)?[89abcdef]00[[:xdigit:]]{13}\b'; > > - } > > - > > - if ($re eq "") { > > - print STDERR "$0: failed to build kernel address regular > expression\n"; > > + ### > > + # Any special cases for other arch's go below this line > > + ### > > + } else { # nothing? then we assume it's a generic 32-bit > > + $re = '\b(0x)?[[:xdigit:]]{8}\b'; > > } > > > > return $re; > > This is messy. We definitely want to warn if we cannot fully determine > the correct regex to use. Also, if you would like to default to 32-bit > you will need to back it up with some reasoning. Currently there is no > default if the correct regex cannot be ascertained. At this stage I feel > this is the correct behaviour, I'm open to discussion though. > > > -- > > 2.14.3 > > > thanks, > Tobin. > --001a113de47e92119a0562a6388c Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable

Hi Tobin,

Thanks very much for your detailed review.
Just wanted to say that am up to my neck in work (an exceptionally busy tim= e), hence will take a while to work on this - around another 3 weeks perhap= s.
I'd like to continue, but if you feel it's too long please move ahe= ad.


On Sat, 6 Jan 2018, 3:42 am= Tobin C. Harding, <me@tobin.cc> wrote:
Hi Kaiwan,

Thanks for the patch

On Thu, Jan 04, 2018 at 05:51:25PM +0530, kaiwan.billimoria@gmail.com wrote:
> The script now attempts to detect the architecture it's running up= on; as of now,
> we explicitly support x86_64, PPC64, ARM64, MIPS64 and x86_32.

This is incorrect. We do not currently support ARM64, MIPS64 and x86_32.
> If it's one of them, we proceed "normally". If we fail t= o detect the arch,
> we fallback to 64-bit scanning, _unless_ the user has passed either of= these
> option switches: "--opt-32bit" and/or "--page-offset-32= bit=3D<val>".
>
> If so, we switch to scanning for leaked addresses based on the value o= f
> PAGE_OFFSET (via an auto-detected or fallback mechanism).
>
> Currently, code (or "rules") to detect special cases for x86= _64, PPC64 and Aarch64
> (in the get_address_re sub) is present. Also, we now have also builtin= "stubs",
> for lack of a better term, where additional rules for other 64-bit arc= h's can be
> plugged into the code, in future, as applicable.
>
> This patch adds support for ix86 and generic 32 bit architectures.
>=C2=A0 - Add command line option for generic 32-bit checking.
>=C2=A0 - Add command line option for page offset.
>=C2=A0 - Add command line option for kernel configuration file.
>=C2=A0 - Parse kernel config file for page offset (CONFIG_PAGE_OFFSET).=
>=C2=A0 - Use page offset when checking for kernel virtual addresses.
This is all pretty confusing. It is hard to discern what is the
description of the problem being fixed and what is the description of
the new behaviour implemented by the patch. Also this patch does not add kernel configuration file option.

> The script has been lightly tested on the following systems:
>=C2=A0 x86_64
>=C2=A0 x86_32 (on a i686 running Debian 7 to be precise)
>=C2=A0 ARM32=C2=A0 (on a Yocto-based qemuarm32 ARM Versatile platform (= ARM926EJ-S cpu))
>
> In the first two cases, one just has to run the script - no parameters= required.
> In the ARM-32 case, it will, by design, fail to detect the architectur= e;
> (re)running it with the '--32-bit' and/or the '--page-offs= et-32bit=3D<hexval>'

This does not match the code. In the code you have defined the option as '--opt-32bit'? '--32-bit' is better ;)

> option switch(es) has the desired effect: it now detects 32-bit leakin= g kernel
> addresses, if any.
>
> Request more testing on the above and other platforms.
>
>
> Signed-off-by: Kaiwan N Billimoria <kaiwan.billimoria@gmail.com>
>
> ---
>=C2=A0 scripts/leaking_addresses.pl | 190 +++++++++++++++++++++++= ++++++++++++--------
>=C2=A0 1 file changed, 156 insertions(+), 34 deletions(-)
>
> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_ad= dresses.pl
> index a29e13e577a7..b0807b3a3c7c 100755
> --- a/scripts/leaking_addresses.pl
> +++ b/scripts/leaking_addresses.pl
> @@ -1,10 +1,10 @@
>=C2=A0 #!/usr/bin/env perl
>=C2=A0 #
>=C2=A0 # (c) 2017 Tobin C. Harding <me@tobin.cc>
> -

Please make only the minimum number of changes required.

> +# (c) 2017 Kaiwan N Billimoria <kaiwan.billimoria@gmail.com>
>=C2=A0 # Licensed under the terms of the GNU GPL License version 2
>=C2=A0 #
> -# leaking_addresses.pl: Scan 64 bit kernel for potential leak= ing addresses.
> +# leaking_addresses.pl: Scan kernel for potential leaking add= resses.
>=C2=A0 #=C2=A0 - Scans dmesg output.
>=C2=A0 #=C2=A0 - Walks directory tree and parses each file (for each di= rectory in @DIRS).
>=C2=A0 #
> @@ -32,11 +32,6 @@ my @DIRS =3D ('/proc', '/sys');
>=C2=A0 # Timer for parsing each file, in seconds.
>=C2=A0 my $TIMEOUT =3D 10;
>
> -# Script can only grep for kernel addresses on the following architec= tures. If
> -# your architecture is not listed here and has a grep'able kernel= address please
> -# consider submitting a patch.
> -my @SUPPORTED_ARCHITECTURES =3D ('x86_64', 'ppc64');<= br> > -
>=C2=A0 # Command line options.
>=C2=A0 my $help =3D 0;
>=C2=A0 my $debug =3D 0;
> @@ -48,7 +43,9 @@ my $suppress_dmesg =3D 0;=C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0# Don't show dmesg in output.
>=C2=A0 my $squash_by_path =3D 0;=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 # Summary report grouped by absolute path.
>=C2=A0 my $squash_by_filename =3D 0;=C2=A0 # Summary report grouped by = filename.
>
> -my $kernel_config_file =3D ""; # Kernel configuration file.=

No need for this to be removed and re-added (if you line up the comments).<= br>
> +my $opt_32_bit =3D 0;=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # Dete= ct (only) 32-bit kernel leaking addresses.

As previously discussed, please be consistent in naming: $opt_32bit

> +my $page_offset_32bit =3D 0;=C2=A0 =C2=A0 =C2=A0# 32-bit: value of CO= NFIG_PAGE_OFFSET.
> +my $kernel_config_file =3D "";=C2=A0 =C2=A0# Kernel configu= ration file.
>
>=C2=A0 # Do not parse these files (absolute path).
>=C2=A0 my @skip_parse_files_abs =3D ('/proc/kmsg',
> @@ -104,10 +101,12 @@ Options:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0--squash-by-path=C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 Show one result per unique path.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0--squash-by-filename=C2= =A0 =C2=A0 =C2=A0 Show one result per unique filename.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0--kernel-config-file=3D<file>=C2=A0 = =C2=A0 =C2=A0Kernel configuration file (e.g /boot/config)
> +=C2=A0 =C2=A0 =C2=A0--opt-32bit=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Detect (only) 32-bit kernel leaking a= ddresses.

=C2=A0 =C2=A0 =C2=A0 =C2=A0 --32-bit=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Scan 32-bit kernel.

> +=C2=A0 =C2=A0 =C2=A0--page-offset-32bit=3D<hex>=C2=A0 =C2=A0 = =C2=A0 =C2=A0PAGE_OFFSET value (for 32-bit kernels).
>=C2=A0 =C2=A0 =C2=A0 =C2=A0-d, --debug=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Display debugging output.
> -=C2=A0 =C2=A0 =C2=A0-h, --help, --versionq=C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 Display this help and exit.
> +=C2=A0 =C2=A0 =C2=A0-h, --help, --version=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0Display this help and exit.
>
> -Scans the running (64 bit) kernel for potential leaking addresses. > +Scans the running kernel for potential leaking addresses.
>
>=C2=A0 EOM
>=C2=A0 =C2=A0 =C2=A0 =C2=A0exit($exitcode);
> @@ -123,7 +122,9 @@ GetOptions(
>=C2=A0 =C2=A0 =C2=A0 =C2=A0'squash-by-path'=C2=A0 =C2=A0 =C2=A0= =C2=A0 =3D> \$squash_by_path,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0'squash-by-filename'=C2=A0 =C2=A0 = =3D> \$squash_by_filename,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0'raw'=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=3D> \$raw,
> -=C2=A0 =C2=A0 =C2=A0'kernel-config-file=3Ds'=C2=A0 =3D> \$= kernel_config_file,
> +=C2=A0 =C2=A0 =C2=A0'opt-32bit'=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0=3D> \$opt_32_bit,
> +=C2=A0 =C2=A0 =C2=A0'page-offset-32bit=3Do'=C2=A0 =C2=A0=3D&g= t; \$page_offset_32bit,
> +=C2=A0 =C2=A0 =C2=A0'kernel-config-file=3Ds'=C2=A0 =3D> \$= kernel_config_file,
>=C2=A0 ) or help(1);
>
>=C2=A0 help(0) if ($help);
> @@ -139,16 +140,15 @@ if (!$input_raw and ($squash_by_path or $squash_= by_filename)) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0exit(128);
>=C2=A0 }
>
> -if (!is_supported_architecture()) {
> -=C2=A0 =C2=A0 =C2=A0printf "\nScript does not support your archi= tecture, sorry.\n";
> -=C2=A0 =C2=A0 =C2=A0printf "\nCurrently we support: \n\n";<= br> > -=C2=A0 =C2=A0 =C2=A0foreach(@SUPPORTED_ARCHITECTURES) {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0printf "\t%s\n&q= uot;, $_;
> -=C2=A0 =C2=A0 =C2=A0}
> +show_detected_architecture() if $debug;
>
> -=C2=A0 =C2=A0 =C2=A0my $archname =3D $Config{archname};
> -=C2=A0 =C2=A0 =C2=A0printf "\n\$ perl -MConfig -e \'print \&= quot;\$Config{archname}\\n\"\'\n";
> -=C2=A0 =C2=A0 =C2=A0printf "%s\n", $archname;
> +if (!is_known_architecture()) {
> +=C2=A0 =C2=A0 =C2=A0printf STDERR "\nFATAL: Script does not reco= gnize your architecture\n";
> +
> +=C2=A0 =C2=A0 =C2=A0my $arch =3D `uname -m`;
> +=C2=A0 =C2=A0 =C2=A0chomp $arch;
> +=C2=A0 =C2=A0 =C2=A0printf "\n\$ uname -m\n";
> +=C2=A0 =C2=A0 =C2=A0printf "%s\n", $arch;
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0exit(129);
>=C2=A0 }

I am going to add a patch to use `uname -m` instead of `perl -MConfig
...` on powerpc. Please rebase your next version.

> @@ -168,21 +168,45 @@ sub dprint
>=C2=A0 =C2=A0 =C2=A0 =C2=A0printf(STDERR @_) if $debug;
>=C2=A0 }
>
> -sub is_supported_architecture
> +sub is_known_architecture

Please don't change this.

>=C2=A0 {
> -=C2=A0 =C2=A0 =C2=A0return (is_x86_64() or is_ppc64());
> +=C2=A0 =C2=A0 =C2=A0return (is_64bit() or is_32bit());
>=C2=A0 }
>
> -sub is_x86_64
> +sub is_32bit
>=C2=A0 {
> -=C2=A0 =C2=A0 =C2=A0my $archname =3D $Config{archname};
> +=C2=A0 =C2=A0 =C2=A0# 32-bit actual case
> +=C2=A0 =C2=A0 =C2=A0if (is_ix86_32()) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return 1;
> +=C2=A0 =C2=A0 =C2=A0}
>
> -=C2=A0 =C2=A0 =C2=A0if ($archname =3D~ m/x86_64/) {
> +=C2=A0 =C2=A0 =C2=A0# 32-bit "forced" case (for any arch ot= her than IA-32)
> +=C2=A0 =C2=A0 =C2=A0if ($opt_32_bit or $page_offset_32bit) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return 1;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0}
>=C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;
>=C2=A0 }

Perhaps


sub is_32bit
{
=C2=A0 =C2=A0 =C2=A0 =C2=A0 # Allow --32-bit and/or --page-offset-32bit to = override.
=C2=A0 =C2=A0 =C2=A0 =C2=A0 if ($opt_32_bit or $page_offset_32bit) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return 1;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 }

=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (is_ix86_32()) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return 1;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 }

=C2=A0 =C2=A0 =C2=A0 =C2=A0 return 0;
}


> +sub is_64bit
> +{
> +=C2=A0 =C2=A0 =C2=A0return (is_x86_64() or is_ppc64() or is_arm64() o= r is_mips64());
> +}
> +
> +sub is_x86_64
> +{
> +=C2=A0 =C2=A0 =C2=A0is_arch("x86_64");
> +}
> +
> +sub is_arm64
> +{
> +=C2=A0 =C2=A0 =C2=A0is_arch("aarch64");
> +}
> +
> +sub is_mips64
> +{
> +=C2=A0 =C2=A0 =C2=A0is_arch("mips64");
> +}
> +

We cannot add these last two. There has been no talk (to my knowledge) of t= he
address format for mips64 or aarch64. If you have tested for these please add as a separate patch.

>=C2=A0 sub is_ppc64
>=C2=A0 {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0my $archname =3D $Config{archname};
> @@ -193,6 +217,47 @@ sub is_ppc64
>=C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;
>=C2=A0 }
>
> +sub is_ix86_32
> +{
> +=C2=A0 =C2=A0 =C2=A0my $arch =3D `uname -m`;
> +
> +=C2=A0 =C2=A0 =C2=A0chomp $arch;
> +=C2=A0 =C2=A0 =C2=A0if ($arch =3D~ m/i[3456]86/) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return 1;
> +=C2=A0 =C2=A0 =C2=A0}
> +=C2=A0 =C2=A0 =C2=A0return 0;
> +}
> +
> +sub is_arch
> +{
> +=C2=A0 =C2=A0 =C2=A0my ($desc) =3D @_;
> +=C2=A0 =C2=A0 =C2=A0my $arch =3D `uname -m`;
> +
> +=C2=A0 =C2=A0 =C2=A0chomp $arch;
> +=C2=A0 =C2=A0 =C2=A0if ($arch eq $desc) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return 1;
> +=C2=A0 =C2=A0 =C2=A0}
> +=C2=A0 =C2=A0 =C2=A0return 0;
> +}
> +
> +sub show_detected_architecture
> +{
> +=C2=A0 =C2=A0 =C2=A0printf "Detected architecture: ";
> +=C2=A0 =C2=A0 =C2=A0if (is_32bit()) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0printf "32 bit\n= ";

is_32bit includes the command line options so this is not _totally_
correct. Prefer

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (is_x64_32()) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ...

> +=C2=A0 =C2=A0 =C2=A0} elsif (is_x86_64()) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0printf "x86_64\n= ";
> +=C2=A0 =C2=A0 =C2=A0} elsif (is_ppc64()) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0printf "PPC64\n&= quot;;
> +=C2=A0 =C2=A0 =C2=A0} elsif (is_arm64()) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0printf "ARM64\n&= quot;;
> +=C2=A0 =C2=A0 =C2=A0} elsif (is_mips64()) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0printf "MIPS64\n= ";
> +=C2=A0 =C2=A0 =C2=A0} else {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0printf "failed t= o detect architecture\n"
> +=C2=A0 =C2=A0 =C2=A0}
> +}
> +
>=C2=A0 # gets config option value from kernel config file
>=C2=A0 sub get_kernel_config_option
>=C2=A0 {
> @@ -212,7 +277,6 @@ sub get_kernel_config_option
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0} else {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0@config_files =3D ($tmp_file);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> -

Please don't do white space changes in the middle or another patch.

>=C2=A0 =C2=A0 =C2=A0 =C2=A0} else {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0my $file =3D = 9;/boot/config-' . `uname -r`;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0chomp $file;
> @@ -220,7 +284,6 @@ sub get_kernel_config_option
>=C2=A0 =C2=A0 =C2=A0 =C2=A0}
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0foreach my $file (@config_files) {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dprint("parsing = config file: %s\n", $file);

This should be a separate patch. 'One thing per patch' is the kerne= l mantra

>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0$value =3D optio= n_from_file($option, $file);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if ($value ne &q= uot;") {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0last;
> @@ -258,6 +321,14 @@ sub is_false_positive
>=C2=A0 {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0my ($match) =3D @_;
>
> +=C2=A0 =C2=A0 =C2=A0# 32 bit architectures, actual or forced
> +

No need for this comment.

> +=C2=A0 =C2=A0 =C2=A0if (is_32bit()) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return is_false_posit= ive_32bit($match);
> +=C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0# 64 bit architectures
> +

Or this one.

>=C2=A0 =C2=A0 =C2=A0 =C2=A0if ($match =3D~ '\b(0x)?(f|F){16}\b'= or
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0$match =3D~ '\b(0x)?0{16}\= b') {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return 1;
> @@ -281,6 +352,58 @@ sub is_in_vsyscall_memory_region
>=C2=A0 =C2=A0 =C2=A0 =C2=A0return ($hex >=3D $region_min and $hex &l= t;=3D $region_max);
>=C2=A0 }
>
> +sub is_false_positive_32bit
> +{
> +=C2=A0 =C2=A0 =C2=A0my ($match) =3D @_;
> +=C2=A0 =C2=A0 =C2=A0state $page_offset =3D get_page_offset(); # only = gets called once
> +
> +=C2=A0 =C2=A0 =C2=A0if ($match =3D~ '\b(0x)?(f|F){8}\b') { > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return 1;
> +=C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0my $addr32 =3D hex($match);
> +=C2=A0 =C2=A0 =C2=A0if ($addr32 < $page_offset) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return 1;
> +=C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0return 0;
> +}

The return value of this sub routine is confusing. Either it should be
commented as returning a hex value or it should return a string. I
prefer a string since the rest of the subs in the file return strings,
less cognitive load to keep everything the same. Unless there is some
clear advantage to returning a hex value (which I cannot see).

> +sub get_page_offset
> +{
> +=C2=A0 =C2=A0 =C2=A0my $page_offset;
> +=C2=A0 =C2=A0 =C2=A0my $default_offset =3D hex("0xc0000000"= );
> +=C2=A0 =C2=A0 =C2=A0my @config_files;
> +
> +=C2=A0 =C2=A0 =C2=A0# Allow --page-offset-32bit to override.
> +=C2=A0 =C2=A0 =C2=A0if ($page_offset_32bit !=3D 0) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return $page_offset_3= 2bit;
> +=C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0$page_offset =3D get_kernel_config_option('CO= NFIG_PAGE_OFFSET');
> +=C2=A0 =C2=A0 =C2=A0return $default_offset;
> +}

This is wrong.

> +
> +sub parse_kernel_config_file
> +{
> +=C2=A0 =C2=A0 =C2=A0my ($file) =3D @_;
> +=C2=A0 =C2=A0 =C2=A0my $config =3D 'CONFIG_PAGE_OFFSET';
> +=C2=A0 =C2=A0 =C2=A0my $str =3D "";
> +=C2=A0 =C2=A0 =C2=A0my $val =3D "";
> +
> +=C2=A0 =C2=A0 =C2=A0open(my $fh, "<", $file) or return &= quot;";
> +=C2=A0 =C2=A0 =C2=A0while (my $line =3D <$fh> ) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if ($line =3D~ /^$con= fig/) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0($str, $val) =3D split /=3D/, $line;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0chomp($val);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0last;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> +=C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0close $fh;
> +=C2=A0 =C2=A0 =C2=A0return $val;
> +}
> +

When do you call this subroutine?

>=C2=A0 # True if argument potentially contains a kernel address.
>=C2=A0 sub may_leak_address
>=C2=A0 {
> @@ -300,8 +423,6 @@ sub may_leak_address
>=C2=A0 =C2=A0 =C2=A0 =C2=A0}
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0$address_re =3D get_address_re();
> -=C2=A0 =C2=A0 =C2=A0dprint("Kernel address regular expression: %= s\n", $address_re);
> -

Please see comments above on 'one thing per patch'.

>=C2=A0 =C2=A0 =C2=A0 =C2=A0while (/($address_re)/g) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (!is_false_po= sitive($1)) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0return 1;
> @@ -313,16 +434,17 @@ sub may_leak_address
>
>=C2=A0 sub get_address_re
>=C2=A0 {
> -=C2=A0 =C2=A0 =C2=A0my $re;
> +=C2=A0 =C2=A0 =C2=A0my $re =3D "";
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (is_x86_64()) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0$re =3D get_x86_= 64_re();
>=C2=A0 =C2=A0 =C2=A0 =C2=A0} elsif (is_ppc64()) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0$re =3D '\b(= 0x)?[89abcdef]00[[:xdigit:]]{13}\b';
> -=C2=A0 =C2=A0 =C2=A0}
> -
> -=C2=A0 =C2=A0 =C2=A0if ($re eq "") {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0print STDERR "$0= : failed to build kernel address regular expression\n";
> +=C2=A0 =C2=A0 =C2=A0###
> +=C2=A0 =C2=A0 =C2=A0# Any special cases for other arch's go below= this line
> +=C2=A0 =C2=A0 =C2=A0###
> +=C2=A0 =C2=A0 =C2=A0} else {=C2=A0 # nothing? then we assume it's= a generic 32-bit
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0$re =3D '\b(0x)?[= [:xdigit:]]{8}\b';
>=C2=A0 =C2=A0 =C2=A0 =C2=A0}
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0return $re;

This is messy. We definitely want to warn if we cannot fully determine
the correct regex to use. Also, if you would like to default to 32-bit
you will need to back it up with some reasoning. Currently there is no
default if the correct regex cannot be ascertained. At this stage I feel this is the correct behaviour, I'm open to discussion though.

> --
> 2.14.3


thanks,
Tobin.
--001a113de47e92119a0562a6388c--