[PATCH v3] scripts: add leaking_addresses.pl

[PATCH v3] scripts: add leaking_addresses.pl

[Tobin C. Harding]

Currently we are leaking addresses from the kernel to user space. This
script is an attempt to find some of those leakages. Script parses
`dmesg` output and /proc and /sys files for hex strings that look like
kernel addresses.

Only works for 64 bit kernels, the reason being that kernel addresses
on 64 bit kernels have 'ffff' as the leading bit pattern making greping
possible. On 32 kernels we don't have this luxury.

Scripts is _slightly_ smarter than a straight grep, we check for false
positives (all 0's or all 1's, and vsyscall start/finish addresses).

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---

v3:
 - Iterate matches to check for results instead of matching input line against
   false positives i.e catch lines that contain results as well as false
   positives. 

v2:
 - Add regex's to prevent false positives.
 - Clean up white space.

 MAINTAINERS                  |   5 +
 scripts/leaking_addresses.pl | 306 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 311 insertions(+)
 create mode 100755 scripts/leaking_addresses.pl

diff --git a/MAINTAINERS b/MAINTAINERS
index e652a3e2929d..c1ad6d133a57 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7745,6 +7745,11 @@ S:	Maintained
 F:	Documentation/scsi/53c700.txt
 F:	drivers/scsi/53c700*
 
+LEAKING_ADDRESSES
+M:	Tobin C. Harding <me@tobin.cc>
+S:	Maintained
+F:	scripts/leaking_addresses.pl
+
 LED SUBSYSTEM
 M:	Richard Purdie <rpurdie@rpsys.net>
 M:	Jacek Anaszewski <jacek.anaszewski@gmail.com>
diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
new file mode 100755
index 000000000000..1487a75cfc52
--- /dev/null
+++ b/scripts/leaking_addresses.pl
@@ -0,0 +1,306 @@
+#!/usr/bin/env perl
+#
+# (c) 2017 Tobin C. Harding <me@tobin.cc>
+# Licensed under the terms of the GNU GPL License version 2
+#
+# leaking_addresses.pl: Scan 64 bit kernel for potential leaking addresses.
+#  - Scans dmesg output.
+#  - Walks directory tree and parses each file (for each directory in @DIRS).
+#
+# You can configure the behaviour of the script;
+#
+#  - By adding paths, for directories you do not want to walk;
+#     absolute paths: @skip_walk_dirs_abs
+#     directory names: @skip_walk_dirs_any
+#
+#  - By adding paths, for files you do not want to parse;
+#     absolute paths: @skip_parse_files_abs
+#     file names: @skip_parse_files_any
+#
+# The use of @skip_xxx_xxx_any causes files to be skipped where ever they occur.
+# For example adding 'fd' to @skip_walk_dirs_any causes the fd/ directory to be
+# skipped for all PID sub-directories of /proc
+#
+# The same thing can be achieved by passing command line options to --dont-walk
+# and --dont-parse. If absolute paths are supplied to these options they are
+# appended to the @skip_xxx_xxx_abs arrays. If file names are supplied to these
+# options, they are appended to the @skip_xxx_xxx_any arrays.
+#
+# Use --debug to output path before parsing, this is useful to find files that
+# cause the script to choke.
+#
+# You may like to set kptr_restrict=2 before running script
+# (see Documentation/sysctl/kernel.txt).
+
+use warnings;
+use strict;
+use POSIX;
+use File::Basename;
+use File::Spec;
+use Cwd 'abs_path';
+use Term::ANSIColor qw(:constants);
+use Getopt::Long qw(:config no_auto_abbrev);
+
+my $P = $0;
+my $V = '0.01';
+
+# Directories to scan.
+my @DIRS = ('/proc', '/sys');
+
+# Command line options.
+my $help = 0;
+my $debug = 0;
+my @dont_walk = ();
+my @dont_parse = ();
+
+# 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/kernel/debug/tracing/trace_pipe',
+			    '/sys/kernel/security/apparmor/revision');
+
+# Do not parse thes 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',
+			  'stderr',
+			  'stdin',
+			  'stdout');
+
+sub help
+{
+	my ($exitcode) = @_;
+
+	print << "EOM";
+Usage: $P [OPTIONS]
+Version: $V
+
+Options:
+
+	--dont-walk=<dir>      Don't walk tree starting at <dir>.
+	--dont-parse=<file>    Don't parse <file>.
+	-d, --debug                Display debugging output.
+	-h, --help, --version      Display this help and exit.
+
+If an absolute path is passed to --dont_XXX then this path is skipped. If a
+single filename is passed then this file/directory will be skipped when
+appearing under any subdirectory.
+
+Example:
+
+	# Just scan dmesg output.
+	scripts/leaking_addresses.pl --dont_walk_abs /proc --dont_walk_abs /sys
+
+Scans the running (64 bit) kernel for potential leaking addresses.
+
+EOM
+	exit($exitcode);
+}
+
+GetOptions(
+	'dont-walk=s'		=> \@dont_walk,
+	'dont-parse=s'		=> \@dont_parse,
+	'd|debug'		=> \$debug,
+	'h|help'		=> \$help,
+	'version'		=> \$help
+) or help(1);
+
+help(0) if ($help);
+
+push_to_global();
+
+parse_dmesg();
+walk(@DIRS);
+
+exit 0;
+
+sub debug_arrays
+{
+	print 'dirs_any: ' . join(", ", @skip_walk_dirs_any) . "\n";
+	print 'dirs_abs: ' . join(", ", @skip_walk_dirs_abs) . "\n";
+	print 'parse_any: ' . join(", ", @skip_parse_files_any) . "\n";
+	print 'parse_abs: ' . join(", ", @skip_parse_files_abs) . "\n";
+}
+
+sub dprint
+{
+	printf(STDERR @_) if $debug;
+}
+
+sub push_in_abs_any
+{
+	my ($in, $abs, $any) = @_;
+
+	foreach my $path (@$in) {
+		if (File::Spec->file_name_is_absolute($path)) {
+			push @$abs, $path;
+		} elsif (index($path,'/') == -1) {
+			push @$any, $path;
+		} else {
+			print 'path error: ' . $path;
+		}
+	}
+}
+
+# Push command line options to global arrays.
+sub push_to_global
+{
+	push_in_abs_any(\@dont_walk, \@skip_walk_dirs_abs, \@skip_walk_dirs_any);
+	push_in_abs_any(\@dont_parse, \@skip_parse_files_abs, \@skip_parse_files_any);
+}
+
+sub is_false_positive
+{
+        my ($match) = @_;
+
+        if ($match =~ '\b(0x)?(f|F){16}\b' or
+            $match =~ '\b(0x)?0{16}\b') {
+                return 1;
+        }
+
+        # vsyscall memory region, we should probably check against a range here.
+        if ($match =~ '\bf{10}600000\b' or
+            $match =~ '\bf{10}601000\b') {
+                return 1;
+        }
+
+        return 0;
+}
+
+# True if argument potentially contains a kernel address.
+sub may_leak_address
+{
+        my ($line) = @_;
+        my $address = '\b(0x)?ffff[[:xdigit:]]{12}\b';
+
+        # Signal masks.
+        if ($line =~ '^SigBlk:' or
+            $line =~ '^SigCgt:') {
+                return 0;
+        }
+
+        if ($line =~ '\bKEY=[[:xdigit:]]{14} [[:xdigit:]]{16} [[:xdigit:]]{16}\b' or
+            $line =~ '\b[[:xdigit:]]{14} [[:xdigit:]]{16} [[:xdigit:]]{16}\b') {
+		return 0;
+        }
+
+        while (/($address)/g) {
+                if (!is_false_positive($1)) {
+                        return 1;
+                }
+        }
+
+        return 0;
+}
+
+sub parse_dmesg
+{
+	open my $cmd, '-|', 'dmesg';
+	while (<$cmd>) {
+		if (may_leak_address($_)) {
+			print 'dmesg: ' . $_;
+		}
+	}
+	close $cmd;
+}
+
+# True if we should skip this path.
+sub skip
+{
+	my ($path, $paths_abs, $paths_any) = @_;
+
+	foreach (@$paths_abs) {
+		return 1 if (/^$path$/);
+	}
+
+	my($filename, $dirs, $suffix) = fileparse($path);
+	foreach (@$paths_any) {
+		return 1 if (/^$filename$/);
+	}
+
+	return 0;
+}
+
+sub skip_parse
+{
+	my ($path) = @_;
+	return skip($path, \@skip_parse_files_abs, \@skip_parse_files_any);
+}
+
+sub parse_file
+{
+	my ($file) = @_;
+
+	if (! -R $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($_)) {
+			print $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 = @_;
+	my %seen;
+
+	while (my $pwd = shift @dirs) {
+		next if (skip_walk($pwd));
+		next if (!opendir(DIR, $pwd));
+		my @files = readdir(DIR);
+		closedir(DIR);
+
+		foreach my $file (@files) {
+			next if ($file eq '.' or $file eq '..');
+
+			my $path = "$pwd/$file";
+			next if (-l $path);
+
+			if (-d $path) {
+				push @dirs, $path;
+			} else {
+				parse_file($path);
+			}
+		}
+	}
+}
+
-- 
2.7.4

[kernel-hardening] [PATCH v3] scripts: add leaking_addresses.pl

[Tobin C. Harding]

Currently we are leaking addresses from the kernel to user space. This
script is an attempt to find some of those leakages. Script parses
`dmesg` output and /proc and /sys files for hex strings that look like
kernel addresses.

Only works for 64 bit kernels, the reason being that kernel addresses
on 64 bit kernels have 'ffff' as the leading bit pattern making greping
possible. On 32 kernels we don't have this luxury.

Scripts is _slightly_ smarter than a straight grep, we check for false
positives (all 0's or all 1's, and vsyscall start/finish addresses).

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---

v3:
 - Iterate matches to check for results instead of matching input line against
   false positives i.e catch lines that contain results as well as false
   positives. 

v2:
 - Add regex's to prevent false positives.
 - Clean up white space.

 MAINTAINERS                  |   5 +
 scripts/leaking_addresses.pl | 306 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 311 insertions(+)
 create mode 100755 scripts/leaking_addresses.pl

diff --git a/MAINTAINERS b/MAINTAINERS
index e652a3e2929d..c1ad6d133a57 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7745,6 +7745,11 @@ S:	Maintained
 F:	Documentation/scsi/53c700.txt
 F:	drivers/scsi/53c700*
 
+LEAKING_ADDRESSES
+M:	Tobin C. Harding <me@tobin.cc>
+S:	Maintained
+F:	scripts/leaking_addresses.pl
+
 LED SUBSYSTEM
 M:	Richard Purdie <rpurdie@rpsys.net>
 M:	Jacek Anaszewski <jacek.anaszewski@gmail.com>
diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
new file mode 100755
index 000000000000..1487a75cfc52
--- /dev/null
+++ b/scripts/leaking_addresses.pl
@@ -0,0 +1,306 @@
+#!/usr/bin/env perl
+#
+# (c) 2017 Tobin C. Harding <me@tobin.cc>
+# Licensed under the terms of the GNU GPL License version 2
+#
+# leaking_addresses.pl: Scan 64 bit kernel for potential leaking addresses.
+#  - Scans dmesg output.
+#  - Walks directory tree and parses each file (for each directory in @DIRS).
+#
+# You can configure the behaviour of the script;
+#
+#  - By adding paths, for directories you do not want to walk;
+#     absolute paths: @skip_walk_dirs_abs
+#     directory names: @skip_walk_dirs_any
+#
+#  - By adding paths, for files you do not want to parse;
+#     absolute paths: @skip_parse_files_abs
+#     file names: @skip_parse_files_any
+#
+# The use of @skip_xxx_xxx_any causes files to be skipped where ever they occur.
+# For example adding 'fd' to @skip_walk_dirs_any causes the fd/ directory to be
+# skipped for all PID sub-directories of /proc
+#
+# The same thing can be achieved by passing command line options to --dont-walk
+# and --dont-parse. If absolute paths are supplied to these options they are
+# appended to the @skip_xxx_xxx_abs arrays. If file names are supplied to these
+# options, they are appended to the @skip_xxx_xxx_any arrays.
+#
+# Use --debug to output path before parsing, this is useful to find files that
+# cause the script to choke.
+#
+# You may like to set kptr_restrict=2 before running script
+# (see Documentation/sysctl/kernel.txt).
+
+use warnings;
+use strict;
+use POSIX;
+use File::Basename;
+use File::Spec;
+use Cwd 'abs_path';
+use Term::ANSIColor qw(:constants);
+use Getopt::Long qw(:config no_auto_abbrev);
+
+my $P = $0;
+my $V = '0.01';
+
+# Directories to scan.
+my @DIRS = ('/proc', '/sys');
+
+# Command line options.
+my $help = 0;
+my $debug = 0;
+my @dont_walk = ();
+my @dont_parse = ();
+
+# 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/kernel/debug/tracing/trace_pipe',
+			    '/sys/kernel/security/apparmor/revision');
+
+# Do not parse thes 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',
+			  'stderr',
+			  'stdin',
+			  'stdout');
+
+sub help
+{
+	my ($exitcode) = @_;
+
+	print << "EOM";
+Usage: $P [OPTIONS]
+Version: $V
+
+Options:
+
+	--dont-walk=<dir>      Don't walk tree starting at <dir>.
+	--dont-parse=<file>    Don't parse <file>.
+	-d, --debug                Display debugging output.
+	-h, --help, --version      Display this help and exit.
+
+If an absolute path is passed to --dont_XXX then this path is skipped. If a
+single filename is passed then this file/directory will be skipped when
+appearing under any subdirectory.
+
+Example:
+
+	# Just scan dmesg output.
+	scripts/leaking_addresses.pl --dont_walk_abs /proc --dont_walk_abs /sys
+
+Scans the running (64 bit) kernel for potential leaking addresses.
+
+EOM
+	exit($exitcode);
+}
+
+GetOptions(
+	'dont-walk=s'		=> \@dont_walk,
+	'dont-parse=s'		=> \@dont_parse,
+	'd|debug'		=> \$debug,
+	'h|help'		=> \$help,
+	'version'		=> \$help
+) or help(1);
+
+help(0) if ($help);
+
+push_to_global();
+
+parse_dmesg();
+walk(@DIRS);
+
+exit 0;
+
+sub debug_arrays
+{
+	print 'dirs_any: ' . join(", ", @skip_walk_dirs_any) . "\n";
+	print 'dirs_abs: ' . join(", ", @skip_walk_dirs_abs) . "\n";
+	print 'parse_any: ' . join(", ", @skip_parse_files_any) . "\n";
+	print 'parse_abs: ' . join(", ", @skip_parse_files_abs) . "\n";
+}
+
+sub dprint
+{
+	printf(STDERR @_) if $debug;
+}
+
+sub push_in_abs_any
+{
+	my ($in, $abs, $any) = @_;
+
+	foreach my $path (@$in) {
+		if (File::Spec->file_name_is_absolute($path)) {
+			push @$abs, $path;
+		} elsif (index($path,'/') == -1) {
+			push @$any, $path;
+		} else {
+			print 'path error: ' . $path;
+		}
+	}
+}
+
+# Push command line options to global arrays.
+sub push_to_global
+{
+	push_in_abs_any(\@dont_walk, \@skip_walk_dirs_abs, \@skip_walk_dirs_any);
+	push_in_abs_any(\@dont_parse, \@skip_parse_files_abs, \@skip_parse_files_any);
+}
+
+sub is_false_positive
+{
+        my ($match) = @_;
+
+        if ($match =~ '\b(0x)?(f|F){16}\b' or
+            $match =~ '\b(0x)?0{16}\b') {
+                return 1;
+        }
+
+        # vsyscall memory region, we should probably check against a range here.
+        if ($match =~ '\bf{10}600000\b' or
+            $match =~ '\bf{10}601000\b') {
+                return 1;
+        }
+
+        return 0;
+}
+
+# True if argument potentially contains a kernel address.
+sub may_leak_address
+{
+        my ($line) = @_;
+        my $address = '\b(0x)?ffff[[:xdigit:]]{12}\b';
+
+        # Signal masks.
+        if ($line =~ '^SigBlk:' or
+            $line =~ '^SigCgt:') {
+                return 0;
+        }
+
+        if ($line =~ '\bKEY=[[:xdigit:]]{14} [[:xdigit:]]{16} [[:xdigit:]]{16}\b' or
+            $line =~ '\b[[:xdigit:]]{14} [[:xdigit:]]{16} [[:xdigit:]]{16}\b') {
+		return 0;
+        }
+
+        while (/($address)/g) {
+                if (!is_false_positive($1)) {
+                        return 1;
+                }
+        }
+
+        return 0;
+}
+
+sub parse_dmesg
+{
+	open my $cmd, '-|', 'dmesg';
+	while (<$cmd>) {
+		if (may_leak_address($_)) {
+			print 'dmesg: ' . $_;
+		}
+	}
+	close $cmd;
+}
+
+# True if we should skip this path.
+sub skip
+{
+	my ($path, $paths_abs, $paths_any) = @_;
+
+	foreach (@$paths_abs) {
+		return 1 if (/^$path$/);
+	}
+
+	my($filename, $dirs, $suffix) = fileparse($path);
+	foreach (@$paths_any) {
+		return 1 if (/^$filename$/);
+	}
+
+	return 0;
+}
+
+sub skip_parse
+{
+	my ($path) = @_;
+	return skip($path, \@skip_parse_files_abs, \@skip_parse_files_any);
+}
+
+sub parse_file
+{
+	my ($file) = @_;
+
+	if (! -R $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($_)) {
+			print $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 = @_;
+	my %seen;
+
+	while (my $pwd = shift @dirs) {
+		next if (skip_walk($pwd));
+		next if (!opendir(DIR, $pwd));
+		my @files = readdir(DIR);
+		closedir(DIR);
+
+		foreach my $file (@files) {
+			next if ($file eq '.' or $file eq '..');
+
+			my $path = "$pwd/$file";
+			next if (-l $path);
+
+			if (-d $path) {
+				push @dirs, $path;
+			} else {
+				parse_file($path);
+			}
+		}
+	}
+}
+
-- 
2.7.4

Re: [PATCH v3] scripts: add leaking_addresses.pl

[Linus Torvalds]

On Sun, Nov 5, 2017 at 9:19 PM, Tobin C. Harding <me@tobin.cc> wrote:
> Currently we are leaking addresses from the kernel to user space. This
> script is an attempt to find some of those leakages. Script parses
> `dmesg` output and /proc and /sys files for hex strings that look like
> kernel addresses.

Lovely. This is great. It shows just how much totally pointless stuff
we leak, and to normal users that really shouldn't need it.

I had planned to wait for 4.15 to look at the printk hashing stuff
etc, but this part I think I could/should merge early just because I
think a lot of kernel developers will go "Why the f*ck would we expose
that kernel address there?"

The module sections stuff etc should likely be obviously root-only,
although maybe I'm missing some tool that ends up using it and is
useful to normal developers.

And I'm thinking we could make kallsyms smarter too, and instead of
depending on kptr_restrict that screws over things with much too big a
hammer, we could make it take 'perf_event_paranoid' into account. I
suspect that's the main user of kallsyms that would still be relevant
to non-root.

I really really hate kptr_restrict due to that whole "all or nothing" thing.

The networking code ends up having a ton of "sk" pointers, and I
suspect those would all be fine just using the hashed address, since
nobody is going to care about the _actual_ kernel address, but
matching a socket reference in one file against another one makes tons
of sense and I would not be surprised if there's a lot of netstat-like
tools that do that. So the whole '%pK' thing is entirely useless for
them, but the "hash %p values" should work fine.

Adding netdev and David Miller explicitly to the cc, since many of the
kernel pointer leaks are from them, and they may not even have been
following this discussion at all.

David - you can see the patch on patchwork:

    https://patchwork.kernel.org/patch/10042605/

and try it out yourself.

I heartily recommend other developers run it too, just to see if it
makes you go "Oh, I didn't even realize.."

This looks much more interesting than the whole "%p vs %pK vs %x" discussion.

                  Linus

Re: [PATCH v3] scripts: add leaking_addresses.pl

[Linus Torvalds]

On Sun, Nov 5, 2017 at 9:19 PM, Tobin C. Harding <me@tobin.cc> wrote:
> Currently we are leaking addresses from the kernel to user space. This
> script is an attempt to find some of those leakages. Script parses
> `dmesg` output and /proc and /sys files for hex strings that look like
> kernel addresses.

Lovely. This is great. It shows just how much totally pointless stuff
we leak, and to normal users that really shouldn't need it.

I had planned to wait for 4.15 to look at the printk hashing stuff
etc, but this part I think I could/should merge early just because I
think a lot of kernel developers will go "Why the f*ck would we expose
that kernel address there?"

The module sections stuff etc should likely be obviously root-only,
although maybe I'm missing some tool that ends up using it and is
useful to normal developers.

And I'm thinking we could make kallsyms smarter too, and instead of
depending on kptr_restrict that screws over things with much too big a
hammer, we could make it take 'perf_event_paranoid' into account. I
suspect that's the main user of kallsyms that would still be relevant
to non-root.

I really really hate kptr_restrict due to that whole "all or nothing" thing.

The networking code ends up having a ton of "sk" pointers, and I
suspect those would all be fine just using the hashed address, since
nobody is going to care about the _actual_ kernel address, but
matching a socket reference in one file against another one makes tons
of sense and I would not be surprised if there's a lot of netstat-like
tools that do that. So the whole '%pK' thing is entirely useless for
them, but the "hash %p values" should work fine.

Adding netdev and David Miller explicitly to the cc, since many of the
kernel pointer leaks are from them, and they may not even have been
following this discussion at all.

David - you can see the patch on patchwork:

    https://patchwork.kernel.org/patch/10042605/

and try it out yourself.

I heartily recommend other developers run it too, just to see if it
makes you go "Oh, I didn't even realize.."

This looks much more interesting than the whole "%p vs %pK vs %x" discussion.

                  Linus

[kernel-hardening] Re: [PATCH v3] scripts: add leaking_addresses.pl

[Linus Torvalds]

On Sun, Nov 5, 2017 at 9:19 PM, Tobin C. Harding <me@tobin.cc> wrote:
> Currently we are leaking addresses from the kernel to user space. This
> script is an attempt to find some of those leakages. Script parses
> `dmesg` output and /proc and /sys files for hex strings that look like
> kernel addresses.

Lovely. This is great. It shows just how much totally pointless stuff
we leak, and to normal users that really shouldn't need it.

I had planned to wait for 4.15 to look at the printk hashing stuff
etc, but this part I think I could/should merge early just because I
think a lot of kernel developers will go "Why the f*ck would we expose
that kernel address there?"

The module sections stuff etc should likely be obviously root-only,
although maybe I'm missing some tool that ends up using it and is
useful to normal developers.

And I'm thinking we could make kallsyms smarter too, and instead of
depending on kptr_restrict that screws over things with much too big a
hammer, we could make it take 'perf_event_paranoid' into account. I
suspect that's the main user of kallsyms that would still be relevant
to non-root.

I really really hate kptr_restrict due to that whole "all or nothing" thing.

The networking code ends up having a ton of "sk" pointers, and I
suspect those would all be fine just using the hashed address, since
nobody is going to care about the _actual_ kernel address, but
matching a socket reference in one file against another one makes tons
of sense and I would not be surprised if there's a lot of netstat-like
tools that do that. So the whole '%pK' thing is entirely useless for
them, but the "hash %p values" should work fine.

Adding netdev and David Miller explicitly to the cc, since many of the
kernel pointer leaks are from them, and they may not even have been
following this discussion at all.

David - you can see the patch on patchwork:

    https://patchwork.kernel.org/patch/10042605/

and try it out yourself.

I heartily recommend other developers run it too, just to see if it
makes you go "Oh, I didn't even realize.."

This looks much more interesting than the whole "%p vs %pK vs %x" discussion.

                  Linus

Re: [PATCH v3] scripts: add leaking_addresses.pl

[Linus Torvalds]

On Mon, Nov 6, 2017 at 9:27 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Lovely. This is great. It shows just how much totally pointless stuff
> we leak, and to normal users that really shouldn't need it.

Side note: it would be good to have some summary view, and perhaps
some way to limit duplicates.

I ended up running this command line from hell to summarize the
different sources:

    perl leaking_addresses.pl |
            cut -d: -f1 |
            sed 's:/[0-9]*/:/X/:g' |
            sed 's:/module/[^/]*/:/module/X/:g' |
            sort | uniq | less -S

and maybe that kind of duplicate culling could be part of the script
itself if you pass it some summary line.

In particular, if would be nice to have a summary report that

 - only shows the first address for a particular source

 - have some logic to collapse repeated entries of "same file, just
different instance"

my sed-invocations there are obviously very ad-hoc, I'm  not actually
advocating that crap, it's only meant as hacky example of what I'm
talking about. Something smarter would be much better.

Because right now if some developer runs it, they might miss some case
that they should care about, simply because it's hidden among all the
thousands of essentially duplicate cases.

                 Linus

Re: [PATCH v3] scripts: add leaking_addresses.pl

[Linus Torvalds]

On Mon, Nov 6, 2017 at 9:27 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Lovely. This is great. It shows just how much totally pointless stuff
> we leak, and to normal users that really shouldn't need it.

Side note: it would be good to have some summary view, and perhaps
some way to limit duplicates.

I ended up running this command line from hell to summarize the
different sources:

    perl leaking_addresses.pl |
            cut -d: -f1 |
            sed 's:/[0-9]*/:/X/:g' |
            sed 's:/module/[^/]*/:/module/X/:g' |
            sort | uniq | less -S

and maybe that kind of duplicate culling could be part of the script
itself if you pass it some summary line.

In particular, if would be nice to have a summary report that

 - only shows the first address for a particular source

 - have some logic to collapse repeated entries of "same file, just
different instance"

my sed-invocations there are obviously very ad-hoc, I'm  not actually
advocating that crap, it's only meant as hacky example of what I'm
talking about. Something smarter would be much better.

Because right now if some developer runs it, they might miss some case
that they should care about, simply because it's hidden among all the
thousands of essentially duplicate cases.

                 Linus

[kernel-hardening] Re: [PATCH v3] scripts: add leaking_addresses.pl

[Linus Torvalds]

On Mon, Nov 6, 2017 at 9:27 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Lovely. This is great. It shows just how much totally pointless stuff
> we leak, and to normal users that really shouldn't need it.

Side note: it would be good to have some summary view, and perhaps
some way to limit duplicates.

I ended up running this command line from hell to summarize the
different sources:

    perl leaking_addresses.pl |
            cut -d: -f1 |
            sed 's:/[0-9]*/:/X/:g' |
            sed 's:/module/[^/]*/:/module/X/:g' |
            sort | uniq | less -S

and maybe that kind of duplicate culling could be part of the script
itself if you pass it some summary line.

In particular, if would be nice to have a summary report that

 - only shows the first address for a particular source

 - have some logic to collapse repeated entries of "same file, just
different instance"

my sed-invocations there are obviously very ad-hoc, I'm  not actually
advocating that crap, it's only meant as hacky example of what I'm
talking about. Something smarter would be much better.

Because right now if some developer runs it, they might miss some case
that they should care about, simply because it's hidden among all the
thousands of essentially duplicate cases.

                 Linus

Re: [kernel-hardening] Re: [PATCH v3] scripts: add leaking_addresses.pl

[Pavel Vasilyev]

 ./leaking_addresses.pl --dont_walk_abs /proc --dont_walk_abs /sys
Unknown option: dont_walk_abs
Unknown option: dont_walk_abs

06.11.2017 20:27, Linus Torvalds пишет:
> David - you can see the patch on patchwork:
>
>     https://patchwork.kernel.org/patch/10042605/
>
> and try it out yourself.
>

Re: [kernel-hardening] Re: [PATCH v3] scripts: add leaking_addresses.pl

[Pavel Vasilyev]

 ./leaking_addresses.pl --dont_walk_abs /proc --dont_walk_abs /sys
Unknown option: dont_walk_abs
Unknown option: dont_walk_abs

06.11.2017 20:27, Linus Torvalds пишет:
> David - you can see the patch on patchwork:
>
>     https://patchwork.kernel.org/patch/10042605/
>
> and try it out yourself.
>

Re: [kernel-hardening] Re: [PATCH v3] scripts: add leaking_addresses.pl

[Pavel Vasilyev]

 ./leaking_addresses.pl --dont_walk_abs /proc --dont_walk_abs /sys
Unknown option: dont_walk_abs
Unknown option: dont_walk_abs

06.11.2017 20:27, Linus Torvalds пишет:
> David - you can see the patch on patchwork:
>
>     https://patchwork.kernel.org/patch/10042605/
>
> and try it out yourself.
>

Re: [kernel-hardening] Re: [PATCH v3] scripts: add leaking_addresses.pl

[Tobin C. Harding]

On Mon, Nov 06, 2017 at 09:25:33PM +0300, Pavel Vasilyev wrote:
>  ./leaking_addresses.pl --dont_walk_abs /proc --dont_walk_abs /sys
> Unknown option: dont_walk_abs
> Unknown option: dont_walk_abs

Oh thanks. Documentation is out of sync with the code, what are the odds.

v4 to come.

thanks,
Tobin.

Re: [kernel-hardening] Re: [PATCH v3] scripts: add leaking_addresses.pl

[Tobin C. Harding]

On Mon, Nov 06, 2017 at 09:25:33PM +0300, Pavel Vasilyev wrote:
>  ./leaking_addresses.pl --dont_walk_abs /proc --dont_walk_abs /sys
> Unknown option: dont_walk_abs
> Unknown option: dont_walk_abs

Oh thanks. Documentation is out of sync with the code, what are the odds.

v4 to come.

thanks,
Tobin.

Re: [kernel-hardening] Re: [PATCH v3] scripts: add leaking_addresses.pl

[Tobin C. Harding]

On Mon, Nov 06, 2017 at 09:25:33PM +0300, Pavel Vasilyev wrote:
>  ./leaking_addresses.pl --dont_walk_abs /proc --dont_walk_abs /sys
> Unknown option: dont_walk_abs
> Unknown option: dont_walk_abs

Oh thanks. Documentation is out of sync with the code, what are the odds.

v4 to come.

thanks,
Tobin.

Re: [PATCH v3] scripts: add leaking_addresses.pl

[Tobin C. Harding]

On Mon, Nov 06, 2017 at 09:41:09AM -0800, Linus Torvalds wrote:
> On Mon, Nov 6, 2017 at 9:27 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Lovely. This is great. It shows just how much totally pointless stuff
> > we leak, and to normal users that really shouldn't need it.
> 
> Side note: it would be good to have some summary view, and perhaps
> some way to limit duplicates.

This has been bothering me also.

> I ended up running this command line from hell to summarize the
> different sources:
> 
>     perl leaking_addresses.pl |
>             cut -d: -f1 |
>             sed 's:/[0-9]*/:/X/:g' |
>             sed 's:/module/[^/]*/:/module/X/:g' |
>             sort | uniq | less -S
> 
> and maybe that kind of duplicate culling could be part of the script
> itself if you pass it some summary line.
> 
> In particular, if would be nice to have a summary report that
> 
>  - only shows the first address for a particular source
> 
>  - have some logic to collapse repeated entries of "same file, just
> different instance"
> 
> my sed-invocations there are obviously very ad-hoc, I'm  not actually
> advocating that crap, it's only meant as hacky example of what I'm
> talking about. Something smarter would be much better.
> 
> Because right now if some developer runs it, they might miss some case
> that they should care about, simply because it's hidden among all the
> thousands of essentially duplicate cases.

Awesome. I'm on it. thanks.

So, cull duplicates by default, add summary report to end of output, add
'--raw' option to dump all the lines (the current output).

thanks,
Tobin.

Re: [PATCH v3] scripts: add leaking_addresses.pl

[Tobin C. Harding]

On Mon, Nov 06, 2017 at 09:41:09AM -0800, Linus Torvalds wrote:
> On Mon, Nov 6, 2017 at 9:27 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Lovely. This is great. It shows just how much totally pointless stuff
> > we leak, and to normal users that really shouldn't need it.
> 
> Side note: it would be good to have some summary view, and perhaps
> some way to limit duplicates.

This has been bothering me also.

> I ended up running this command line from hell to summarize the
> different sources:
> 
>     perl leaking_addresses.pl |
>             cut -d: -f1 |
>             sed 's:/[0-9]*/:/X/:g' |
>             sed 's:/module/[^/]*/:/module/X/:g' |
>             sort | uniq | less -S
> 
> and maybe that kind of duplicate culling could be part of the script
> itself if you pass it some summary line.
> 
> In particular, if would be nice to have a summary report that
> 
>  - only shows the first address for a particular source
> 
>  - have some logic to collapse repeated entries of "same file, just
> different instance"
> 
> my sed-invocations there are obviously very ad-hoc, I'm  not actually
> advocating that crap, it's only meant as hacky example of what I'm
> talking about. Something smarter would be much better.
> 
> Because right now if some developer runs it, they might miss some case
> that they should care about, simply because it's hidden among all the
> thousands of essentially duplicate cases.

Awesome. I'm on it. thanks.

So, cull duplicates by default, add summary report to end of output, add
'--raw' option to dump all the lines (the current output).

thanks,
Tobin.

[kernel-hardening] Re: [PATCH v3] scripts: add leaking_addresses.pl

[Tobin C. Harding]

On Mon, Nov 06, 2017 at 09:41:09AM -0800, Linus Torvalds wrote:
> On Mon, Nov 6, 2017 at 9:27 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Lovely. This is great. It shows just how much totally pointless stuff
> > we leak, and to normal users that really shouldn't need it.
> 
> Side note: it would be good to have some summary view, and perhaps
> some way to limit duplicates.

This has been bothering me also.

> I ended up running this command line from hell to summarize the
> different sources:
> 
>     perl leaking_addresses.pl |
>             cut -d: -f1 |
>             sed 's:/[0-9]*/:/X/:g' |
>             sed 's:/module/[^/]*/:/module/X/:g' |
>             sort | uniq | less -S
> 
> and maybe that kind of duplicate culling could be part of the script
> itself if you pass it some summary line.
> 
> In particular, if would be nice to have a summary report that
> 
>  - only shows the first address for a particular source
> 
>  - have some logic to collapse repeated entries of "same file, just
> different instance"
> 
> my sed-invocations there are obviously very ad-hoc, I'm  not actually
> advocating that crap, it's only meant as hacky example of what I'm
> talking about. Something smarter would be much better.
> 
> Because right now if some developer runs it, they might miss some case
> that they should care about, simply because it's hidden among all the
> thousands of essentially duplicate cases.

Awesome. I'm on it. thanks.

So, cull duplicates by default, add summary report to end of output, add
'--raw' option to dump all the lines (the current output).

thanks,
Tobin.

Re: [PATCH v3] scripts: add leaking_addresses.pl

[Kees Cook]

On Mon, Nov 6, 2017 at 9:27 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Sun, Nov 5, 2017 at 9:19 PM, Tobin C. Harding <me@tobin.cc> wrote:
>> Currently we are leaking addresses from the kernel to user space. This
>> script is an attempt to find some of those leakages. Script parses
>> `dmesg` output and /proc and /sys files for hex strings that look like
>> kernel addresses.
>
> Lovely. This is great. It shows just how much totally pointless stuff
> we leak, and to normal users that really shouldn't need it.
>
> I had planned to wait for 4.15 to look at the printk hashing stuff
> etc, but this part I think I could/should merge early just because I
> think a lot of kernel developers will go "Why the f*ck would we expose
> that kernel address there?"
>
> The module sections stuff etc should likely be obviously root-only,
> although maybe I'm missing some tool that ends up using it and is
> useful to normal developers.
>
> And I'm thinking we could make kallsyms smarter too, and instead of
> depending on kptr_restrict that screws over things with much too big a
> hammer, we could make it take 'perf_event_paranoid' into account. I
> suspect that's the main user of kallsyms that would still be relevant
> to non-root.

Linus, what do you have in mind for the root-only "yes we really need
the actual address output" exceptions?

For example, right now /sys/kernel/debug/kernel_page_tables
(CONFIG_X86_PTDUMP=y) needs actual address and currently uses %x.

Looking other places that stand out, it seems like
/proc/lockdep_chains and /proc/lockdep (CONFIG_LOCKDEP=y) has a ton of
%p usage. It's unclear to me if a hash is sufficient for meaningful
debugging there?

Seems like these three from dmesg could be removed?

[    0.000000] Base memory trampoline at [ffffa3fc40099000] 99000 size 24576
arch/x86/realmode/init.c

[    0.000000] percpu: Embedded 38 pages/cpu @ffffa4007fc00000 s116944
r8192 d30512 u524288
mm/percpu.c

[    0.456395] software IO TLB [mem 0xbbfdf000-0xbffdf000] (64MB)
mapped at [ffffa3fcfbfdf000-ffffa3fcfffdefff]
lib/swiotlb.c

Tobin, some other feedback on v4...

I find the output hard to parse. Instead of:

[27527 lockdep_chains] [ffffffffb226c628] cgroup_mutex

Could we have:

27527 /proc/lockdep_chains: [ffffffffb226c628] cgroup_mutex

At the very least, getting the full file path is needed or might not
be clear where something lives.

And for my kernels, I needed to exclude usbmon or the script would
hang (perhaps add a read timeout to the script to detect stalling
files?)

diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
index 282c0cc2bdea..a9b729c0a052 100644
--- a/scripts/leaking_addresses.pl
+++ b/scripts/leaking_addresses.pl
@@ -70,6 +70,7 @@ my @skip_walk_dirs_any = ('self',
                          'thread-self',
                          'cwd',
                          'fd',
+                         'usbmon',
                          'stderr',
                          'stdin',
                          'stdout');


-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v3] scripts: add leaking_addresses.pl

