[PATCH 0/4] leaking_addresses: simplify and optimize

[PATCH 0/4] leaking_addresses: simplify and optimize

[Tobin C. Harding]

leaking_addresses.pl is currently woefully slow.  This series fixes
that.  Also, currently configuring which files/directories to skip is
overly complicated.  We can simplify the code configuration quite easily
by combining the configuration arrays.

This series does not apply on top of the mainline but is available here:

git://git.kernel.org/pub/scm/linux/kernel/git/tobin/leaks.git leaks-testing


thanks,
Tobin.

Tobin C. Harding (4):
  leaking_addresses: do not parse binary files
  leaking_addresses: simplify path skipping
  leaking_addresses: cache architecture name
  leaking_addresses: add scan_once array

 scripts/leaking_addresses.pl | 125 ++++++++++++++++++++++---------------------
 1 file changed, 64 insertions(+), 61 deletions(-)

-- 
2.7.4

[PATCH 1/4] leaking_addresses: do not parse binary files

[Tobin C. Harding]

Currently script parses binary files.  Since we are scanning for
readable kernel addresses there is no need to parse binary files.  We
can use Perl to check if file is binary and skip parsing it if so.

Do not parse binary files.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 scripts/leaking_addresses.pl | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
index 05906f6cf6b9..3d5c3096aac8 100755
--- a/scripts/leaking_addresses.pl
+++ b/scripts/leaking_addresses.pl
@@ -462,6 +462,10 @@ sub parse_file
 		return;
 	}
 
