On Wed, 2017-12-06 at 15:04 +1100, Tobin C. Harding wrote:
> On Tue, Dec 05, 2017 at 11:56:44AM +0530, kaiwan.billimo...@gmail.com wrote:
> > Currently, leaking_addresses.pl only supports scanning 64 bit
> > architectures. This is due to how the regular expressions are formed. We
> > can do better than this. 32 architectures can be supported if we take
> > into consideration the kernel virtual address split (via the PAGE_OFFSET
> > kernel configurable).
> >
> > Add support for ix86 32 bit architectures.
> > - Add command line option for page offset.
> > - Add command line option for kernel configuration file.
> > - Parse kernel config file for page offset (CONFIG_PAGE_OFFSET).
> > - Use page offset when checking for kernel virtual addresses.
> >
> >
> > Signed-off-by: Kaiwan N Billimoria <kaiwan.billimo...@gmail.com>
> > ---
>
> Right, this is starting to look awesome.
Great!
> > Note- This patch represents co development by Tobin and Kaiwan (plus
> > suggestions from
> > Alexander Kapshuk). Applies on Tobin's tree 'leaks' branch on top of commit
> > 680db1ef560f
> > (leaking_addresses: fix typo function not called).
> >
> >
> > scripts/leaking_addresses.pl | 169
> > +++++++++++++++++++++++++++++++++++++------
> > 1 file changed, 148 insertions(+), 21 deletions(-)
> >
> > diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> > index 2d5336b3e1ea..6b015980d117 100755
> > --- a/scripts/leaking_addresses.pl
> > +++ b/scripts/leaking_addresses.pl
> > @@ -24,6 +24,7 @@ use Cwd 'abs_path';
> > use Term::ANSIColor qw(:constants);
> > use Getopt::Long qw(:config no_auto_abbrev);
> > use Config;
> > +use feature 'state';
> >
> > my $P = $0;
> > my $V = '0.01';
> > @@ -37,18 +38,20 @@ my $TIMEOUT = 10;
> > # Script can only grep for kernel addresses on the following
> > architectures. If
> > # your architecture is not listed here and has a grep'able kernel address
> > please
> > # consider submitting a patch.
> > -my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64');
> > +my @SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64', 'i[3456]86');
> >
> > # Command line options.
> > my $help = 0;
> > my $debug = 0;
> > -my $raw = 0;
> > -my $output_raw = ""; # Write raw results to file.
> > -my $input_raw = ""; # Read raw results from file instead of
> > scanning.
> > +my $raw = 0; # Show raw output.
> > +my $output_raw = ""; # Write raw results to file.
> > +my $input_raw = ""; # Read raw results from file instead of
> > scanning.
> > +my $suppress_dmesg = 0; # Don't show dmesg in output.
> > +my $squash_by_path = 0; # Summary report grouped by absolute
> > path.
> > +my $squash_by_filename = 0; # Summary report grouped by filename.
> >
> > -my $suppress_dmesg = 0; # Don't show dmesg in output.
> > -my $squash_by_path = 0; # Summary report grouped by absolute
> > path.
> > -my $squash_by_filename = 0; # Summary report grouped by filename.
> > +my $page_offset_32bit = 0; # 32-bit: value of CONFIG_PAGE_OFFSET
> > +my $kernel_config_file = ""; # Kernel configuration file.
> >
> > # Do not parse these files (absolute path).
> > my @skip_parse_files_abs = ('/proc/kmsg',
> > @@ -97,14 +100,16 @@ Version: $V
> >
> > Options:
> >
> > - -o, --output-raw=<file> Save results for future processing.
> > - -i, --input-raw=<file> Read results from file instead of scanning.
> > - --raw Show raw results (default).
> > - --suppress-dmesg Do not show dmesg results.
> > - --squash-by-path Show one result per unique path.
> > - --squash-by-filename Show one result per unique filename.
> > - -d, --debug Display debugging output.
> > - -h, --help, --version Display this help and exit.
> > + -o, --output-raw=<file> Save results for future processing.
> > + -i, --input-raw=<file> Read results from file instead of
> > scanning.
> > + --raw Show raw results (default).
> > + --suppress-dmesg Do not show dmesg results.
> > + --squash-by-path Show one result per unique path.
> > + --squash-by-filename Show one result per unique filename.
> > + --page-offset-32bit=<hex> PAGE_OFFSET value (for 32-bit kernels).
> > + --kernel-config-file=<file> Kernel configuration file (e.g
> > /boot/config)
> > + -d, --debug Display debugging output.
> > + -h, --help, --version Display this help and exit.
> >
> > Examples:
> >
> > @@ -117,7 +122,10 @@ Examples:
> > # View summary report.
> > $0 --input-raw scan.out --squash-by-filename
> >
> > -Scans the running (64 bit) kernel for potential leaking addresses.
> > + # Scan kernel on a 32-bit system with a 2GB:2GB virtual address split.
> > + $0 --page-offset-32bit=0x80000000
> > +
> > +Scans the running kernel for potential leaking addresses.
> >
> > EOM
> > exit($exitcode);
> > @@ -133,6 +141,8 @@ GetOptions(
> > 'squash-by-path' => \$squash_by_path,
> > 'squash-by-filename' => \$squash_by_filename,
> > 'raw' => \$raw,
> > + 'page-offset-32bit=o' => \$page_offset_32bit,
> > + 'kernel-config-file=s' => \$kernel_config_file,
> > ) or help(1);
> >
> > help(0) if ($help);
> > @@ -148,6 +158,7 @@ if (!$input_raw and ($squash_by_path or
> > $squash_by_filename)) {
> > exit(128);
> > }
> >
> > +show_detected_architecture() if $debug;
> > if (!is_supported_architecture()) {
> > printf "\nScript does not support your architecture, sorry.\n";
> > printf "\nCurrently we support: \n\n";
> > @@ -179,7 +190,7 @@ sub dprint
> >
> > sub is_supported_architecture
> > {
> > - return (is_x86_64() or is_ppc64());
> > + return (is_x86_64() or is_ppc64() or is_ix86_32());
> > }
> >
> > sub is_x86_64
> > @@ -202,10 +213,40 @@ sub is_ppc64
> > return 0;
> > }
> >
> > +sub is_ix86_32
> > +{
> > + my $archname = $Config{archname};
> > +
> > + if ($archname =~ m/i[3456]86-linux/) {
> > + return 1;
> > + }
> > + return 0;
> > +}
> > +
> > +sub show_detected_architecture
> > +{
> > + printf "Detected architecture: ";
> > + if (is_ix86_32()) {
> > + printf "32 bit x86\n";
> > + } elsif (is_x86_64()) {
> > + printf "x86_64\n";
> > + } elsif (is_ppc64()) {
> > + printf "ppc64\n";
> > + } else {
> > + printf "failed to detect architecture\n"
> > + }
> > +}
> > +
> > sub is_false_positive
> > {
> > my ($match) = @_;
> >
> > + if (is_ix86_32()) {
> > + return is_false_positive_ix86_32($match);
> > + }
> > +
> > + # 64 bit architectures
> > +
> > if ($match =~ '\b(0x)?(f|F){16}\b' or
> > $match =~ '\b(0x)?0{16}\b') {
> > return 1;
> > @@ -222,6 +263,89 @@ sub is_false_positive
> > return 0;
> > }
> >
> > +sub is_false_positive_ix86_32
> > +{
> > + my ($match) = @_;
> > + state $page_offset = get_page_offset(); # only gets called once
>
> nit: new line here
Will do
> > + if ($match =~ '\b(0x)?(f|F){8}\b') {
> > + return 1;
> > + }
> > +
> > + my $addr32 = eval hex($match);
> > + if ($addr32 < $page_offset) {
> > + return 1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +sub get_page_offset
> > +{
> > + my $page_offset;
> > + my $default_offset = hex("0xc0000000");
> > + my @config_files;
>
> my $tmp_file = "";
>
> See comments below for reasoning.
>
> > + # Allow --page-offset-32bit to override.
> > + if ($page_offset_32bit != 0) {
> > + return $page_offset_32bit;
> > + }
> > +
> > + # Allow --kernel-config-file to override.
> > + if ($kernel_config_file ne "") {
> > + @config_files = ($kernel_config_file);
> > + } else {
> > + my $config_file = '/boot/config-' . `uname -r`;
> > + @config_files = ($config_file, '/boot/config');
> > + }
> > +
> > + if (-R "/proc/config.gz") {
> > + my $tmp_file = "/tmp/tmpkconf";
>
> $tmp_file = "/tmp/tmpkconf";
>
> > + if (system("gunzip < /proc/config.gz > $tmp_file")) {
> > + dprint " parse_kernel_config: system(gunzip...)
> > failed\n";
> > + } else {
> > + $page_offset = parse_kernel_config_file($tmp_file);
> > + if ($page_offset ne "") {
> > + return hex($page_offset);
> > + }
> > + }
> > + system("rm -f $tmp_file");
> > + }
>
> The logic is a bit broken here. sub returns without rm'ing tmp file.
Caught! :-) Thanks..
> I
> believe we discussed using
>
> @config_files = ($tmp_file);
>
> Then continuing to iterate @config_files as done.
I thought, why not just do this:
if (-R "/proc/config.gz") {
my $tmp_file = "/tmp/tmpkconf";
if (system("gunzip < /proc/config.gz > $tmp_file")) {
dprint " parse_kernel_config: system(gunzip...)
failed\n";
} else {
$page_offset = parse_kernel_config_file($tmp_file);
if ($page_offset ne "") {
system("rm -f $tmp_file");
return hex($page_offset);
}
system("rm -f $tmp_file");
}
}
Also, this way, the '$tmp_file' var remains localized to the handling of
the /proc/config.gz file 'if' statement scope.
> > +
> > + foreach my $config_file (@config_files) {
> > + chomp $config_file;
> > + $page_offset = parse_kernel_config_file($config_file);
> > + if ($page_offset ne "") {
> > + return hex($page_offset);
> > + }
> > + }
>
> We may need to use 'last' instead of returning so we can check for
>
> if ($tmp_file ne "") {
> system("rm -f $tmp_file");
> }
Not required, if we use the manner I propose above..
>
> And one final (particularly trivial) nitpick
>
> Can you use the brief commit log with prefix
>
> leaking_addresses:
>
> please. That prefix is what is currently used. Using 'scripts:' makes it
> hard to fit a descriptive message within 52 characters.
>
Understood, sorry for the current patch series not using this style.
> I know we have changed it already, but perhaps it should mention x86 not
> just 32 bit (since it is not 32 bit generic).
>
> I realized while reviewing your code that there is no reason for this to
> be x86 specific, if we can get a config file with CONFIG_PAGE_OFFSET
> then we can scan the kernel like this irrespective of architecture. Perl
> doesn't manage to correctly identify the RaspberryPi I tried it on as 32
> bit so we may not be able to do it how we currently are.
Interesting..
>
> I'm mentioning this because I don't want you to go to all this work and
> then remove a bunch of your code immediately while making it 32 bit
> generic. If you want to work on a generic version then I'm happy to work
> with you on it.
Sure, lets try for a generic ver! Thanks for your help on this..
As your experience woth the R Pi shows, we may have to just resort to building a
generic framework of sorts, letting folks "plugin" appropriate "truth values"
for their particular platform; this way, we support as much as we can for now
and, going forward, it's generic.
As of right now though, am unsure what this "generic framework" is..
> If you would prefer to just get this done and merged
> then we can do that too.
>
> As I've said before I'm new to the maintainer role so still learning how
> best to approach things. Thanks for your patience.
IMO, you're doing just great (me, am not so sure :) ).
Thanks,
Kaiwan.
> Hope this helps,
> Tobin.