[Kees Cook]

On Mon, Nov 6, 2017 at 9:27 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Sun, Nov 5, 2017 at 9:19 PM, Tobin C. Harding <me@tobin.cc> wrote:
>> Currently we are leaking addresses from the kernel to user space. This
>> script is an attempt to find some of those leakages. Script parses
>> `dmesg` output and /proc and /sys files for hex strings that look like
>> kernel addresses.
>
> Lovely. This is great. It shows just how much totally pointless stuff
> we leak, and to normal users that really shouldn't need it.
>
> I had planned to wait for 4.15 to look at the printk hashing stuff
> etc, but this part I think I could/should merge early just because I
> think a lot of kernel developers will go "Why the f*ck would we expose
> that kernel address there?"
>
> The module sections stuff etc should likely be obviously root-only,
> although maybe I'm missing some tool that ends up using it and is
> useful to normal developers.
>
> And I'm thinking we could make kallsyms smarter too, and instead of
> depending on kptr_restrict that screws over things with much too big a
> hammer, we could make it take 'perf_event_paranoid' into account. I
> suspect that's the main user of kallsyms that would still be relevant
> to non-root.

Linus, what do you have in mind for the root-only "yes we really need
the actual address output" exceptions?

For example, right now /sys/kernel/debug/kernel_page_tables
(CONFIG_X86_PTDUMP=y) needs actual address and currently uses %x.

Looking other places that stand out, it seems like
/proc/lockdep_chains and /proc/lockdep (CONFIG_LOCKDEP=y) has a ton of
%p usage. It's unclear to me if a hash is sufficient for meaningful
debugging there?

Seems like these three from dmesg could be removed?

[    0.000000] Base memory trampoline at [ffffa3fc40099000] 99000 size 24576
arch/x86/realmode/init.c

[    0.000000] percpu: Embedded 38 pages/cpu @ffffa4007fc00000 s116944
r8192 d30512 u524288
mm/percpu.c

