Re: [PATCH v3] scripts: leaking_addresses: add support for 32-bit kernel addresses

From: kaiwan . billimoria
Date: Wed Dec 06 2017 - 07:54:08 EST

On Wed, 2017-12-06 at 17:21 +0530, kaiwan.billimoria@xxxxxxxxx wrote:
> On Wed, 2017-12-06 at 15:04 +1100, Tobin C. Harding wrote:
> > On Tue, Dec 05, 2017 at 11:56:44AM +0530, kaiwan.billimoria@xxxxxxxxx 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.billimoria@xxxxxxxxx>
> > > ---
> >
> > 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..
>

A way forward, perhaps:
uname -m

Looks promising.
The output on a few test platforms:

+--------------+------------------+
Platform/CPU | `uname -m` |
+--------------+------------------+
x86_64 | x86_64 |
+--------------+------------------+
x86-32 | i686 |
+--------------+------------------+
ARM-32 (yocto | armv5tejl |
qemuarm32) | |
+--------------+------------------+
ARM64 (yocto | aarch64 |
qemuarm64) | |
+--------------+------------------+
MIPS64 (yocto | mips64 |
qemumips64) | |
+--------------+------------------+
ARM32 (qemu | armv7l |
IMX6 Sabrelite)| |
+--------------+------------------+

> > 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.