+	if (! -T $file) {
+		return;
+	}
+
 	if (skip_parse($file)) {
 		dprint "skipping file: $file\n";
 		return;
-- 
2.7.4

[PATCH 2/4] leaking_addresses: simplify path skipping

[Tobin C. Harding]

Currently script has multiple configuration arrays.  This is confusing,
evident by the fact that a bunch of the entries are in the wrong place.
We can simplify the code by just having a single array for absolute
paths to skip and a single array for file names to skip wherever they
appear in the scanned directory tree.  There are also currently multiple
subroutines to handle the different arrays, we can reduce these to a
single subroutine also.

Simplify the path skipping code.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 scripts/leaking_addresses.pl | 90 ++++++++++++++------------------------------
 1 file changed, 29 insertions(+), 61 deletions(-)

diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
index 3d5c3096aac8..e7bf15a45a69 100755
--- a/scripts/leaking_addresses.pl
+++ b/scripts/leaking_addresses.pl
@@ -48,41 +48,26 @@ my $kernel_config_file = "";	# Kernel configuration file.
 my $opt_32bit = 0;		# Scan 32-bit kernel.
 my $page_offset_32bit = 0;	# Page offset for 32-bit kernel.
 
-# Do not parse these files (absolute path).
-my @skip_parse_files_abs = ('/proc/kmsg',
-			    '/proc/kcore',
-			    '/proc/fs/ext4/sdb1/mb_groups',
-			    '/proc/1/fd/3',
-			    '/sys/firmware/devicetree',
-			    '/proc/device-tree',
-			    '/sys/kernel/debug/tracing/trace_pipe',
-			    '/sys/kernel/security/apparmor/revision');
-
-# Do not parse these files under any subdirectory.
-my @skip_parse_files_any = ('0',
-			    '1',
-			    '2',
-			    'pagemap',
-			    'events',
-			    'access',
-			    'registers',
-			    'snapshot_raw',
-			    'trace_pipe_raw',
-			    'ptmx',
-			    'trace_pipe');
-
-# Do not walk these directories (absolute path).
-my @skip_walk_dirs_abs = ();
-
-# Do not walk these directories under any subdirectory.
-my @skip_walk_dirs_any = ('self',
-			  'thread-self',
-			  'cwd',
-			  'fd',
-			  'usbmon',
-			  'stderr',
-			  'stdin',
-			  'stdout');
+# Skip these absolute paths.
+my @skip_abs = (
+	'/proc/kmsg',
+	'/sys/firmware/devicetree',
+	'/proc/device-tree',
+	'/sys/kernel/debug/tracing/trace_pipe',
+	'/sys/kernel/security/apparmor/revision');
+
+# Skip these under any subdirectory.
+my @skip_any = (
+	'pagemap',
+	'events',
+	'access',
+	'registers',
+	'snapshot_raw',
+	'trace_pipe_raw',
+	'ptmx',
+	'trace_pipe',
+	'fd',
+	'usbmon');
 
 sub help
 {
@@ -417,26 +402,20 @@ sub parse_dmesg
 # True if we should skip this path.
 sub skip
 {
-	my ($path, $paths_abs, $paths_any) = @_;
+	my ($path) = @_;
 
-	foreach (@$paths_abs) {
+	foreach (@skip_abs) {
 		return 1 if (/^$path$/);
 	}
 
 	my($filename, $dirs, $suffix) = fileparse($path);
-	foreach (@$paths_any) {
+	foreach (@skip_any) {
 		return 1 if (/^$filename$/);
 	}
 
 	return 0;
 }
 
-sub skip_parse
-{
-	my ($path) = @_;
-	return skip($path, \@skip_parse_files_abs, \@skip_parse_files_any);
-}
-
 sub timed_parse_file
 {
 	my ($file) = @_;
@@ -466,12 +445,6 @@ sub parse_file
 		return;
 	}
 
-	if (skip_parse($file)) {
-		dprint "skipping file: $file\n";
-		return;
-	}
-	dprint "parsing: $file\n";
-
 	open my $fh, "<", $file or return;
 	while ( <$fh> ) {
 		if (may_leak_address($_)) {
@@ -481,21 +454,12 @@ sub parse_file
 	close $fh;
 }
 
-
-# True if we should skip walking this directory.
-sub skip_walk
-{
-	my ($path) = @_;
-	return skip($path, \@skip_walk_dirs_abs, \@skip_walk_dirs_any)
-}
-
 # Recursively walk directory tree.
 sub walk
 {
 	my @dirs = @_;
 
 	while (my $pwd = shift @dirs) {
-		next if (skip_walk($pwd));
 		next if (!opendir(DIR, $pwd));
 		my @files = readdir(DIR);
 		closedir(DIR);
@@ -506,11 +470,15 @@ sub walk
 			my $path = "$pwd/$file";
 			next if (-l $path);
 
+			next if (skip($path));
+
 			if (-d $path) {
 				push @dirs, $path;
-			} else {
-				timed_parse_file($path);
+				next;
 			}
+
+			dprint "parsing: $file\n";
+			timed_parse_file($path);
 		}
 	}
 }
-- 
2.7.4

Re: [PATCH 2/4] leaking_addresses: simplify path skipping

[Tycho Andersen]

Hi Tobin,

On Mon, Feb 19, 2018 at 01:50:47PM +1100, Tobin C. Harding wrote:
> -# Do not parse these files under any subdirectory.
> -my @skip_parse_files_any = ('0',
> -			    '1',
> -			    '2',
> -			    'pagemap',
> -			    'events',
> -			    'access',
> -			    'registers',
> -			    'snapshot_raw',
> -			    'trace_pipe_raw',
> -			    'ptmx',
> -			    'trace_pipe');

It might be worth adding 'syscall' here; the pointers listed are user
pointers, and negative syscall args will show up like kernel pointers,
e.g. I get this output, which is spurious:

/proc/31808/syscall: 0 0x3 0x55b107a38180 0x2000 0xffffffffffffffb0 0x55b107a302d0 0x55b107a38180 0x7fffa313b8e8 0x7ff098560d11

Cheers,

Tycho

Re: [PATCH 2/4] leaking_addresses: simplify path skipping

[Tobin C. Harding]

On Sun, Feb 25, 2018 at 06:26:31PM -0700, Tycho Andersen wrote:
> Hi Tobin,
> 
> On Mon, Feb 19, 2018 at 01:50:47PM +1100, Tobin C. Harding wrote:
> > -# Do not parse these files under any subdirectory.
> > -my @skip_parse_files_any = ('0',
> > -			    '1',
> > -			    '2',
> > -			    'pagemap',
> > -			    'events',
> > -			    'access',
> > -			    'registers',
> > -			    'snapshot_raw',
> > -			    'trace_pipe_raw',
> > -			    'ptmx',
> > -			    'trace_pipe');
> 
> It might be worth adding 'syscall' here; the pointers listed are user
> pointers, and negative syscall args will show up like kernel pointers,
> e.g. I get this output, which is spurious:
> 
> /proc/31808/syscall: 0 0x3 0x55b107a38180 0x2000 0xffffffffffffffb0 0x55b107a302d0 0x55b107a38180 0x7fffa313b8e8 0x7ff098560d11

Nice.  Will add.

thanks,
Tobin.

[PATCH 3/4] leaking_addresses: cache architecture name

[Tobin C. Harding]

Currently we are repeatedly calling `uname -m`.  This is causing the
script to take a long time to run (more than 10 seconds to parse
/proc/kallsyms).  We can use Perl state variables to cache the result of
the first call to `uname -m`.  With this change in place the script
scans the whole kernel in under a minute.

Cache machine architecture in state variable.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 scripts/leaking_addresses.pl | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
index e7bf15a45a69..f52e91ef7d5c 100755
--- a/scripts/leaking_addresses.pl
+++ b/scripts/leaking_addresses.pl
@@ -175,7 +175,7 @@ sub is_32bit
 
 sub is_ix86_32
 {
-       my $arch = `uname -m`;
+       state $arch = `uname -m`;
 
        chomp $arch;
        if ($arch =~ m/i[3456]86/) {
@@ -198,12 +198,14 @@ sub is_arch
 
 sub is_x86_64
 {
-	return is_arch('x86_64');
+	state $is = is_arch('x86_64');
+	return $is;
 }
 
 sub is_ppc64
 {
-	return is_arch('ppc64');
+	state $is = is_arch('ppc64');
+	return $is;
 }
 
 # Gets config option value from kernel config file.
-- 
2.7.4

[PATCH 4/4] leaking_addresses: add scan_once array

[Tobin C. Harding]

There are files under /proc that have the same format for each PID, e.g
'smaps'.  We need only scan these files a single time to verify that
they are not leaking addresses.  This reduces the work the script must
do.

Add once_only array.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 scripts/leaking_addresses.pl | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
index f52e91ef7d5c..ab4e70d9efde 100755
--- a/scripts/leaking_addresses.pl
+++ b/scripts/leaking_addresses.pl
@@ -69,6 +69,12 @@ my @skip_any = (
 	'fd',
 	'usbmon');
 
+# These files are the same format under each PID that they appear.
+# We need only pass them once.
+my @once_only = (
+	'smaps',
+	'mb_groups');
+
 sub help
 {
 	my ($exitcode) = @_;
@@ -401,6 +407,25 @@ sub parse_dmesg
 	close $cmd;
 }
 
+sub already_scanned
+{
+	my ($filename) = @_;
+	state %seen;
+
+	foreach (@once_only) {
+		if (/^$filename$/) {
+			if ($seen{$_} == 1) {
+				return 1;
+			}
+			$seen{$_} = 1;
+
+			return 0;
+		}
+	}
+
+	return 0;
+}
+
 # True if we should skip this path.
 sub skip
 {
@@ -415,6 +440,10 @@ sub skip
 		return 1 if (/^$filename$/);
 	}
 
+	if (already_scanned($filename)) {
+		return 1;
+	}
+
 	return 0;
 }
 
-- 
2.7.4

Re: [PATCH 4/4] leaking_addresses: add scan_once array

[Tycho Andersen]

Hi Tobin,

On Mon, Feb 19, 2018 at 01:50:49PM +1100, Tobin C. Harding wrote:
> +sub already_scanned
> +{
> +	my ($filename) = @_;
> +	state %seen;
> +
> +	foreach (@once_only) {
> +		if (/^$filename$/) {
> +			if ($seen{$_} == 1) {

This should be something like,

if (($seen{$_} //= 0) == 1) {

otherwise I get a bunch of uninitialized warnings,

Use of uninitialized value in pattern match (m//) at /usr/share/perl/5.26/Math/BigInt.pm line 1199.
    Math::BigInt::bcmp(Math::BigInt=HASH(0x55dc2f7e4580), undef) called at /usr/share/perl/5.26/Math/BigInt.pm line 1257
    Math::BigInt::beq(Math::BigInt=HASH(0x55dc2f7e4580), undef) called at /usr/share/perl/5.26/Math/BigInt.pm line 105
    Math::BigInt::__ANON__(Math::BigInt=HASH(0x55dc2f7e4580), undef, 1) called at ./leaking_addresses.pl line 422
    main::already_scanned("smaps") called at ./leaking_addresses.pl line 448
    main::skip("/proc/1/smaps") called at ./leaking_addresses.pl line 509
    main::walk("/proc", "/sys") called at ./leaking_addresses.pl line 159

Tycho

Re: [PATCH 4/4] leaking_addresses: add scan_once array

[Tobin C. Harding]

On Sun, Feb 25, 2018 at 06:09:53PM -0700, Tycho Andersen wrote:
> Hi Tobin,
> 
> On Mon, Feb 19, 2018 at 01:50:49PM +1100, Tobin C. Harding wrote:
> > +sub already_scanned
> > +{
> > +	my ($filename) = @_;
> > +	state %seen;
> > +
> > +	foreach (@once_only) {
> > +		if (/^$filename$/) {
> > +			if ($seen{$_} == 1) {
> 
> This should be something like,
> 
> if (($seen{$_} //= 0) == 1) {
> 
> otherwise I get a bunch of uninitialized warnings,
> 
> Use of uninitialized value in pattern match (m//) at /usr/share/perl/5.26/Math/BigInt.pm line 1199.
>     Math::BigInt::bcmp(Math::BigInt=HASH(0x55dc2f7e4580), undef) called at /usr/share/perl/5.26/Math/BigInt.pm line 1257
>     Math::BigInt::beq(Math::BigInt=HASH(0x55dc2f7e4580), undef) called at /usr/share/perl/5.26/Math/BigInt.pm line 105
>     Math::BigInt::__ANON__(Math::BigInt=HASH(0x55dc2f7e4580), undef, 1) called at ./leaking_addresses.pl line 422
>     main::already_scanned("smaps") called at ./leaking_addresses.pl line 448
>     main::skip("/proc/1/smaps") called at ./leaking_addresses.pl line 509
>     main::walk("/proc", "/sys") called at ./leaking_addresses.pl line 159
> 
> Tycho

Cool, thanks.  Will fix and re-spin.


	Tobin