[    0.456395] software IO TLB [mem 0xbbfdf000-0xbffdf000] (64MB)
mapped at [ffffa3fcfbfdf000-ffffa3fcfffdefff]
lib/swiotlb.c

Tobin, some other feedback on v4...

I find the output hard to parse. Instead of:

[27527 lockdep_chains] [ffffffffb226c628] cgroup_mutex

Could we have:

27527 /proc/lockdep_chains: [ffffffffb226c628] cgroup_mutex

At the very least, getting the full file path is needed or might not
be clear where something lives.

And for my kernels, I needed to exclude usbmon or the script would
hang (perhaps add a read timeout to the script to detect stalling
files?)

diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
index 282c0cc2bdea..a9b729c0a052 100644
--- a/scripts/leaking_addresses.pl
+++ b/scripts/leaking_addresses.pl
@@ -70,6 +70,7 @@ my @skip_walk_dirs_any = ('self',
                          'thread-self',
                          'cwd',
                          'fd',
+                         'usbmon',
                          'stderr',
                          'stdin',
                          'stdout');


-Kees

-- 
Kees Cook
Pixel Security

[kernel-hardening] Re: [PATCH v3] scripts: add leaking_addresses.pl

[Kees Cook]

On Mon, Nov 6, 2017 at 9:27 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Sun, Nov 5, 2017 at 9:19 PM, Tobin C. Harding <me@tobin.cc> wrote:
>> Currently we are leaking addresses from the kernel to user space. This
>> script is an attempt to find some of those leakages. Script parses
>> `dmesg` output and /proc and /sys files for hex strings that look like
>> kernel addresses.
>
> Lovely. This is great. It shows just how much totally pointless stuff
> we leak, and to normal users that really shouldn't need it.
>
> I had planned to wait for 4.15 to look at the printk hashing stuff
> etc, but this part I think I could/should merge early just because I
> think a lot of kernel developers will go "Why the f*ck would we expose
> that kernel address there?"
>
> The module sections stuff etc should likely be obviously root-only,
> although maybe I'm missing some tool that ends up using it and is
> useful to normal developers.
>
> And I'm thinking we could make kallsyms smarter too, and instead of
> depending on kptr_restrict that screws over things with much too big a
> hammer, we could make it take 'perf_event_paranoid' into account. I
> suspect that's the main user of kallsyms that would still be relevant
> to non-root.

Linus, what do you have in mind for the root-only "yes we really need
the actual address output" exceptions?

For example, right now /sys/kernel/debug/kernel_page_tables
(CONFIG_X86_PTDUMP=y) needs actual address and currently uses %x.

Looking other places that stand out, it seems like
/proc/lockdep_chains and /proc/lockdep (CONFIG_LOCKDEP=y) has a ton of
%p usage. It's unclear to me if a hash is sufficient for meaningful
debugging there?

Seems like these three from dmesg could be removed?

[    0.000000] Base memory trampoline at [ffffa3fc40099000] 99000 size 24576
arch/x86/realmode/init.c

[    0.000000] percpu: Embedded 38 pages/cpu @ffffa4007fc00000 s116944
r8192 d30512 u524288
mm/percpu.c

[    0.456395] software IO TLB [mem 0xbbfdf000-0xbffdf000] (64MB)
mapped at [ffffa3fcfbfdf000-ffffa3fcfffdefff]
lib/swiotlb.c

Tobin, some other feedback on v4...

I find the output hard to parse. Instead of:

[27527 lockdep_chains] [ffffffffb226c628] cgroup_mutex

Could we have:

27527 /proc/lockdep_chains: [ffffffffb226c628] cgroup_mutex

At the very least, getting the full file path is needed or might not
be clear where something lives.

And for my kernels, I needed to exclude usbmon or the script would
hang (perhaps add a read timeout to the script to detect stalling
files?)

diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
index 282c0cc2bdea..a9b729c0a052 100644
--- a/scripts/leaking_addresses.pl
+++ b/scripts/leaking_addresses.pl
@@ -70,6 +70,7 @@ my @skip_walk_dirs_any = ('self',
                          'thread-self',
                          'cwd',
                          'fd',
+                         'usbmon',
                          'stderr',
                          'stdin',
                          'stdout');


-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v3] scripts: add leaking_addresses.pl

[Linus Torvalds]

On Tue, Nov 7, 2017 at 1:22 PM, Kees Cook <keescook@chromium.org> wrote:
>
> Linus, what do you have in mind for the root-only "yes we really need
> the actual address output" exceptions?

I am convinced that absolutely none of them should use '%pK'.

So far we have actually never seen a valid case wher %pK was really
the right thing to do.

> For example, right now /sys/kernel/debug/kernel_page_tables
> (CONFIG_X86_PTDUMP=y) needs actual address and currently uses %x.

So I think it could continue to use %x, and just make sure the whole
file is root-only.

And that is why %pK is so wrong. It's almost never really about root.

Look at /proc/kallsyms, for example. There it's mainly about kernel
profiles (although there certainly have been other uses historically,
and maybe some of them remain) - which we have another flag for
entirely that is very much specifically about kernel profiles.

> Looking other places that stand out, it seems like
> /proc/lockdep_chains and /proc/lockdep (CONFIG_LOCKDEP=y) has a ton of
> %p usage. It's unclear to me if a hash is sufficient for meaningful
> debugging there?

Maybe not, but that is also _so_ esoteric that I suspect the right fix
is to just make it root-only readable.

I've never used it, we should check with people who have. I get the
feeling that this is purely for PeterZ debugging.

The very first commit that introduced that code actually has a

    (FIXME: should go into debugfs)

so I suspect it never should have been user-readable to begin with. I
guess it makes some things easier, but it really is *very* different
from things like profiling.

Profiling you often *cannot* do as root - some things you profile
really shouldn't be run as root, and might even refuse to do so. So
requiring you to be root just to get a kernel profile is very bad.

But looking at lockdep stats? Yeah, 'sudo' isn't so big of a deal.

And I really suspect that's true of a _lot_ of these %p things that
really want a pointer. It's not that they really want %pK, it's that
they shouldn't have been visible to regular users in the first place.

Things that *do* want a pointer and should be visible to regular users
are things like oops messages etc, but even there we obviously
generally want to use %pS/%pF when possible (but generally %x when not
- things like register contents etc that *may* contain pointers).

And if they really are visible to users - because you want to
cross-correlate them for things like netstat - I think the hashing is
the right thing to do both for root and for regular users.

> Seems like these three from dmesg could be removed?
>
> [    0.000000] Base memory trampoline at [ffffa3fc40099000] 99000 size 24576
> arch/x86/realmode/init.c
>
> [    0.000000] percpu: Embedded 38 pages/cpu @ffffa4007fc00000 s116944
> r8192 d30512 u524288
> mm/percpu.c
>
> [    0.456395] software IO TLB [mem 0xbbfdf000-0xbffdf000] (64MB)
> mapped at [ffffa3fcfbfdf000-ffffa3fcfffdefff]
> lib/swiotlb.c

Yes, I think the solution for a lot of the random device discovery
messages etc is to just remove them. They were likely useful when the
code was new and untested, and just stayed around afterwards.

                      Linus

Re: [PATCH v3] scripts: add leaking_addresses.pl

[Linus Torvalds]

On Tue, Nov 7, 2017 at 1:22 PM, Kees Cook <keescook@chromium.org> wrote:
>
> Linus, what do you have in mind for the root-only "yes we really need
> the actual address output" exceptions?

I am convinced that absolutely none of them should use '%pK'.

So far we have actually never seen a valid case wher %pK was really
the right thing to do.

> For example, right now /sys/kernel/debug/kernel_page_tables
> (CONFIG_X86_PTDUMP=y) needs actual address and currently uses %x.

So I think it could continue to use %x, and just make sure the whole
file is root-only.

And that is why %pK is so wrong. It's almost never really about root.

Look at /proc/kallsyms, for example. There it's mainly about kernel
profiles (although there certainly have been other uses historically,
and maybe some of them remain) - which we have another flag for
entirely that is very much specifically about kernel profiles.

> Looking other places that stand out, it seems like
> /proc/lockdep_chains and /proc/lockdep (CONFIG_LOCKDEP=y) has a ton of
> %p usage. It's unclear to me if a hash is sufficient for meaningful
> debugging there?

Maybe not, but that is also _so_ esoteric that I suspect the right fix
is to just make it root-only readable.

I've never used it, we should check with people who have. I get the
feeling that this is purely for PeterZ debugging.

The very first commit that introduced that code actually has a

    (FIXME: should go into debugfs)

so I suspect it never should have been user-readable to begin with. I
guess it makes some things easier, but it really is *very* different
from things like profiling.

Profiling you often *cannot* do as root - some things you profile
really shouldn't be run as root, and might even refuse to do so. So
requiring you to be root just to get a kernel profile is very bad.

But looking at lockdep stats? Yeah, 'sudo' isn't so big of a deal.

And I really suspect that's true of a _lot_ of these %p things that
really want a pointer. It's not that they really want %pK, it's that
they shouldn't have been visible to regular users in the first place.

Things that *do* want a pointer and should be visible to regular users
are things like oops messages etc, but even there we obviously
generally want to use %pS/%pF when possible (but generally %x when not
- things like register contents etc that *may* contain pointers).

And if they really are visible to users - because you want to
cross-correlate them for things like netstat - I think the hashing is
the right thing to do both for root and for regular users.

> Seems like these three from dmesg could be removed?
>
> [    0.000000] Base memory trampoline at [ffffa3fc40099000] 99000 size 24576
> arch/x86/realmode/init.c
>
> [    0.000000] percpu: Embedded 38 pages/cpu @ffffa4007fc00000 s116944
> r8192 d30512 u524288
> mm/percpu.c
>
> [    0.456395] software IO TLB [mem 0xbbfdf000-0xbffdf000] (64MB)
> mapped at [ffffa3fcfbfdf000-ffffa3fcfffdefff]
> lib/swiotlb.c

Yes, I think the solution for a lot of the random device discovery
messages etc is to just remove them. They were likely useful when the
code was new and untested, and just stayed around afterwards.

                      Linus

[kernel-hardening] Re: [PATCH v3] scripts: add leaking_addresses.pl

[Linus Torvalds]

On Tue, Nov 7, 2017 at 1:22 PM, Kees Cook <keescook@chromium.org> wrote:
>
> Linus, what do you have in mind for the root-only "yes we really need
> the actual address output" exceptions?

I am convinced that absolutely none of them should use '%pK'.

So far we have actually never seen a valid case wher %pK was really
the right thing to do.

> For example, right now /sys/kernel/debug/kernel_page_tables
> (CONFIG_X86_PTDUMP=y) needs actual address and currently uses %x.

So I think it could continue to use %x, and just make sure the whole
file is root-only.

And that is why %pK is so wrong. It's almost never really about root.

Look at /proc/kallsyms, for example. There it's mainly about kernel
profiles (although there certainly have been other uses historically,
and maybe some of them remain) - which we have another flag for
entirely that is very much specifically about kernel profiles.

> Looking other places that stand out, it seems like
> /proc/lockdep_chains and /proc/lockdep (CONFIG_LOCKDEP=y) has a ton of
> %p usage. It's unclear to me if a hash is sufficient for meaningful
> debugging there?

Maybe not, but that is also _so_ esoteric that I suspect the right fix
is to just make it root-only readable.

I've never used it, we should check with people who have. I get the
feeling that this is purely for PeterZ debugging.

The very first commit that introduced that code actually has a

    (FIXME: should go into debugfs)

so I suspect it never should have been user-readable to begin with. I
guess it makes some things easier, but it really is *very* different
from things like profiling.

Profiling you often *cannot* do as root - some things you profile
really shouldn't be run as root, and might even refuse to do so. So
requiring you to be root just to get a kernel profile is very bad.

But looking at lockdep stats? Yeah, 'sudo' isn't so big of a deal.

And I really suspect that's true of a _lot_ of these %p things that
really want a pointer. It's not that they really want %pK, it's that
they shouldn't have been visible to regular users in the first place.

Things that *do* want a pointer and should be visible to regular users
are things like oops messages etc, but even there we obviously
generally want to use %pS/%pF when possible (but generally %x when not
- things like register contents etc that *may* contain pointers).

And if they really are visible to users - because you want to
cross-correlate them for things like netstat - I think the hashing is
the right thing to do both for root and for regular users.

> Seems like these three from dmesg could be removed?
>
> [    0.000000] Base memory trampoline at [ffffa3fc40099000] 99000 size 24576
> arch/x86/realmode/init.c
>
> [    0.000000] percpu: Embedded 38 pages/cpu @ffffa4007fc00000 s116944
> r8192 d30512 u524288
> mm/percpu.c
>
> [    0.456395] software IO TLB [mem 0xbbfdf000-0xbffdf000] (64MB)
> mapped at [ffffa3fcfbfdf000-ffffa3fcfffdefff]
> lib/swiotlb.c

Yes, I think the solution for a lot of the random device discovery
messages etc is to just remove them. They were likely useful when the
code was new and untested, and just stayed around afterwards.

                      Linus

Re: [PATCH v3] scripts: add leaking_addresses.pl

[Kees Cook]

On Tue, Nov 7, 2017 at 1:44 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Nov 7, 2017 at 1:22 PM, Kees Cook <keescook@chromium.org> wrote:
>>
>> Linus, what do you have in mind for the root-only "yes we really need
>> the actual address output" exceptions?
>
> I am convinced that absolutely none of them should use '%pK'.
>
> So far we have actually never seen a valid case wher %pK was really
> the right thing to do.

