Message-ID: <1512133747.17323.3.camel@gmail.com>
Date:   Fri, 01 Dec 2017 18:39:07 +0530
From:   kaiwan.billimoria@...il.com
To:     "Tobin C. Harding" <me@...in.cc>
Cc:     Alexander Kapshuk <alexander.kapshuk@...il.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        kernel-hardening@...ts.openwall.com
Subject: Re: [PATCH] leaking_addresses: add support for 32-bit kernel
 addresses

Hi,

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

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

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

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

On Thu, 2017-11-30 at 07:48 +1100, Tobin C. Harding wrote:
> On Wed, Nov 29, 2017 at 04:32:54PM +0530, Kaiwan N Billimoria wrote:
> > Hi,
> > 
> > On Wed, Nov 29, 2017 at 3:46 PM, Tobin C. Harding <me@...in.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@...in.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@...in.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.

Powered by blists - more mailing lists