One case to keep in mind (likely for the future, once we stabilize
this current plan), is to consider the situation of "untrusted root
users", in the case of locked down systems where modules are disabled,
etc, etc. In those cases, there's a brighter line between kernel
memory and uid 0. (And when we do start looking at that class of info
leaks, we'll be faced with a possible mess from %x-but-root-only
usage.)

>> For example, right now /sys/kernel/debug/kernel_page_tables
>> (CONFIG_X86_PTDUMP=y) needs actual address and currently uses %x.
>
> So I think it could continue to use %x, and just make sure the whole
> file is root-only.

Yup, sounds good.

Looks like the emerging rules for displaying virtual addresses are:

- don't use %p
- especially don't use %p in dmesg
- really, go use %pS/%pF
- if you absolutely need an address, show it with %x and have the file
be root-only

What should the guidance be for physical addresses? They're sensitive
too (especially since they're reachable via the physmap), but we've
traditionally been leaving them in dmesg, etc.

>> Looking other places that stand out, it seems like
>> /proc/lockdep_chains and /proc/lockdep (CONFIG_LOCKDEP=y) has a ton of
>> %p usage. It's unclear to me if a hash is sufficient for meaningful
>> debugging there?
>
> Maybe not, but that is also _so_ esoteric that I suspect the right fix
> is to just make it root-only readable.

Anyone running a "real" system with LOCKDEP is out of their mind. :)

> I've never used it, we should check with people who have. I get the
> feeling that this is purely for PeterZ debugging.
>
> The very first commit that introduced that code actually has a
>
>     (FIXME: should go into debugfs)

Peter, Paul, what do you think about relocating these files?
(/proc/lockdep* into debugfs)

> so I suspect it never should have been user-readable to begin with. I
> guess it makes some things easier, but it really is *very* different
> from things like profiling.

I didn't make that clear in the original report: I was running this as
root just to see what even the root leaks looked like (it was
surprisingly small, IMO). Those files are already root-only.

>> Seems like these three from dmesg could be removed?
>>
>> [    0.000000] Base memory trampoline at [ffffa3fc40099000] 99000 size 24576
>> arch/x86/realmode/init.c
>>
>> [    0.000000] percpu: Embedded 38 pages/cpu @ffffa4007fc00000 s116944
>> r8192 d30512 u524288
>> mm/percpu.c
>>
>> [    0.456395] software IO TLB [mem 0xbbfdf000-0xbffdf000] (64MB)
>> mapped at [ffffa3fcfbfdf000-ffffa3fcfffdefff]
>> lib/swiotlb.c
>
> Yes, I think the solution for a lot of the random device discovery
> messages etc is to just remove them. They were likely useful when the
> code was new and untested, and just stayed around afterwards.

Yup, sounds right. (Though my physical address question from above
remains: should these also drop their physical address reports?)

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v3] scripts: add leaking_addresses.pl

[Kees Cook]

On Tue, Nov 7, 2017 at 1:44 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Nov 7, 2017 at 1:22 PM, Kees Cook <keescook@chromium.org> wrote:
>>
>> Linus, what do you have in mind for the root-only "yes we really need
>> the actual address output" exceptions?
>
> I am convinced that absolutely none of them should use '%pK'.
>
> So far we have actually never seen a valid case wher %pK was really
> the right thing to do.

One case to keep in mind (likely for the future, once we stabilize
this current plan), is to consider the situation of "untrusted root
users", in the case of locked down systems where modules are disabled,
etc, etc. In those cases, there's a brighter line between kernel
memory and uid 0. (And when we do start looking at that class of info
leaks, we'll be faced with a possible mess from %x-but-root-only
usage.)

>> For example, right now /sys/kernel/debug/kernel_page_tables
>> (CONFIG_X86_PTDUMP=y) needs actual address and currently uses %x.
>
> So I think it could continue to use %x, and just make sure the whole
> file is root-only.

Yup, sounds good.

Looks like the emerging rules for displaying virtual addresses are:

- don't use %p
- especially don't use %p in dmesg
- really, go use %pS/%pF
- if you absolutely need an address, show it with %x and have the file
be root-only

What should the guidance be for physical addresses? They're sensitive
too (especially since they're reachable via the physmap), but we've
traditionally been leaving them in dmesg, etc.

>> Looking other places that stand out, it seems like
>> /proc/lockdep_chains and /proc/lockdep (CONFIG_LOCKDEP=y) has a ton of
>> %p usage. It's unclear to me if a hash is sufficient for meaningful
>> debugging there?
>
> Maybe not, but that is also _so_ esoteric that I suspect the right fix
> is to just make it root-only readable.

Anyone running a "real" system with LOCKDEP is out of their mind. :)

> I've never used it, we should check with people who have. I get the
> feeling that this is purely for PeterZ debugging.
>
> The very first commit that introduced that code actually has a
>
>     (FIXME: should go into debugfs)

Peter, Paul, what do you think about relocating these files?
(/proc/lockdep* into debugfs)

> so I suspect it never should have been user-readable to begin with. I
> guess it makes some things easier, but it really is *very* different
> from things like profiling.

I didn't make that clear in the original report: I was running this as
root just to see what even the root leaks looked like (it was
surprisingly small, IMO). Those files are already root-only.

>> Seems like these three from dmesg could be removed?
>>
>> [    0.000000] Base memory trampoline at [ffffa3fc40099000] 99000 size 24576
>> arch/x86/realmode/init.c
>>
>> [    0.000000] percpu: Embedded 38 pages/cpu @ffffa4007fc00000 s116944
>> r8192 d30512 u524288
>> mm/percpu.c
>>
>> [    0.456395] software IO TLB [mem 0xbbfdf000-0xbffdf000] (64MB)
>> mapped at [ffffa3fcfbfdf000-ffffa3fcfffdefff]
>> lib/swiotlb.c
>
> Yes, I think the solution for a lot of the random device discovery
> messages etc is to just remove them. They were likely useful when the
> code was new and untested, and just stayed around afterwards.

Yup, sounds right. (Though my physical address question from above
remains: should these also drop their physical address reports?)

-Kees

-- 
Kees Cook
Pixel Security

[kernel-hardening] Re: [PATCH v3] scripts: add leaking_addresses.pl

[Kees Cook]

On Tue, Nov 7, 2017 at 1:44 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Nov 7, 2017 at 1:22 PM, Kees Cook <keescook@chromium.org> wrote:
>>
>> Linus, what do you have in mind for the root-only "yes we really need
>> the actual address output" exceptions?
>
> I am convinced that absolutely none of them should use '%pK'.
>
> So far we have actually never seen a valid case wher %pK was really
> the right thing to do.

One case to keep in mind (likely for the future, once we stabilize
this current plan), is to consider the situation of "untrusted root
users", in the case of locked down systems where modules are disabled,
etc, etc. In those cases, there's a brighter line between kernel
memory and uid 0. (And when we do start looking at that class of info
leaks, we'll be faced with a possible mess from %x-but-root-only
usage.)

>> For example, right now /sys/kernel/debug/kernel_page_tables
>> (CONFIG_X86_PTDUMP=y) needs actual address and currently uses %x.
>
> So I think it could continue to use %x, and just make sure the whole
> file is root-only.

Yup, sounds good.

Looks like the emerging rules for displaying virtual addresses are:

- don't use %p
- especially don't use %p in dmesg
- really, go use %pS/%pF
- if you absolutely need an address, show it with %x and have the file
be root-only

What should the guidance be for physical addresses? They're sensitive
too (especially since they're reachable via the physmap), but we've
traditionally been leaving them in dmesg, etc.

>> Looking other places that stand out, it seems like
>> /proc/lockdep_chains and /proc/lockdep (CONFIG_LOCKDEP=y) has a ton of
>> %p usage. It's unclear to me if a hash is sufficient for meaningful
>> debugging there?
>
> Maybe not, but that is also _so_ esoteric that I suspect the right fix
> is to just make it root-only readable.

Anyone running a "real" system with LOCKDEP is out of their mind. :)

> I've never used it, we should check with people who have. I get the
> feeling that this is purely for PeterZ debugging.
>
> The very first commit that introduced that code actually has a
>
>     (FIXME: should go into debugfs)

Peter, Paul, what do you think about relocating these files?
(/proc/lockdep* into debugfs)

> so I suspect it never should have been user-readable to begin with. I
> guess it makes some things easier, but it really is *very* different
> from things like profiling.

I didn't make that clear in the original report: I was running this as
root just to see what even the root leaks looked like (it was
surprisingly small, IMO). Those files are already root-only.

>> Seems like these three from dmesg could be removed?
>>
>> [    0.000000] Base memory trampoline at [ffffa3fc40099000] 99000 size 24576
>> arch/x86/realmode/init.c
>>
>> [    0.000000] percpu: Embedded 38 pages/cpu @ffffa4007fc00000 s116944
>> r8192 d30512 u524288
>> mm/percpu.c
>>
>> [    0.456395] software IO TLB [mem 0xbbfdf000-0xbffdf000] (64MB)
>> mapped at [ffffa3fcfbfdf000-ffffa3fcfffdefff]
>> lib/swiotlb.c
>
> Yes, I think the solution for a lot of the random device discovery
> messages etc is to just remove them. They were likely useful when the
> code was new and untested, and just stayed around afterwards.

Yup, sounds right. (Though my physical address question from above
remains: should these also drop their physical address reports?)

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v3] scripts: add leaking_addresses.pl

[Steven Rostedt]

On Tue, 7 Nov 2017 13:44:01 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> > Looking other places that stand out, it seems like
> > /proc/lockdep_chains and /proc/lockdep (CONFIG_LOCKDEP=y) has a ton of
> > %p usage. It's unclear to me if a hash is sufficient for meaningful
> > debugging there?  
> 
> Maybe not, but that is also _so_ esoteric that I suspect the right fix
> is to just make it root-only readable.

Also note, I don't believe anyone should be running a LOCKDEP
configured kernel in a production (secured) environment. As it adds
quite a bit of overhead. It's something you run on test environments to
make sure it doesn't detect any possible deadlocks.

> 
> I've never used it, we should check with people who have. I get the
> feeling that this is purely for PeterZ debugging.

I've used it. But then again, I also debug lockdep ;-)

> 
> The very first commit that introduced that code actually has a
> 
>     (FIXME: should go into debugfs)
> 
> so I suspect it never should have been user-readable to begin with. I
> guess it makes some things easier, but it really is *very* different
> from things like profiling.

Want me to whip up a patch to move the file?

-- Steve

> 
> Profiling you often *cannot* do as root - some things you profile
> really shouldn't be run as root, and might even refuse to do so. So
> requiring you to be root just to get a kernel profile is very bad.
> 
> But looking at lockdep stats? Yeah, 'sudo' isn't so big of a deal.
> 
> 

Re: [PATCH v3] scripts: add leaking_addresses.pl

[Steven Rostedt]

On Tue, 7 Nov 2017 13:44:01 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> > Looking other places that stand out, it seems like
> > /proc/lockdep_chains and /proc/lockdep (CONFIG_LOCKDEP=y) has a ton of
> > %p usage. It's unclear to me if a hash is sufficient for meaningful
> > debugging there?  
> 
> Maybe not, but that is also _so_ esoteric that I suspect the right fix
> is to just make it root-only readable.

Also note, I don't believe anyone should be running a LOCKDEP
configured kernel in a production (secured) environment. As it adds
quite a bit of overhead. It's something you run on test environments to
make sure it doesn't detect any possible deadlocks.

> 
> I've never used it, we should check with people who have. I get the
> feeling that this is purely for PeterZ debugging.

I've used it. But then again, I also debug lockdep ;-)

> 
> The very first commit that introduced that code actually has a
> 
>     (FIXME: should go into debugfs)
> 
> so I suspect it never should have been user-readable to begin with. I
> guess it makes some things easier, but it really is *very* different
> from things like profiling.

Want me to whip up a patch to move the file?

-- Steve

> 
> Profiling you often *cannot* do as root - some things you profile
> really shouldn't be run as root, and might even refuse to do so. So
> requiring you to be root just to get a kernel profile is very bad.
> 
> But looking at lockdep stats? Yeah, 'sudo' isn't so big of a deal.
> 
> 

[kernel-hardening] Re: [PATCH v3] scripts: add leaking_addresses.pl

[Steven Rostedt]

On Tue, 7 Nov 2017 13:44:01 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> > Looking other places that stand out, it seems like
> > /proc/lockdep_chains and /proc/lockdep (CONFIG_LOCKDEP=y) has a ton of
> > %p usage. It's unclear to me if a hash is sufficient for meaningful
> > debugging there?  
> 
> Maybe not, but that is also _so_ esoteric that I suspect the right fix
> is to just make it root-only readable.

Also note, I don't believe anyone should be running a LOCKDEP
configured kernel in a production (secured) environment. As it adds
quite a bit of overhead. It's something you run on test environments to
make sure it doesn't detect any possible deadlocks.

> 
> I've never used it, we should check with people who have. I get the
> feeling that this is purely for PeterZ debugging.

I've used it. But then again, I also debug lockdep ;-)

> 
> The very first commit that introduced that code actually has a
> 
>     (FIXME: should go into debugfs)
> 
> so I suspect it never should have been user-readable to begin with. I
> guess it makes some things easier, but it really is *very* different
> from things like profiling.

Want me to whip up a patch to move the file?

-- Steve

> 
> Profiling you often *cannot* do as root - some things you profile
> really shouldn't be run as root, and might even refuse to do so. So
> requiring you to be root just to get a kernel profile is very bad.
> 
> But looking at lockdep stats? Yeah, 'sudo' isn't so big of a deal.
> 
> 

Re: [PATCH v3] scripts: add leaking_addresses.pl

[Tobin C. Harding]

Hi Kees,

It seems I over looked your suggestions when submitting v4. My mistake.

On Tue, Nov 07, 2017 at 01:22:13PM -0800, Kees Cook wrote:
> On Mon, Nov 6, 2017 at 9:27 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > On Sun, Nov 5, 2017 at 9:19 PM, Tobin C. Harding <me@tobin.cc> wrote:
> >> Currently we are leaking addresses from the kernel to user space. This
> >> script is an attempt to find some of those leakages. Script parses
> >> `dmesg` output and /proc and /sys files for hex strings that look like
> >> kernel addresses.
> >
> > Lovely. This is great. It shows just how much totally pointless stuff
> > we leak, and to normal users that really shouldn't need it.
> >
> > I had planned to wait for 4.15 to look at the printk hashing stuff
> > etc, but this part I think I could/should merge early just because I
> > think a lot of kernel developers will go "Why the f*ck would we expose
> > that kernel address there?"
> >
> > The module sections stuff etc should likely be obviously root-only,
> > although maybe I'm missing some tool that ends up using it and is
> > useful to normal developers.
> >
> > And I'm thinking we could make kallsyms smarter too, and instead of
> > depending on kptr_restrict that screws over things with much too big a
> > hammer, we could make it take 'perf_event_paranoid' into account. I
> > suspect that's the main user of kallsyms that would still be relevant
> > to non-root.
> 
> Linus, what do you have in mind for the root-only "yes we really need
> the actual address output" exceptions?
> 
> For example, right now /sys/kernel/debug/kernel_page_tables
> (CONFIG_X86_PTDUMP=y) needs actual address and currently uses %x.
> 
> Looking other places that stand out, it seems like
> /proc/lockdep_chains and /proc/lockdep (CONFIG_LOCKDEP=y) has a ton of
> %p usage. It's unclear to me if a hash is sufficient for meaningful
> debugging there?
> 
> Seems like these three from dmesg could be removed?
> 
> [    0.000000] Base memory trampoline at [ffffa3fc40099000] 99000 size 24576
> arch/x86/realmode/init.c
> 
> [    0.000000] percpu: Embedded 38 pages/cpu @ffffa4007fc00000 s116944
> r8192 d30512 u524288
> mm/percpu.c
> 
> [    0.456395] software IO TLB [mem 0xbbfdf000-0xbffdf000] (64MB)
> mapped at [ffffa3fcfbfdf000-ffffa3fcfffdefff]
> lib/swiotlb.c
> 
> Tobin, some other feedback on v4...
> 
> I find the output hard to parse. Instead of:
> 
> [27527 lockdep_chains] [ffffffffb226c628] cgroup_mutex
> 
> Could we have:
> 
> 27527 /proc/lockdep_chains: [ffffffffb226c628] cgroup_mutex

This is what I had during development, it becomes had to parse when the
message contains ':' and also if the address is not contained in braces
(I'm assuming '[ffffffffb226c628] cgroup_mutex' is the message).

We could use your suggested format but replace the ':' character?

> At the very least, getting the full file path is needed or might not
> be clear where something lives.

Current dev version includes this.

> And for my kernels, I needed to exclude usbmon or the script would
> hang (perhaps add a read timeout to the script to detect stalling
> files?)

Good idea, I'll add this.

> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> index 282c0cc2bdea..a9b729c0a052 100644
> --- a/scripts/leaking_addresses.pl
> +++ b/scripts/leaking_addresses.pl
> @@ -70,6 +70,7 @@ my @skip_walk_dirs_any = ('self',
>                           'thread-self',
>                           'cwd',
>                           'fd',
> +                         'usbmon',
>                           'stderr',
>                           'stdin',
>                           'stdout');
> 

Added this.

thanks. Again, sorry for missing this before v4.

Tobin

Re: [PATCH v3] scripts: add leaking_addresses.pl

[Tobin C. Harding]

Hi Kees,

It seems I over looked your suggestions when submitting v4. My mistake.

On Tue, Nov 07, 2017 at 01:22:13PM -0800, Kees Cook wrote:
> On Mon, Nov 6, 2017 at 9:27 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > On Sun, Nov 5, 2017 at 9:19 PM, Tobin C. Harding <me@tobin.cc> wrote:
> >> Currently we are leaking addresses from the kernel to user space. This
> >> script is an attempt to find some of those leakages. Script parses
> >> `dmesg` output and /proc and /sys files for hex strings that look like
> >> kernel addresses.
> >
> > Lovely. This is great. It shows just how much totally pointless stuff
> > we leak, and to normal users that really shouldn't need it.
> >
> > I had planned to wait for 4.15 to look at the printk hashing stuff
> > etc, but this part I think I could/should merge early just because I
> > think a lot of kernel developers will go "Why the f*ck would we expose
> > that kernel address there?"
> >
> > The module sections stuff etc should likely be obviously root-only,
> > although maybe I'm missing some tool that ends up using it and is
> > useful to normal developers.
> >
> > And I'm thinking we could make kallsyms smarter too, and instead of
> > depending on kptr_restrict that screws over things with much too big a
> > hammer, we could make it take 'perf_event_paranoid' into account. I
> > suspect that's the main user of kallsyms that would still be relevant
> > to non-root.
> 
> Linus, what do you have in mind for the root-only "yes we really need
> the actual address output" exceptions?
> 
> For example, right now /sys/kernel/debug/kernel_page_tables
> (CONFIG_X86_PTDUMP=y) needs actual address and currently uses %x.
> 
> Looking other places that stand out, it seems like
> /proc/lockdep_chains and /proc/lockdep (CONFIG_LOCKDEP=y) has a ton of
> %p usage. It's unclear to me if a hash is sufficient for meaningful
> debugging there?
> 
> Seems like these three from dmesg could be removed?
> 
> [    0.000000] Base memory trampoline at [ffffa3fc40099000] 99000 size 24576
> arch/x86/realmode/init.c
> 
> [    0.000000] percpu: Embedded 38 pages/cpu @ffffa4007fc00000 s116944
> r8192 d30512 u524288
> mm/percpu.c
> 
> [    0.456395] software IO TLB [mem 0xbbfdf000-0xbffdf000] (64MB)
> mapped at [ffffa3fcfbfdf000-ffffa3fcfffdefff]
> lib/swiotlb.c
> 
> Tobin, some other feedback on v4...
> 
> I find the output hard to parse. Instead of:
> 
> [27527 lockdep_chains] [ffffffffb226c628] cgroup_mutex
> 
> Could we have:
> 
> 27527 /proc/lockdep_chains: [ffffffffb226c628] cgroup_mutex

This is what I had during development, it becomes had to parse when the
message contains ':' and also if the address is not contained in braces
(I'm assuming '[ffffffffb226c628] cgroup_mutex' is the message).

We could use your suggested format but replace the ':' character?

> At the very least, getting the full file path is needed or might not
> be clear where something lives.

Current dev version includes this.

> And for my kernels, I needed to exclude usbmon or the script would
> hang (perhaps add a read timeout to the script to detect stalling
> files?)

Good idea, I'll add this.

> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> index 282c0cc2bdea..a9b729c0a052 100644
> --- a/scripts/leaking_addresses.pl
> +++ b/scripts/leaking_addresses.pl
> @@ -70,6 +70,7 @@ my @skip_walk_dirs_any = ('self',
>                           'thread-self',
>                           'cwd',
>                           'fd',
> +                         'usbmon',
>                           'stderr',
>                           'stdin',
>                           'stdout');
> 

Added this.

thanks. Again, sorry for missing this before v4.

Tobin

[kernel-hardening] Re: [PATCH v3] scripts: add leaking_addresses.pl

[Tobin C. Harding]

Hi Kees,

It seems I over looked your suggestions when submitting v4. My mistake.

On Tue, Nov 07, 2017 at 01:22:13PM -0800, Kees Cook wrote:
> On Mon, Nov 6, 2017 at 9:27 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > On Sun, Nov 5, 2017 at 9:19 PM, Tobin C. Harding <me@tobin.cc> wrote:
> >> Currently we are leaking addresses from the kernel to user space. This
> >> script is an attempt to find some of those leakages. Script parses
> >> `dmesg` output and /proc and /sys files for hex strings that look like
> >> kernel addresses.
> >
> > Lovely. This is great. It shows just how much totally pointless stuff
> > we leak, and to normal users that really shouldn't need it.
> >
> > I had planned to wait for 4.15 to look at the printk hashing stuff
> > etc, but this part I think I could/should merge early just because I
> > think a lot of kernel developers will go "Why the f*ck would we expose
> > that kernel address there?"
> >
> > The module sections stuff etc should likely be obviously root-only,
> > although maybe I'm missing some tool that ends up using it and is
> > useful to normal developers.
> >
> > And I'm thinking we could make kallsyms smarter too, and instead of
> > depending on kptr_restrict that screws over things with much too big a
> > hammer, we could make it take 'perf_event_paranoid' into account. I
> > suspect that's the main user of kallsyms that would still be relevant
> > to non-root.
> 
> Linus, what do you have in mind for the root-only "yes we really need
> the actual address output" exceptions?
> 
> For example, right now /sys/kernel/debug/kernel_page_tables
> (CONFIG_X86_PTDUMP=y) needs actual address and currently uses %x.
> 
> Looking other places that stand out, it seems like
> /proc/lockdep_chains and /proc/lockdep (CONFIG_LOCKDEP=y) has a ton of
> %p usage. It's unclear to me if a hash is sufficient for meaningful
> debugging there?
> 
> Seems like these three from dmesg could be removed?
> 
> [    0.000000] Base memory trampoline at [ffffa3fc40099000] 99000 size 24576
> arch/x86/realmode/init.c
> 
> [    0.000000] percpu: Embedded 38 pages/cpu @ffffa4007fc00000 s116944
> r8192 d30512 u524288
> mm/percpu.c
> 
> [    0.456395] software IO TLB [mem 0xbbfdf000-0xbffdf000] (64MB)
> mapped at [ffffa3fcfbfdf000-ffffa3fcfffdefff]
> lib/swiotlb.c
> 
> Tobin, some other feedback on v4...
> 
> I find the output hard to parse. Instead of:
> 
> [27527 lockdep_chains] [ffffffffb226c628] cgroup_mutex
> 
> Could we have:
> 
> 27527 /proc/lockdep_chains: [ffffffffb226c628] cgroup_mutex

This is what I had during development, it becomes had to parse when the
message contains ':' and also if the address is not contained in braces
(I'm assuming '[ffffffffb226c628] cgroup_mutex' is the message).

We could use your suggested format but replace the ':' character?

> At the very least, getting the full file path is needed or might not
> be clear where something lives.

Current dev version includes this.

> And for my kernels, I needed to exclude usbmon or the script would
> hang (perhaps add a read timeout to the script to detect stalling
> files?)

Good idea, I'll add this.

> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> index 282c0cc2bdea..a9b729c0a052 100644
> --- a/scripts/leaking_addresses.pl
> +++ b/scripts/leaking_addresses.pl
> @@ -70,6 +70,7 @@ my @skip_walk_dirs_any = ('self',
>                           'thread-self',
>                           'cwd',
>                           'fd',
> +                         'usbmon',
>                           'stderr',
>                           'stdin',
>                           'stdout');
> 

Added this.

thanks. Again, sorry for missing this before v4.

Tobin

Re: [PATCH v3] scripts: add leaking_addresses.pl

[Tobin C. Harding]

On Tue, Nov 07, 2017 at 01:44:01PM -0800, Linus Torvalds wrote:
> On Tue, Nov 7, 2017 at 1:22 PM, Kees Cook <keescook@chromium.org> wrote:
> >
> > Linus, what do you have in mind for the root-only "yes we really need
> > the actual address output" exceptions?
> 
> I am convinced that absolutely none of them should use '%pK'.
> 
> So far we have actually never seen a valid case wher %pK was really
> the right thing to do.
> 
> > For example, right now /sys/kernel/debug/kernel_page_tables
> > (CONFIG_X86_PTDUMP=y) needs actual address and currently uses %x.
> 
> So I think it could continue to use %x, and just make sure the whole
> file is root-only.
> 
> And that is why %pK is so wrong. It's almost never really about root.
> 
> Look at /proc/kallsyms, for example. There it's mainly about kernel
> profiles (although there certainly have been other uses historically,
> and maybe some of them remain) - which we have another flag for
> entirely that is very much specifically about kernel profiles.
> 
> > Looking other places that stand out, it seems like
> > /proc/lockdep_chains and /proc/lockdep (CONFIG_LOCKDEP=y) has a ton of
> > %p usage. It's unclear to me if a hash is sufficient for meaningful
> > debugging there?
> 
> Maybe not, but that is also _so_ esoteric that I suspect the right fix
> is to just make it root-only readable.
> 
> I've never used it, we should check with people who have. I get the
> feeling that this is purely for PeterZ debugging.
> 
> The very first commit that introduced that code actually has a
> 
>     (FIXME: should go into debugfs)
> 
> so I suspect it never should have been user-readable to begin with. I
> guess it makes some things easier, but it really is *very* different
> from things like profiling.
> 
> Profiling you often *cannot* do as root - some things you profile
> really shouldn't be run as root, and might even refuse to do so. So
> requiring you to be root just to get a kernel profile is very bad.
> 
> But looking at lockdep stats? Yeah, 'sudo' isn't so big of a deal.
> 
> And I really suspect that's true of a _lot_ of these %p things that
> really want a pointer. It's not that they really want %pK, it's that
> they shouldn't have been visible to regular users in the first place.
> 
> Things that *do* want a pointer and should be visible to regular users
> are things like oops messages etc, but even there we obviously
> generally want to use %pS/%pF when possible (but generally %x when not
> - things like register contents etc that *may* contain pointers).

This is opt-in, it means asking developers to do the right thing every time
they think they need a pointer. If we hash %p as soon as someone has
been bitten by it they will start using %x and sooner or later they will
use %x exclusively (suggesting using %x for _really_ necessary addresses
will only hasten the process).

If the only real benefit of hashing %p is to clean up kruft why don't we
add %pX and do tree wide substitution of all %p to %pX. People that get
broken will change it back to %p and we will have half a chance of
finding new abusers of %p down the track.

If there is not a 'one size fits all' solution, as we have seen with
kptr_restrict, then should we not be trying to make it easier for people
to do the right thing _and_ easier for us to catch it when we do the
wrong thing?

There is already almost 30 000 users of %{x,X}, surely we don't want to
promote more kernel address printing to be mixed in with all of that?

> And if they really are visible to users - because you want to
> cross-correlate them for things like netstat - I think the hashing is
> the right thing to do both for root and for regular users.
> 
> > Seems like these three from dmesg could be removed?
> >
> > [    0.000000] Base memory trampoline at [ffffa3fc40099000] 99000 size 24576
> > arch/x86/realmode/init.c
> >
> > [    0.000000] percpu: Embedded 38 pages/cpu @ffffa4007fc00000 s116944
> > r8192 d30512 u524288
> > mm/percpu.c
> >
> > [    0.456395] software IO TLB [mem 0xbbfdf000-0xbffdf000] (64MB)
> > mapped at [ffffa3fcfbfdf000-ffffa3fcfffdefff]
> > lib/swiotlb.c
> 
> Yes, I think the solution for a lot of the random device discovery
> messages etc is to just remove them. They were likely useful when the
> code was new and untested, and just stayed around afterwards.

Is anyone actually going to do this? If the hashing gets done then these
messages are not a risk, are _real_ kernel developers going to bother
cleaning this up? Surely newbies are not going to get much love either
if they submit patches that '[PATCH] remove unnecessary printk message'.

thanks,
Tobin.

Re: [PATCH v3] scripts: add leaking_addresses.pl

[Tobin C. Harding]

On Tue, Nov 07, 2017 at 01:44:01PM -0800, Linus Torvalds wrote:
> On Tue, Nov 7, 2017 at 1:22 PM, Kees Cook <keescook@chromium.org> wrote:
> >
> > Linus, what do you have in mind for the root-only "yes we really need
> > the actual address output" exceptions?
> 
> I am convinced that absolutely none of them should use '%pK'.
> 
> So far we have actually never seen a valid case wher %pK was really
> the right thing to do.
> 
> > For example, right now /sys/kernel/debug/kernel_page_tables
> > (CONFIG_X86_PTDUMP=y) needs actual address and currently uses %x.
> 
> So I think it could continue to use %x, and just make sure the whole
> file is root-only.
> 
> And that is why %pK is so wrong. It's almost never really about root.
> 
> Look at /proc/kallsyms, for example. There it's mainly about kernel
> profiles (although there certainly have been other uses historically,
> and maybe some of them remain) - which we have another flag for
> entirely that is very much specifically about kernel profiles.
> 
> > Looking other places that stand out, it seems like
> > /proc/lockdep_chains and /proc/lockdep (CONFIG_LOCKDEP=y) has a ton of
> > %p usage. It's unclear to me if a hash is sufficient for meaningful
> > debugging there?
> 
> Maybe not, but that is also _so_ esoteric that I suspect the right fix
> is to just make it root-only readable.
> 
> I've never used it, we should check with people who have. I get the
> feeling that this is purely for PeterZ debugging.
> 
> The very first commit that introduced that code actually has a
> 
>     (FIXME: should go into debugfs)
> 
> so I suspect it never should have been user-readable to begin with. I
> guess it makes some things easier, but it really is *very* different
> from things like profiling.
> 
> Profiling you often *cannot* do as root - some things you profile
> really shouldn't be run as root, and might even refuse to do so. So
> requiring you to be root just to get a kernel profile is very bad.
> 
> But looking at lockdep stats? Yeah, 'sudo' isn't so big of a deal.
> 
> And I really suspect that's true of a _lot_ of these %p things that
> really want a pointer. It's not that they really want %pK, it's that
> they shouldn't have been visible to regular users in the first place.
> 
> Things that *do* want a pointer and should be visible to regular users
> are things like oops messages etc, but even there we obviously
> generally want to use %pS/%pF when possible (but generally %x when not
> - things like register contents etc that *may* contain pointers).

This is opt-in, it means asking developers to do the right thing every time
they think they need a pointer. If we hash %p as soon as someone has
been bitten by it they will start using %x and sooner or later they will
use %x exclusively (suggesting using %x for _really_ necessary addresses
will only hasten the process).

If the only real benefit of hashing %p is to clean up kruft why don't we
add %pX and do tree wide substitution of all %p to %pX. People that get
broken will change it back to %p and we will have half a chance of
finding new abusers of %p down the track.

If there is not a 'one size fits all' solution, as we have seen with
kptr_restrict, then should we not be trying to make it easier for people
to do the right thing _and_ easier for us to catch it when we do the
wrong thing?

There is already almost 30 000 users of %{x,X}, surely we don't want to
promote more kernel address printing to be mixed in with all of that?

> And if they really are visible to users - because you want to
> cross-correlate them for things like netstat - I think the hashing is
> the right thing to do both for root and for regular users.
> 
> > Seems like these three from dmesg could be removed?
> >
> > [    0.000000] Base memory trampoline at [ffffa3fc40099000] 99000 size 24576
> > arch/x86/realmode/init.c
> >
> > [    0.000000] percpu: Embedded 38 pages/cpu @ffffa4007fc00000 s116944
> > r8192 d30512 u524288
> > mm/percpu.c
> >
> > [    0.456395] software IO TLB [mem 0xbbfdf000-0xbffdf000] (64MB)
> > mapped at [ffffa3fcfbfdf000-ffffa3fcfffdefff]
> > lib/swiotlb.c
> 
> Yes, I think the solution for a lot of the random device discovery
> messages etc is to just remove them. They were likely useful when the
> code was new and untested, and just stayed around afterwards.

Is anyone actually going to do this? If the hashing gets done then these
messages are not a risk, are _real_ kernel developers going to bother
cleaning this up? Surely newbies are not going to get much love either
if they submit patches that '[PATCH] remove unnecessary printk message'.

thanks,
Tobin.

[kernel-hardening] Re: [PATCH v3] scripts: add leaking_addresses.pl

[Tobin C. Harding]

On Tue, Nov 07, 2017 at 01:44:01PM -0800, Linus Torvalds wrote:
> On Tue, Nov 7, 2017 at 1:22 PM, Kees Cook <keescook@chromium.org> wrote:
> >
> > Linus, what do you have in mind for the root-only "yes we really need
> > the actual address output" exceptions?
> 
> I am convinced that absolutely none of them should use '%pK'.
> 
> So far we have actually never seen a valid case wher %pK was really
> the right thing to do.
> 
> > For example, right now /sys/kernel/debug/kernel_page_tables
> > (CONFIG_X86_PTDUMP=y) needs actual address and currently uses %x.
> 
> So I think it could continue to use %x, and just make sure the whole
> file is root-only.
> 
> And that is why %pK is so wrong. It's almost never really about root.
> 
> Look at /proc/kallsyms, for example. There it's mainly about kernel
> profiles (although there certainly have been other uses historically,
> and maybe some of them remain) - which we have another flag for
> entirely that is very much specifically about kernel profiles.
> 
> > Looking other places that stand out, it seems like
> > /proc/lockdep_chains and /proc/lockdep (CONFIG_LOCKDEP=y) has a ton of
> > %p usage. It's unclear to me if a hash is sufficient for meaningful
> > debugging there?
> 
> Maybe not, but that is also _so_ esoteric that I suspect the right fix
> is to just make it root-only readable.
> 
> I've never used it, we should check with people who have. I get the
> feeling that this is purely for PeterZ debugging.
> 
> The very first commit that introduced that code actually has a
> 
>     (FIXME: should go into debugfs)
> 
> so I suspect it never should have been user-readable to begin with. I
> guess it makes some things easier, but it really is *very* different
> from things like profiling.
> 
> Profiling you often *cannot* do as root - some things you profile
> really shouldn't be run as root, and might even refuse to do so. So
> requiring you to be root just to get a kernel profile is very bad.
> 
> But looking at lockdep stats? Yeah, 'sudo' isn't so big of a deal.
> 
> And I really suspect that's true of a _lot_ of these %p things that
> really want a pointer. It's not that they really want %pK, it's that
> they shouldn't have been visible to regular users in the first place.
> 
> Things that *do* want a pointer and should be visible to regular users
> are things like oops messages etc, but even there we obviously
> generally want to use %pS/%pF when possible (but generally %x when not
> - things like register contents etc that *may* contain pointers).

This is opt-in, it means asking developers to do the right thing every time
they think they need a pointer. If we hash %p as soon as someone has
been bitten by it they will start using %x and sooner or later they will
use %x exclusively (suggesting using %x for _really_ necessary addresses
will only hasten the process).

If the only real benefit of hashing %p is to clean up kruft why don't we
add %pX and do tree wide substitution of all %p to %pX. People that get
broken will change it back to %p and we will have half a chance of
finding new abusers of %p down the track.

If there is not a 'one size fits all' solution, as we have seen with
kptr_restrict, then should we not be trying to make it easier for people
to do the right thing _and_ easier for us to catch it when we do the
wrong thing?

There is already almost 30 000 users of %{x,X}, surely we don't want to
promote more kernel address printing to be mixed in with all of that?

> And if they really are visible to users - because you want to
> cross-correlate them for things like netstat - I think the hashing is
> the right thing to do both for root and for regular users.
> 
> > Seems like these three from dmesg could be removed?
> >
> > [    0.000000] Base memory trampoline at [ffffa3fc40099000] 99000 size 24576
> > arch/x86/realmode/init.c
> >
> > [    0.000000] percpu: Embedded 38 pages/cpu @ffffa4007fc00000 s116944
> > r8192 d30512 u524288
> > mm/percpu.c
> >
> > [    0.456395] software IO TLB [mem 0xbbfdf000-0xbffdf000] (64MB)
> > mapped at [ffffa3fcfbfdf000-ffffa3fcfffdefff]
> > lib/swiotlb.c
> 
> Yes, I think the solution for a lot of the random device discovery
> messages etc is to just remove them. They were likely useful when the
> code was new and untested, and just stayed around afterwards.

Is anyone actually going to do this? If the hashing gets done then these
messages are not a risk, are _real_ kernel developers going to bother
cleaning this up? Surely newbies are not going to get much love either
if they submit patches that '[PATCH] remove unnecessary printk message'.

thanks,
Tobin.

Re: [PATCH v3] scripts: add leaking_addresses.pl

[Tobin C. Harding]

On Mon, Nov 06, 2017 at 09:27:09AM -0800, Linus Torvalds wrote:
> On Sun, Nov 5, 2017 at 9:19 PM, Tobin C. Harding <me@tobin.cc> wrote:
> > Currently we are leaking addresses from the kernel to user space. This
> > script is an attempt to find some of those leakages. Script parses
> > `dmesg` output and /proc and /sys files for hex strings that look like
> > kernel addresses.
> 
> Lovely. This is great. It shows just how much totally pointless stuff
> we leak, and to normal users that really shouldn't need it.
> 
> I had planned to wait for 4.15 to look at the printk hashing stuff
> etc, but this part I think I could/should merge early just because I
> think a lot of kernel developers will go "Why the f*ck would we expose
> that kernel address there?"

If we assume kptr_restrict is totally broken. Running this (with
kptr_restrict==0 and outputting a summary) hints that plugging all these
leaks is not an insurmountable problem. Perhaps we do not need to hash
%p by default? If we add %pX (or what ever) that hashes the address then
using the script to find the problems we can migrate to %pX as needed.

This approach relies on a concrete concensus (do we have those ;) as to
how and when we should print pointers. I just replied to you on that
topic in another thread. This raises the issue that this hashing
discussion is crazy fragmented and hard for people to follow (CC lists
are different on a few of the threads also), is it correct protocol to
patch Documentation with the concensus we have so far?

---
 Documentation/leaking_addresses.rst | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 Documentation/leaking_addresses.rst

diff --git a/Documentation/leaking_addresses.rst b/Documentation/leaking_addresses.rst
new file mode 100644
index 000000000000..737a1b76b3f7
--- /dev/null
+++ b/Documentation/leaking_addresses.rst
@@ -0,0 +1,33 @@
+Leaking Kernel Addresses
+========================
+
+If we show kernel addresses to user space bad things can happen.
+
+Work in Progress
+----------------
+
+- Create a tool to scan the kernel for leaking addresses.
+  - Partially done, see scripts/leaking_addresses.pl
+- Provide some sort of pointer hashing (i.e unique identifier).
+- Move away from kptr_restrict (and %pK).
+- Fix leaks on a case by case basis.
+
+WTF, just tell me how to print a pointer
+----------------------------------------
+
+Essentially you must consider _carefully_ who the needs to see the address and why.
+
+- If it is for profiling guard with perf_event_paranoid.
+- If the file is intended for root-only, then guard via file permissions.
+- If a unique identifier will suffice use hashing specifier (still to do).
+
+- If you really need the address ...    
+
+Ideas
+-----
+
+- Add a printk specifier that prints conditionally based on
+  perf_event_paranoid? 
+
+- Add seq_printf_sanitize() that only shows addresses to root (based on the
+  permissions of the process that opens the file)?  
-- 
2.7.4

thanks,
Tobin.

Re: [PATCH v3] scripts: add leaking_addresses.pl

[Tobin C. Harding]

On Mon, Nov 06, 2017 at 09:27:09AM -0800, Linus Torvalds wrote:
> On Sun, Nov 5, 2017 at 9:19 PM, Tobin C. Harding <me@tobin.cc> wrote:
> > Currently we are leaking addresses from the kernel to user space. This
> > script is an attempt to find some of those leakages. Script parses
> > `dmesg` output and /proc and /sys files for hex strings that look like
> > kernel addresses.
> 
> Lovely. This is great. It shows just how much totally pointless stuff
> we leak, and to normal users that really shouldn't need it.
> 
> I had planned to wait for 4.15 to look at the printk hashing stuff
> etc, but this part I think I could/should merge early just because I
> think a lot of kernel developers will go "Why the f*ck would we expose
> that kernel address there?"

If we assume kptr_restrict is totally broken. Running this (with
kptr_restrict==0 and outputting a summary) hints that plugging all these
leaks is not an insurmountable problem. Perhaps we do not need to hash
%p by default? If we add %pX (or what ever) that hashes the address then
using the script to find the problems we can migrate to %pX as needed.

This approach relies on a concrete concensus (do we have those ;) as to
how and when we should print pointers. I just replied to you on that
topic in another thread. This raises the issue that this hashing
discussion is crazy fragmented and hard for people to follow (CC lists
are different on a few of the threads also), is it correct protocol to
patch Documentation with the concensus we have so far?

---
 Documentation/leaking_addresses.rst | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 Documentation/leaking_addresses.rst

diff --git a/Documentation/leaking_addresses.rst b/Documentation/leaking_addresses.rst
new file mode 100644
index 000000000000..737a1b76b3f7
--- /dev/null
+++ b/Documentation/leaking_addresses.rst
@@ -0,0 +1,33 @@
+Leaking Kernel Addresses
+========================
+
+If we show kernel addresses to user space bad things can happen.
+
+Work in Progress
+----------------
+
+- Create a tool to scan the kernel for leaking addresses.
+  - Partially done, see scripts/leaking_addresses.pl
+- Provide some sort of pointer hashing (i.e unique identifier).
+- Move away from kptr_restrict (and %pK).
+- Fix leaks on a case by case basis.
+
+WTF, just tell me how to print a pointer
+----------------------------------------
+
+Essentially you must consider _carefully_ who the needs to see the address and why.
+
+- If it is for profiling guard with perf_event_paranoid.
+- If the file is intended for root-only, then guard via file permissions.
+- If a unique identifier will suffice use hashing specifier (still to do).
+
+- If you really need the address ...    
+
+Ideas
+-----
+
+- Add a printk specifier that prints conditionally based on
+  perf_event_paranoid? 
+
+- Add seq_printf_sanitize() that only shows addresses to root (based on the
+  permissions of the process that opens the file)?  
-- 
2.7.4

thanks,
Tobin.

[kernel-hardening] Re: [PATCH v3] scripts: add leaking_addresses.pl

[Tobin C. Harding]

On Mon, Nov 06, 2017 at 09:27:09AM -0800, Linus Torvalds wrote:
> On Sun, Nov 5, 2017 at 9:19 PM, Tobin C. Harding <me@tobin.cc> wrote:
> > Currently we are leaking addresses from the kernel to user space. This
> > script is an attempt to find some of those leakages. Script parses
> > `dmesg` output and /proc and /sys files for hex strings that look like
> > kernel addresses.
> 
> Lovely. This is great. It shows just how much totally pointless stuff
> we leak, and to normal users that really shouldn't need it.
> 
> I had planned to wait for 4.15 to look at the printk hashing stuff
> etc, but this part I think I could/should merge early just because I
> think a lot of kernel developers will go "Why the f*ck would we expose
> that kernel address there?"

If we assume kptr_restrict is totally broken. Running this (with
kptr_restrict==0 and outputting a summary) hints that plugging all these
leaks is not an insurmountable problem. Perhaps we do not need to hash
%p by default? If we add %pX (or what ever) that hashes the address then
using the script to find the problems we can migrate to %pX as needed.

This approach relies on a concrete concensus (do we have those ;) as to
how and when we should print pointers. I just replied to you on that
topic in another thread. This raises the issue that this hashing
discussion is crazy fragmented and hard for people to follow (CC lists
are different on a few of the threads also), is it correct protocol to
patch Documentation with the concensus we have so far?

---
 Documentation/leaking_addresses.rst | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 Documentation/leaking_addresses.rst

diff --git a/Documentation/leaking_addresses.rst b/Documentation/leaking_addresses.rst
new file mode 100644
index 000000000000..737a1b76b3f7
--- /dev/null
+++ b/Documentation/leaking_addresses.rst
@@ -0,0 +1,33 @@
+Leaking Kernel Addresses
+========================
+
+If we show kernel addresses to user space bad things can happen.
+
+Work in Progress
+----------------
+
+- Create a tool to scan the kernel for leaking addresses.
+  - Partially done, see scripts/leaking_addresses.pl
+- Provide some sort of pointer hashing (i.e unique identifier).
+- Move away from kptr_restrict (and %pK).
+- Fix leaks on a case by case basis.
+
+WTF, just tell me how to print a pointer
+----------------------------------------
+
+Essentially you must consider _carefully_ who the needs to see the address and why.
+
+- If it is for profiling guard with perf_event_paranoid.
+- If the file is intended for root-only, then guard via file permissions.
+- If a unique identifier will suffice use hashing specifier (still to do).
+
+- If you really need the address ...    
+
+Ideas
+-----
+
+- Add a printk specifier that prints conditionally based on
+  perf_event_paranoid? 
+
+- Add seq_printf_sanitize() that only shows addresses to root (based on the
+  permissions of the process that opens the file)?  
-- 
2.7.4

thanks,
Tobin.

Re: [PATCH v3] scripts: add leaking_addresses.pl

[Tobin C. Harding]

On Tue, Nov 07, 2017 at 01:22:13PM -0800, Kees Cook wrote:
> On Mon, Nov 6, 2017 at 9:27 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
[snip]
> And for my kernels, I needed to exclude usbmon or the script would
> hang (perhaps add a read timeout to the script to detect stalling
> files?)

Bother. I submitted the patch set without adding this suggestion. Even
after apologizing for missing it.

Better context switching required.

thanks,
Tobin.

Re: [PATCH v3] scripts: add leaking_addresses.pl

[Tobin C. Harding]

On Tue, Nov 07, 2017 at 01:22:13PM -0800, Kees Cook wrote:
> On Mon, Nov 6, 2017 at 9:27 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
[snip]
> And for my kernels, I needed to exclude usbmon or the script would
> hang (perhaps add a read timeout to the script to detect stalling
> files?)

Bother. I submitted the patch set without adding this suggestion. Even
after apologizing for missing it.

Better context switching required.

thanks,
Tobin.

[kernel-hardening] Re: [PATCH v3] scripts: add leaking_addresses.pl

[Tobin C. Harding]

On Tue, Nov 07, 2017 at 01:22:13PM -0800, Kees Cook wrote:
> On Mon, Nov 6, 2017 at 9:27 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
[snip]
> And for my kernels, I needed to exclude usbmon or the script would
> hang (perhaps add a read timeout to the script to detect stalling
> files?)

Bother. I submitted the patch set without adding this suggestion. Even
after apologizing for missing it.

Better context switching required.

thanks,
Tobin.

Re: [PATCH v3] scripts: add leaking_addresses.pl

[Peter Zijlstra]

On Tue, Nov 07, 2017 at 05:44:13PM -0500, Steven Rostedt wrote:
> On Tue, 7 Nov 2017 13:44:01 -0800
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > > Looking other places that stand out, it seems like
> > > /proc/lockdep_chains and /proc/lockdep (CONFIG_LOCKDEP=y) has a ton of
> > > %p usage. It's unclear to me if a hash is sufficient for meaningful
> > > debugging there?  
> > 
> > Maybe not, but that is also _so_ esoteric that I suspect the right fix
> > is to just make it root-only readable.
> 
> Also note, I don't believe anyone should be running a LOCKDEP
> configured kernel in a production (secured) environment. As it adds
> quite a bit of overhead. It's something you run on test environments to
> make sure it doesn't detect any possible deadlocks.
> 
> > 
> > I've never used it, we should check with people who have. I get the
> > feeling that this is purely for PeterZ debugging.
> 
> I've used it. But then again, I also debug lockdep ;-)
> 
> > 
> > The very first commit that introduced that code actually has a
> > 
> >     (FIXME: should go into debugfs)
> > 
> > so I suspect it never should have been user-readable to begin with. I
> > guess it makes some things easier, but it really is *very* different
> > from things like profiling.
> 
> Want me to whip up a patch to move the file?

Fine by me; create /debug/lockdep/ for the 3 files or something like
that.

As to the actual addresses, they can be used to double check things are
in fact the same object (in case of name collisions), are in static
memory (as these things ought to be) etc.. But mostly they're not too
important.

And yes, as everybody says, LOCKDEP is debug tool; you run this on your
(local) dev kernels, anything else it out of spec.

Re: [PATCH v3] scripts: add leaking_addresses.pl

[Peter Zijlstra]

On Tue, Nov 07, 2017 at 05:44:13PM -0500, Steven Rostedt wrote:
> On Tue, 7 Nov 2017 13:44:01 -0800
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > > Looking other places that stand out, it seems like
> > > /proc/lockdep_chains and /proc/lockdep (CONFIG_LOCKDEP=y) has a ton of
> > > %p usage. It's unclear to me if a hash is sufficient for meaningful
> > > debugging there?  
> > 
> > Maybe not, but that is also _so_ esoteric that I suspect the right fix
> > is to just make it root-only readable.
> 
> Also note, I don't believe anyone should be running a LOCKDEP
> configured kernel in a production (secured) environment. As it adds
> quite a bit of overhead. It's something you run on test environments to
> make sure it doesn't detect any possible deadlocks.
> 
> > 
> > I've never used it, we should check with people who have. I get the
> > feeling that this is purely for PeterZ debugging.
> 
> I've used it. But then again, I also debug lockdep ;-)
> 
> > 
> > The very first commit that introduced that code actually has a
> > 
> >     (FIXME: should go into debugfs)
> > 
> > so I suspect it never should have been user-readable to begin with. I
> > guess it makes some things easier, but it really is *very* different
> > from things like profiling.
> 
> Want me to whip up a patch to move the file?

Fine by me; create /debug/lockdep/ for the 3 files or something like
that.

As to the actual addresses, they can be used to double check things are
in fact the same object (in case of name collisions), are in static
memory (as these things ought to be) etc.. But mostly they're not too
important.

And yes, as everybody says, LOCKDEP is debug tool; you run this on your
(local) dev kernels, anything else it out of spec.

[kernel-hardening] Re: [PATCH v3] scripts: add leaking_addresses.pl

[Peter Zijlstra]

On Tue, Nov 07, 2017 at 05:44:13PM -0500, Steven Rostedt wrote:
> On Tue, 7 Nov 2017 13:44:01 -0800
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > > Looking other places that stand out, it seems like
> > > /proc/lockdep_chains and /proc/lockdep (CONFIG_LOCKDEP=y) has a ton of
> > > %p usage. It's unclear to me if a hash is sufficient for meaningful
> > > debugging there?  
> > 
> > Maybe not, but that is also _so_ esoteric that I suspect the right fix
> > is to just make it root-only readable.
> 
> Also note, I don't believe anyone should be running a LOCKDEP
> configured kernel in a production (secured) environment. As it adds
> quite a bit of overhead. It's something you run on test environments to
> make sure it doesn't detect any possible deadlocks.
> 
> > 
> > I've never used it, we should check with people who have. I get the
> > feeling that this is purely for PeterZ debugging.
> 
> I've used it. But then again, I also debug lockdep ;-)
> 
> > 
> > The very first commit that introduced that code actually has a
> > 
> >     (FIXME: should go into debugfs)
> > 
> > so I suspect it never should have been user-readable to begin with. I
> > guess it makes some things easier, but it really is *very* different
> > from things like profiling.
> 
> Want me to whip up a patch to move the file?

Fine by me; create /debug/lockdep/ for the 3 files or something like
that.

As to the actual addresses, they can be used to double check things are
in fact the same object (in case of name collisions), are in static
memory (as these things ought to be) etc.. But mostly they're not too
important.

And yes, as everybody says, LOCKDEP is debug tool; you run this on your
(local) dev kernels, anything else it out of spec.