from:"Kaiwan N Billimoria"

Re: [RFC PATCH v3 1/8] Use refcount_t for ucounts reference counting

2021-01-18 Thread Kaiwan N Billimoria

(Sorry for the gmail client)
My 0.2, HTH:
a) AFAIK, refcount_inc() (and similar friends) don't return any value
b) they're designed to just WARN() if they saturate or if you're
attempting to increment the value 0 (as it's possibly a UAF bug)
c) refcount_inc_checked() is documented as "Similar to atomic_inc(),
but will saturate at UINT_MAX and WARN"
d) we should avoid using the __foo() when foo() 's present as far as
is sanely possible...

So is one expected to just fix things when they break? - as signalled
by the WARN firing?

--
Regards, kaiwan.


On Tue, Jan 19, 2021 at 2:26 AM Alexey Gladkov  wrote:
>
> On Mon, Jan 18, 2021 at 12:34:29PM -0800, Linus Torvalds wrote:
> > On Mon, Jan 18, 2021 at 11:46 AM Alexey Gladkov
> >  wrote:
> > >
> > > Sorry about that. I thought that this code is not needed when switching
> > > from int to refcount_t. I was wrong.
> >
> > Well, you _may_ be right. I personally didn't check how the return
> > value is used.
> >
> > I only reacted to "it certainly _may_ be used, and there is absolutely
> > no comment anywhere about why it wouldn't matter".
>
> I have not found examples where checked the overflow after calling
> refcount_inc/refcount_add.
>
> For example in kernel/fork.c:2298 :
>
>current->signal->nr_threads++;
>atomic_inc(>signal->live);
>refcount_inc(>signal->sigcnt);
>
> $ semind search signal_struct.sigcnt
> def include/linux/sched/signal.h:83 refcount_t  
> sigcnt;
> m-- kernel/fork.c:723 put_signal_struct if 
> (refcount_dec_and_test(>sigcnt))
> m-- kernel/fork.c:1571 copy_signal  refcount_set(>sigcnt, 1);
> m-- kernel/fork.c:2298 copy_process 
> refcount_inc(>signal->sigcnt);
>
> It seems to me that the only way is to use __refcount_inc and then compare
> the old value with REFCOUNT_MAX
>
> Since I have not seen examples of such checks, I thought that this is
> acceptable. Sorry once again. I have not tried to hide these changes.
>
> --
> Rgrds, legion
>
>

cgroups v2: issues faced attempting to setup cpu limiting

2019-07-22 Thread Kaiwan N Billimoria

Hi All,

Am facing some issues getting CPU limiting working with cgroups v2. Pl read on
for the details; a solution is much appreciated!


Env: 5.0.0 Linux kernel on x86_64 Fedora 29

When attempting to setup CPU limiting using cgroups v2, I effectively
disabled cgroups v1 by passing

cgroup_no_v1=all

as a kernel cmdline option. That seems fine and now various controllers (cpu,
io, memory, ...) show up under /sys/fs/cgroup/unified/cgroup.controllers.

However, doing:

# mkdir /sys/fs/cgroup/unified/test1
# echo "+cpu " > /sys/fs/cgroup/unified/test1/cgroup.subtree_control
bash: echo: write error: No such file or directory
#

I understand that this is expected, as the man page on cgroups(7) mentions:

"... As at Linux 4.15, the cgroups v2 cpu controller does not support
control of realtime processes, and the controller can be enabled in the
root cgroup only if all realtime threads are in the root cgroup. (If there
are realtime processes in nonroot cgroups, then a write(2) of the string
"+cpu" to the cgroup.subtree_control file fails with the error EINVAL.
However, on some systems, systemd(1) places certain realtime processes in
nonroot cgroups in the v2 hierarchy. On such systems, these processes must
first be moved to the root cgroup before the cpu controller can be
enabled. ..."

My questions are (forgive them if too basic!): how exactly does one
'move realtime processes to the root cgroup'? What are the commands?

Next, how does one identify which processes? The ones that have sched policy
SCHED_FIFO or SCHED_RR? Would using a utility wrapper make this simpler?
(libcgroup, cgmanager, etc) - do they play well with cgroups2?

TIA!

Regards,
Kaiwan.
---
amazon author page: https://www.amazon.com/-/e/B07KNJSRJX

cgroups v2: issues faced attempting to setup cpu limiting

2019-07-16 Thread Kaiwan N Billimoria

Hi,

Am facing some issues getting CPU limiting working with cgroups v2. Pl read on 
for the details;
a solution is much appreciated!


Env: custom 5.0.0 Linux kernel on x86_64 Fedora 29

When attempting to setup CPU limiting using cgroups v2, I effectively disabled 
cgroups v1 by passing

cgroup_no_v1=all

as a kernel cmdline option. That seems fine and now various controllers (cpu, 
io, memory, ...)
show up under /sys/fs/cgroup/unified/cgroup.controllers.

However, doing:

# mkdir /sys/fs/cgroup/unified/test1
# echo "+cpu " > /sys/fs/cgroup/unified/test1/cgroup.subtree_control
bash: echo: write error: No such file or directory
#

I understand that this is expected, as the man page on cgroups(7) mentions:

"... As at Linux 4.15, the cgroups v2 cpu controller does not support
control of realtime processes, and the controller can be enabled in the
root cgroup only if all realtime threads are in the root cgroup. (If there
are realtime processes in nonroot cgroups, then a write(2) of the string
"+cpu" to the cgroup.subtree_control file fails with the error EINVAL.
However, on some systems, systemd(1) places certain realtime processes in
nonroot cgroups in the v2 hierarchy. On such systems, these processes must
first be moved to the root cgroup before the cpu controller can be
enabled. ..."

My questions are: how exactly does one 'move realtime processes to the root 
cgroup'?
What are the commands?

Next, how does one identify which processes? The ones that have sched policy 
SCHED_FIFO
or SCHED_RR? Would using a utility wrapper make this simpler? (libcgroup, 
cgmanager,
etc) - do they play well with cgroups2?

TIA!

Regards,
Kaiwan.

amazon author page: https://www.amazon.com/-/e/B07KNJSRJX

Re: [PATCH] leaking_addresses: add generic 32-bit support

2017-12-25 Thread Kaiwan N Billimoria

The script attempts to detect the architecture it's running upon; as of now,
we explicitly support x86_64, PPC64 and x86_32.
If it's one of them, we proceed "normally". If we fail to detect the arch,
we fallback to 64-bit scanning, unless the user has passed either of these
option switches: "--opt-32bit" and/or "--page-offset-32bit=".

If so, we switch to scanning for leaked addresses based on the value of
PAGE_OFFSET (via an auto-detected or fallback mechanism).

As of now, we have code (or "rules") to detect special cases for x86_64 and 
PPC64
(in the get_address_re sub). Also, we now have also builtin "stubs", for lack 
of a better term, where additional rules for other 64-bit arch's can be plugged 
into the code,
in future, as applicable.

Signed-off-by: Kaiwan N Billimoria <kaiwan.billimo...@gmail.com>

---
 scripts/leaking_addresses.pl | 190 +++
 1 file changed, 156 insertions(+), 34 deletions(-)

This patch is based on Tobin's suggestions and my replies to them (see prev 
email in this thread).


diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
index a29e13e577a7..b0807b3a3c7c 100755
--- a/scripts/leaking_addresses.pl
+++ b/scripts/leaking_addresses.pl
@@ -1,10 +1,10 @@
 #!/usr/bin/env perl
 #
 # (c) 2017 Tobin C. Harding <m...@tobin.cc>
-
+# (c) 2017 Kaiwan N Billimoria <kaiwan.billimo...@gmail.com>
 # Licensed under the terms of the GNU GPL License version 2
 #
-# leaking_addresses.pl: Scan 64 bit kernel for potential leaking addresses.
+# leaking_addresses.pl: Scan kernel for potential leaking addresses.
 #  - Scans dmesg output.
 #  - Walks directory tree and parses each file (for each directory in @DIRS).
 #
@@ -32,11 +32,6 @@ my @DIRS = ('/proc', '/sys');
 # Timer for parsing each file, in seconds.
 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');
-
 # Command line options.
 my $help = 0;
 my $debug = 0;
@@ -48,7 +43,9 @@ 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 $kernel_config_file = "";   # Kernel configuration file.
+my $opt_32_bit = 0;# Detect (only) 32-bit kernel leaking addresses.
+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',
@@ -104,10 +101,12 @@ Options:
  --squash-by-path  Show one result per unique path.
  --squash-by-filename  Show one result per unique filename.
--kernel-config-file= Kernel configuration file (e.g 
/boot/config)
+   --opt-32bit Detect (only) 32-bit kernel leaking 
addresses.
+   --page-offset-32bit=   PAGE_OFFSET value (for 32-bit kernels).
-d, --debug Display debugging output.
-   -h, --help, --versionq  Display this help and exit.
+   -h, --help, --version   Display this help and exit.
 
-Scans the running (64 bit) kernel for potential leaking addresses.
+Scans the running kernel for potential leaking addresses.
 
 EOM
exit($exitcode);
@@ -123,7 +122,9 @@ GetOptions(
'squash-by-path'=> \$squash_by_path,
'squash-by-filename'=> \$squash_by_filename,
'raw'   => \$raw,
-   'kernel-config-file=s'  => \$kernel_config_file,
+   'opt-32bit' => \$opt_32_bit,
+   'page-offset-32bit=o'   => \$page_offset_32bit,
+   'kernel-config-file=s'  => \$kernel_config_file,
 ) or help(1);
 
 help(0) if ($help);
@@ -139,16 +140,15 @@ if (!$input_raw and ($squash_by_path or 
$squash_by_filename)) {
exit(128);
 }
 
-if (!is_supported_architecture()) {
-   printf "\nScript does not support your architecture, sorry.\n";
-   printf "\nCurrently we support: \n\n";
-   foreach(@SUPPORTED_ARCHITECTURES) {
-   printf "\t%s\n", $_;
-   }
+show_detected_architecture() if $debug;
 
-   my $archname = $Config{archname};
-   printf "\n\$ perl -MConfig -e \'print \"\$Config{archname}\\n\"\'\n";
-   printf "%s\n", $archname;
+if (!is_known_architecture()) {
+   printf STDERR "\nFATAL: Script does not recognize your architecture\n";
+
+   my $arch = `uname -m`;
+   chomp $arch;
+   printf "\n\$ uname -m\n";
+   printf "%s\n", $arch;
 
exit(129);
 }
@@ -168,21 +168,45

Re: [PATCH] leaking_addresses: add generic 32-bit support

2017-12-25 Thread Kaiwan N Billimoria

The script attempts to detect the architecture it's running upon; as of now,
we explicitly support x86_64, PPC64 and x86_32.
If it's one of them, we proceed "normally". If we fail to detect the arch,
we fallback to 64-bit scanning, unless the user has passed either of these
option switches: "--opt-32bit" and/or "--page-offset-32bit=".

If so, we switch to scanning for leaked addresses based on the value of
PAGE_OFFSET (via an auto-detected or fallback mechanism).

As of now, we have code (or "rules") to detect special cases for x86_64 and 
PPC64
(in the get_address_re sub). Also, we now have also builtin "stubs", for lack 
of a better term, where additional rules for other 64-bit arch's can be plugged 
into the code,
in future, as applicable.

Signed-off-by: Kaiwan N Billimoria 

---
 scripts/leaking_addresses.pl | 190 +++
 1 file changed, 156 insertions(+), 34 deletions(-)

This patch is based on Tobin's suggestions and my replies to them (see prev 
email in this thread).


diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
index a29e13e577a7..b0807b3a3c7c 100755
--- a/scripts/leaking_addresses.pl
+++ b/scripts/leaking_addresses.pl
@@ -1,10 +1,10 @@
 #!/usr/bin/env perl
 #
 # (c) 2017 Tobin C. Harding 
-
+# (c) 2017 Kaiwan N Billimoria 
 # Licensed under the terms of the GNU GPL License version 2
 #
-# leaking_addresses.pl: Scan 64 bit kernel for potential leaking addresses.
+# leaking_addresses.pl: Scan kernel for potential leaking addresses.
 #  - Scans dmesg output.
 #  - Walks directory tree and parses each file (for each directory in @DIRS).
 #
@@ -32,11 +32,6 @@ my @DIRS = ('/proc', '/sys');
 # Timer for parsing each file, in seconds.
 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');
-
 # Command line options.
 my $help = 0;
 my $debug = 0;
@@ -48,7 +43,9 @@ 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 $kernel_config_file = "";   # Kernel configuration file.
+my $opt_32_bit = 0;# Detect (only) 32-bit kernel leaking addresses.
+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',
@@ -104,10 +101,12 @@ Options:
  --squash-by-path  Show one result per unique path.
  --squash-by-filename  Show one result per unique filename.
--kernel-config-file= Kernel configuration file (e.g 
/boot/config)
+   --opt-32bit Detect (only) 32-bit kernel leaking 
addresses.
+   --page-offset-32bit=   PAGE_OFFSET value (for 32-bit kernels).
-d, --debug Display debugging output.
-   -h, --help, --versionq  Display this help and exit.
+   -h, --help, --version   Display this help and exit.
 
-Scans the running (64 bit) kernel for potential leaking addresses.
+Scans the running kernel for potential leaking addresses.
 
 EOM
exit($exitcode);
@@ -123,7 +122,9 @@ GetOptions(
'squash-by-path'=> \$squash_by_path,
'squash-by-filename'=> \$squash_by_filename,
'raw'   => \$raw,
-   'kernel-config-file=s'  => \$kernel_config_file,
+   'opt-32bit' => \$opt_32_bit,
+   'page-offset-32bit=o'   => \$page_offset_32bit,
+   'kernel-config-file=s'  => \$kernel_config_file,
 ) or help(1);
 
 help(0) if ($help);
@@ -139,16 +140,15 @@ if (!$input_raw and ($squash_by_path or 
$squash_by_filename)) {
exit(128);
 }
 
-if (!is_supported_architecture()) {
-   printf "\nScript does not support your architecture, sorry.\n";
-   printf "\nCurrently we support: \n\n";
-   foreach(@SUPPORTED_ARCHITECTURES) {
-   printf "\t%s\n", $_;
-   }
+show_detected_architecture() if $debug;
 
-   my $archname = $Config{archname};
-   printf "\n\$ perl -MConfig -e \'print \"\$Config{archname}\\n\"\'\n";
-   printf "%s\n", $archname;
+if (!is_known_architecture()) {
+   printf STDERR "\nFATAL: Script does not recognize your architecture\n";
+
+   my $arch = `uname -m`;
+   chomp $arch;
+   printf "\n\$ uname -m\n";
+   printf "%s\n", $arch;
 
exit(129);
 }
@@ -168,21 +168,45 @@ sub dprint
printf(STDERR @_) if $debug;
 }
 
-sub is_supported_architecture
+su

Re: [PATCH] leaking_addresses: add generic 32-bit support

2017-12-25 Thread Kaiwan N Billimoria

Hey, Merry Xmas all !!   :-)

Re inline below,
Updated patch to follow..

On Mon, 18 Dec 2017 16:57:46 +1100
"Tobin C. Harding" <m...@tobin.cc> wrote:

> On Mon, Dec 18, 2017 at 09:24:47AM +0530, kaiwan.billimo...@gmail.com
> wrote:
> > The script attempts to detect the architecture it's running upon;
> > as of now, we explicitly support x86_64, PPC64 and x86_32.
> > If it's one of them, we proceed "normally". If we fail to detect
> > the arch, we fallback to 64-bit scanning, unless the user has
> > passed either of these option switches: "--32-bit" and/or
> > "--page-offset-32bit=".
> > 
> > If so, we switch to scanning for leaked addresses based on the
> > value of PAGE_OFFSET (via an auto-detected or fallback mechanism).
> > 
> > As of now, we have code (or "rules") to detect special cases for
> > x86_64 and ppc64 (in the get_address_re sub). Also, we now have
> > also builtin "stubs", for lack of a better term, where additional
> > rules for other 64-bit arch's can be plugged in, in future, as
> > applicable.
> > 
> > Signed-off-by: Kaiwan N Billimoria <kaiwan.billimo...@gmail.com>
> > ---
> > 
> > This is a patch based on Tobin's latest tree, 'leaks' branch. 
> > Applies on top of commit 6c3942594657 (leaking_addresses: add
> > support for 5 page table levels (origin/leaks))  
> 
> That commit is not the tip of the branch. leaks branch is currently at
> 
> commit 266891c62bf0 (leaking_addresses: add support for 5 page table
> levels)
> 
> > 
> > Thanks,
> > Kaiwan.
> > 
> >  scripts/leaking_addresses.pl | 213
> > +-- 1 file changed, 184
> > insertions(+), 29 deletions(-)
> > 
> > diff --git a/scripts/leaking_addresses.pl
> > b/scripts/leaking_addresses.pl index a29e13e577a7..a667f243c95b
> > 100755 --- a/scripts/leaking_addresses.pl
> > +++ b/scripts/leaking_addresses.pl
> > @@ -1,10 +1,10 @@
> >  #!/usr/bin/env perl
> >  #
> >  # (c) 2017 Tobin C. Harding <m...@tobin.cc>
> > -
> > +# (c) 2017 Kaiwan N Billimoria <kaiwan.billimo...@gmail.com>
> >  # Licensed under the terms of the GNU GPL License version 2
> >  #
> > -# leaking_addresses.pl: Scan 64 bit kernel for potential leaking
> > addresses. +# leaking_addresses.pl: Scan kernel for potential
> > leaking addresses. #  - Scans dmesg output.
> >  #  - Walks directory tree and parses each file (for each directory
> > in @DIRS). #
> > @@ -35,7 +35,7 @@ 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;
> > @@ -48,7 +48,9 @@ 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 $kernel_config_file = "";   # Kernel configuration file.
> > +my $opt_32_bit = 0;# Detect 32-bit kernel leaking
> > addresses. +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,17 +99,19 @@ Version: $V
> >  
> >  Options:
> >  
> > -   -o, --output-raw= Save results for future
> > processing.
> > -   -i, --input-raw=  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.
> > -   --kernel-config-file= Kernel configuration file
> > (e.g /boot/config)
> > -   -d, --debug Display debugging output.
> > -   -h, --help, --versionq  Display this help and exit.
> > +   -o, --output-raw= Save results for future
> > processing.
> > +   -i, --input-raw=  Read re

Re: [PATCH] leaking_addresses: add generic 32-bit support

2017-12-25 Thread Kaiwan N Billimoria

Hey, Merry Xmas all !!   :-)

Re inline below,
Updated patch to follow..

On Mon, 18 Dec 2017 16:57:46 +1100
"Tobin C. Harding"  wrote:

> On Mon, Dec 18, 2017 at 09:24:47AM +0530, kaiwan.billimo...@gmail.com
> wrote:
> > The script attempts to detect the architecture it's running upon;
> > as of now, we explicitly support x86_64, PPC64 and x86_32.
> > If it's one of them, we proceed "normally". If we fail to detect
> > the arch, we fallback to 64-bit scanning, unless the user has
> > passed either of these option switches: "--32-bit" and/or
> > "--page-offset-32bit=".
> > 
> > If so, we switch to scanning for leaked addresses based on the
> > value of PAGE_OFFSET (via an auto-detected or fallback mechanism).
> > 
> > As of now, we have code (or "rules") to detect special cases for
> > x86_64 and ppc64 (in the get_address_re sub). Also, we now have
> > also builtin "stubs", for lack of a better term, where additional
> > rules for other 64-bit arch's can be plugged in, in future, as
> > applicable.
> > 
> > Signed-off-by: Kaiwan N Billimoria 
> > ---
> > 
> > This is a patch based on Tobin's latest tree, 'leaks' branch. 
> > Applies on top of commit 6c3942594657 (leaking_addresses: add
> > support for 5 page table levels (origin/leaks))  
> 
> That commit is not the tip of the branch. leaks branch is currently at
> 
> commit 266891c62bf0 (leaking_addresses: add support for 5 page table
> levels)
> 
> > 
> > Thanks,
> > Kaiwan.
> > 
> >  scripts/leaking_addresses.pl | 213
> > +-- 1 file changed, 184
> > insertions(+), 29 deletions(-)
> > 
> > diff --git a/scripts/leaking_addresses.pl
> > b/scripts/leaking_addresses.pl index a29e13e577a7..a667f243c95b
> > 100755 --- a/scripts/leaking_addresses.pl
> > +++ b/scripts/leaking_addresses.pl
> > @@ -1,10 +1,10 @@
> >  #!/usr/bin/env perl
> >  #
> >  # (c) 2017 Tobin C. Harding 
> > -
> > +# (c) 2017 Kaiwan N Billimoria 
> >  # Licensed under the terms of the GNU GPL License version 2
> >  #
> > -# leaking_addresses.pl: Scan 64 bit kernel for potential leaking
> > addresses. +# leaking_addresses.pl: Scan kernel for potential
> > leaking addresses. #  - Scans dmesg output.
> >  #  - Walks directory tree and parses each file (for each directory
> > in @DIRS). #
> > @@ -35,7 +35,7 @@ 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;
> > @@ -48,7 +48,9 @@ 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 $kernel_config_file = "";   # Kernel configuration file.
> > +my $opt_32_bit = 0;# Detect 32-bit kernel leaking
> > addresses. +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,17 +99,19 @@ Version: $V
> >  
> >  Options:
> >  
> > -   -o, --output-raw= Save results for future
> > processing.
> > -   -i, --input-raw=  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.
> > -   --kernel-config-file= Kernel configuration file
> > (e.g /boot/config)
> > -   -d, --debug Display debugging output.
> > -   -h, --help, --versionq  Display this help and exit.
> > +   -o, --output-raw= Save results for future
> > processing.
> > +   -i, --input-raw=  Read results from file
> > instead of scanning.
> > +   --raw   Show raw results
> > (def

Re: [PATCH 4/5] leaking_addresses: add support for kernel config file

2017-12-07 Thread Kaiwan N Billimoria

On Thu, Dec 7, 2017 at 10:02 AM, Tobin C. Harding <m...@tobin.cc> wrote:
> Features that rely on the ability to get kernel configuration options
> are ready to be implemented in script. In preparation for this we can
> add support for kernel config options as a separate patch to ease
> review.
>
> Add support for locating and parsing kernel configuration file.
>
> Signed-off-by: Tobin C. Harding <m...@tobin.cc>
> Co-Developed-by: Kaiwan N Billimoria <kaiwan.billimo...@gmail.com>
> ---
>
> get_kernel_config_option() is not super clean, any improvements most welcome.
>
> Kaiwan,
>
> This needs your Signed-off-by tag if you want me to apply it with
> the Co-Developed-tag
>
> thanks,
> Tobin.
>
Adding my signed-off tag..

Signed-off-by:  Kaiwan N Billimoria <kaiwan.billimo...@gmail.com>

>  scripts/leaking_addresses.pl | 64 
> +++-
>  1 file changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> index cb69ccd4153a..892bfe9e01fe 100755
> --- a/scripts/leaking_addresses.pl
> +++ b/scripts/leaking_addresses.pl
> @@ -42,10 +42,10 @@ 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 $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 $kernel_config_file = "";   # Kernel configuration file.
>
>  # Do not parse these files (absolute path).
>  my @skip_parse_files_abs = ('/proc/kmsg',
> @@ -100,6 +100,7 @@ Options:
>   --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.
> +   --kernel-config-file= Kernel configuration file (e.g 
> /boot/config)
> -d, --debug Display debugging output.
> -h, --help, --versionq  Display this help and exit.
>
> @@ -119,6 +120,7 @@ GetOptions(
> 'squash-by-path'=> \$squash_by_path,
> 'squash-by-filename'=> \$squash_by_filename,
> 'raw'   => \$raw,
> +   'kernel-config-file=s'  => \$kernel_config_file,
>  ) or help(1);
>
>  help(0) if ($help);
> @@ -188,6 +190,66 @@ sub is_ppc64
> return 0;
>  }
>
> +# gets config option value from kernel config file
> +sub get_kernel_config_option
> +{
> +   my ($option) = @_;
> +   my $value = "";
> +   my $tmp_file = "";
> +   my @config_files;
> +
> +   # Allow --kernel-config-file to override.
> +   if ($kernel_config_file ne "") {
> +   @config_files = ($kernel_config_file);
> +   } elsif (-R "/proc/config.gz") {
> +   my $tmp_file = "/tmp/tmpkconf";
> +
> +   if (system("gunzip < /proc/config.gz > $tmp_file")) {
> +   dprint "$0: system(gunzip < /proc/config.gz) 
> failed\n";
> +   } else {
> +   @config_files = ($tmp_file);
> +   }
> +
> +   } else {
> +   my $file = '/boot/config-' . `uname -r`;
> +   @config_files = ($file, '/boot/config');
> +   }
> +
> +   foreach my $file (@config_files) {
> +#  chomp $config_file;
> +   $value = option_from_file($option, $file);
> +   if ($value ne "") {
> +   last;
> +   }
> +   }
> +
> +   if ($tmp_file ne "") {
> +   system("rm -f $tmp_file");
> +   }
> +
> +   return $value;
> +}
> +
> +# Parses $file and returns kernel configuration option value.
> +sub option_from_file
> +{
> +   my ($option, $file) = @_;
> +   my $str = "";
> +   my $val = "";
> +
> +   open(my $fh, "<", $file) or return "";
> +   while (my $line = <$fh> ) {
> +   if ($line =~ /^$option/) {
> +   ($str, $val) = split /=/, $line;
> +   chomp($val);
> +   last;
> +   }
> +   }
> +
> +   close $fh;
> +   return $val;
> +}
> +
>  sub is_false_positive
>  {
> my ($match) = @_;
> --
> 2.7.4
>

Re: [PATCH 4/5] leaking_addresses: add support for kernel config file

2017-12-07 Thread Kaiwan N Billimoria

On Thu, Dec 7, 2017 at 10:02 AM, Tobin C. Harding  wrote:
> Features that rely on the ability to get kernel configuration options
> are ready to be implemented in script. In preparation for this we can
> add support for kernel config options as a separate patch to ease
> review.
>
> Add support for locating and parsing kernel configuration file.
>
> Signed-off-by: Tobin C. Harding 
> Co-Developed-by: Kaiwan N Billimoria 
> ---
>
> get_kernel_config_option() is not super clean, any improvements most welcome.
>
> Kaiwan,
>
> This needs your Signed-off-by tag if you want me to apply it with
> the Co-Developed-tag
>
> thanks,
> Tobin.
>
Adding my signed-off tag..

Signed-off-by:  Kaiwan N Billimoria 

>  scripts/leaking_addresses.pl | 64 
> +++-
>  1 file changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
> index cb69ccd4153a..892bfe9e01fe 100755
> --- a/scripts/leaking_addresses.pl
> +++ b/scripts/leaking_addresses.pl
> @@ -42,10 +42,10 @@ 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 $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 $kernel_config_file = "";   # Kernel configuration file.
>
>  # Do not parse these files (absolute path).
>  my @skip_parse_files_abs = ('/proc/kmsg',
> @@ -100,6 +100,7 @@ Options:
>   --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.
> +   --kernel-config-file= Kernel configuration file (e.g 
> /boot/config)
> -d, --debug Display debugging output.
> -h, --help, --versionq  Display this help and exit.
>
> @@ -119,6 +120,7 @@ GetOptions(
> 'squash-by-path'=> \$squash_by_path,
> 'squash-by-filename'=> \$squash_by_filename,
> 'raw'   => \$raw,
> +   'kernel-config-file=s'  => \$kernel_config_file,
>  ) or help(1);
>
>  help(0) if ($help);
> @@ -188,6 +190,66 @@ sub is_ppc64
> return 0;
>  }
>
> +# gets config option value from kernel config file
> +sub get_kernel_config_option
> +{
> +   my ($option) = @_;
> +   my $value = "";
> +   my $tmp_file = "";
> +   my @config_files;
> +
> +   # Allow --kernel-config-file to override.
> +   if ($kernel_config_file ne "") {
> +   @config_files = ($kernel_config_file);
> +   } elsif (-R "/proc/config.gz") {
> +   my $tmp_file = "/tmp/tmpkconf";
> +
> +   if (system("gunzip < /proc/config.gz > $tmp_file")) {
> +   dprint "$0: system(gunzip < /proc/config.gz) 
> failed\n";
> +   } else {
> +   @config_files = ($tmp_file);
> +   }
> +
> +   } else {
> +   my $file = '/boot/config-' . `uname -r`;
> +   @config_files = ($file, '/boot/config');
> +   }
> +
> +   foreach my $file (@config_files) {
> +#  chomp $config_file;
> +   $value = option_from_file($option, $file);
> +   if ($value ne "") {
> +   last;
> +   }
> +   }
> +
> +   if ($tmp_file ne "") {
> +   system("rm -f $tmp_file");
> +   }
> +
> +   return $value;
> +}
> +
> +# Parses $file and returns kernel configuration option value.
> +sub option_from_file
> +{
> +   my ($option, $file) = @_;
> +   my $str = "";
> +   my $val = "";
> +
> +   open(my $fh, "<", $file) or return "";
> +   while (my $line = <$fh> ) {
> +   if ($line =~ /^$option/) {
> +   ($str, $val) = split /=/, $line;
> +   chomp($val);
> +   last;
> +   }
> +   }
> +
> +   close $fh;
> +   return $val;
> +}
> +
>  sub is_false_positive
>  {
> my ($match) = @_;
> --
> 2.7.4
>

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

2017-12-04 Thread Kaiwan N Billimoria

Sure, thanks Alexander..
Tobin, request you to pl make the change while merging, thanks..


Thanks & Regards,
Kaiwan.


On Mon, Dec 4, 2017 at 6:07 PM, Alexander Kapshuk
<alexander.kaps...@gmail.com> wrote:
> On Mon, Dec 4, 2017 at 12:20 PM,  <kaiwan.billimo...@gmail.com> wrote:
>> On Mon, 2017-12-04 at 19:21 +1100, Tobin C. Harding wrote:
>>> On Mon, Dec 04, 2017 at 10:51:53AM +0530, Kaiwan N Billimoria wrote:
>>> > > ---
>>> > diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
>>> > index 9906dcf8b807..260b52e456f1 100755
>>> > --- a/scripts/leaking_addresses.pl
>>> > +++ b/scripts/leaking_addresses.pl
>>> > @@ -266,7 +266,7 @@ sub is_false_positive
>>> >  sub is_false_positive_ix86_32
>>> >  {
>>> > my ($match) = @_;
>>> > -   state $page_offset = eval get_page_offset(); # only gets called 
>>> > once
>>> > +   state $page_offset = hex get_page_offset(); # only gets called 
>>> > once
>>>
>>> I don't think this is valid ;) I meant use hex() to convert the string
>>> to an int so it doesn't throw the warning (inside get_page_offset()).
>>
>> Yup, got it, thanks   :-p
>> Combined patch below:
>>
>>
>> ---
>>  scripts/leaking_addresses.pl | 13 ++---
>>  1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
>> index 9906dcf8b807..a595a2c66b12 100755
>> --- a/scripts/leaking_addresses.pl
>> +++ b/scripts/leaking_addresses.pl
>> @@ -266,8 +266,7 @@ sub is_false_positive
>>  sub is_false_positive_ix86_32
>>  {
>> my ($match) = @_;
>> -   state $page_offset = eval get_page_offset(); # only gets called once
>> -
>> +   state $page_offset = get_page_offset(); # only gets called once
>> if ($match =~ '\b(0x)?(f|F){8}\b') {
>> return 1;
>> }
>> @@ -283,7 +282,7 @@ sub is_false_positive_ix86_32
>>  sub get_page_offset
>>  {
>> my $page_offset;
>> -   my $default_offset = "0xc000";
>> +   my $default_offset = hex("0xc000");
>> my @config_files;
>>
>> # Allow --page-offset-32bit to override.
>> @@ -306,23 +305,23 @@ sub get_page_offset
>> } else {
>> $page_offset = parse_kernel_config_file($tmp_file);
>> if ($page_offset ne "") {
>> -   return $page_offset;
>> +   return hex($page_offset);
>> }
>> }
>> system("rm -f $tmp_file");
>> }
>>
>> foreach my $config_file (@config_files) {
>> -   $config_file =~ s/\R*//g;
>> +   chomp $config_file;
>> $page_offset = parse_kernel_config_file($config_file);
>> if ($page_offset ne "") {
>> -   return $page_offset;
>> +   return hex($page_offset);
>> }
>> }
>>
>> printf STDERR "\nFailed to parse kernel config files\n";
>> printf STDERR "*** NOTE ***\n";
>> -   printf STDERR "Falling back to PAGE_OFFSET = %s\n\n", 
>> $default_offset;
>> +   printf STDERR "Falling back to PAGE_OFFSET = 0x%x\n\n", 
>> $default_offset;
>
> Better use the '#' flag with the 'x' conversion specifier:
> perl -e 'my $default_offset = hex("0xc000");printf "%#x\n", 
> $default_offset'
> 0xc000
>
>>
>> return $default_offset;
>>  }
>> --
>> 2.14.3
>>
>> Thanks,
>> Kaiwan.
>>
>>> thanks,
>>> Tobin.

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

2017-12-04 Thread Kaiwan N Billimoria

Sure, thanks Alexander..
Tobin, request you to pl make the change while merging, thanks..


Thanks & Regards,
Kaiwan.


On Mon, Dec 4, 2017 at 6:07 PM, Alexander Kapshuk
 wrote:
> On Mon, Dec 4, 2017 at 12:20 PM,   wrote:
>> On Mon, 2017-12-04 at 19:21 +1100, Tobin C. Harding wrote:
>>> On Mon, Dec 04, 2017 at 10:51:53AM +0530, Kaiwan N Billimoria wrote:
>>> > > ---
>>> > diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
>>> > index 9906dcf8b807..260b52e456f1 100755
>>> > --- a/scripts/leaking_addresses.pl
>>> > +++ b/scripts/leaking_addresses.pl
>>> > @@ -266,7 +266,7 @@ sub is_false_positive
>>> >  sub is_false_positive_ix86_32
>>> >  {
>>> > my ($match) = @_;
>>> > -   state $page_offset = eval get_page_offset(); # only gets called 
>>> > once
>>> > +   state $page_offset = hex get_page_offset(); # only gets called 
>>> > once
>>>
>>> I don't think this is valid ;) I meant use hex() to convert the string
>>> to an int so it doesn't throw the warning (inside get_page_offset()).
>>
>> Yup, got it, thanks   :-p
>> Combined patch below:
>>
>>
>> ---
>>  scripts/leaking_addresses.pl | 13 ++---
>>  1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
>> index 9906dcf8b807..a595a2c66b12 100755
>> --- a/scripts/leaking_addresses.pl
>> +++ b/scripts/leaking_addresses.pl
>> @@ -266,8 +266,7 @@ sub is_false_positive
>>  sub is_false_positive_ix86_32
>>  {
>> my ($match) = @_;
>> -   state $page_offset = eval get_page_offset(); # only gets called once
>> -
>> +   state $page_offset = get_page_offset(); # only gets called once
>> if ($match =~ '\b(0x)?(f|F){8}\b') {
>> return 1;
>> }
>> @@ -283,7 +282,7 @@ sub is_false_positive_ix86_32
>>  sub get_page_offset
>>  {
>> my $page_offset;
>> -   my $default_offset = "0xc000";
>> +   my $default_offset = hex("0xc000");
>> my @config_files;
>>
>> # Allow --page-offset-32bit to override.
>> @@ -306,23 +305,23 @@ sub get_page_offset
>> } else {
>> $page_offset = parse_kernel_config_file($tmp_file);
>> if ($page_offset ne "") {
>> -   return $page_offset;
>> +   return hex($page_offset);
>> }
>> }
>> system("rm -f $tmp_file");
>> }
>>
>> foreach my $config_file (@config_files) {
>> -   $config_file =~ s/\R*//g;
>> +   chomp $config_file;
>> $page_offset = parse_kernel_config_file($config_file);
>> if ($page_offset ne "") {
>> -   return $page_offset;
>> +   return hex($page_offset);
>> }
>> }
>>
>> printf STDERR "\nFailed to parse kernel config files\n";
>> printf STDERR "*** NOTE ***\n";
>> -   printf STDERR "Falling back to PAGE_OFFSET = %s\n\n", 
>> $default_offset;
>> +   printf STDERR "Falling back to PAGE_OFFSET = 0x%x\n\n", 
>> $default_offset;
>
> Better use the '#' flag with the 'x' conversion specifier:
> perl -e 'my $default_offset = hex("0xc000");printf "%#x\n", 
> $default_offset'
> 0xc000
>
>>
>> return $default_offset;
>>  }
>> --
>> 2.14.3
>>
>> Thanks,
>> Kaiwan.
>>
>>> thanks,
>>> Tobin.

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

2017-12-03 Thread Kaiwan N Billimoria

> On Mon, Dec 4, 2017 at 10:25 AM, Tobin C. Harding  wrote:
>>
>> > With the 'eval', no warning, it's fine.
>>
>> Why not use hex()?
>
>> >
>> > foreach my $config_file (@config_files) {
>> > +   $config_file =~ s/\R*//g;
>>
>> Is there some reason you don't use chomp()?
>

Wrt your suggestions:

---
diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
index 9906dcf8b807..260b52e456f1 100755
--- a/scripts/leaking_addresses.pl
+++ b/scripts/leaking_addresses.pl
@@ -266,7 +266,7 @@ sub is_false_positive
 sub is_false_positive_ix86_32
 {
my ($match) = @_;
-   state $page_offset = eval get_page_offset(); # only gets called once
+   state $page_offset = hex get_page_offset(); # only gets called once

if ($match =~ '\b(0x)?(f|F){8}\b') {
return 1;
@@ -313,7 +313,7 @@ sub get_page_offset
}

foreach my $config_file (@config_files) {
-   $config_file =~ s/\R*//g;
+   chomp $config_file;
$page_offset = parse_kernel_config_file($config_file);
if ($page_offset ne "") {
return $page_offset;


Thanks & Regards,
Kaiwan.

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

2017-12-03 Thread Kaiwan N Billimoria

> On Mon, Dec 4, 2017 at 10:25 AM, Tobin C. Harding  wrote:
>>
>> > With the 'eval', no warning, it's fine.
>>
>> Why not use hex()?
>
>> >
>> > foreach my $config_file (@config_files) {
>> > +   $config_file =~ s/\R*//g;
>>
>> Is there some reason you don't use chomp()?
>

Wrt your suggestions:

---
diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
index 9906dcf8b807..260b52e456f1 100755
--- a/scripts/leaking_addresses.pl
+++ b/scripts/leaking_addresses.pl
@@ -266,7 +266,7 @@ sub is_false_positive
 sub is_false_positive_ix86_32
 {
my ($match) = @_;
-   state $page_offset = eval get_page_offset(); # only gets called once
+   state $page_offset = hex get_page_offset(); # only gets called once

if ($match =~ '\b(0x)?(f|F){8}\b') {
return 1;
@@ -313,7 +313,7 @@ sub get_page_offset
}

foreach my $config_file (@config_files) {
-   $config_file =~ s/\R*//g;
+   chomp $config_file;
$page_offset = parse_kernel_config_file($config_file);
if ($page_offset ne "") {
return $page_offset;


Thanks & Regards,
Kaiwan.

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

2017-12-03 Thread Kaiwan N Billimoria

On Mon, Dec 4, 2017 at 10:25 AM, Tobin C. Harding  wrote:
>
> > With the 'eval', no warning, it's fine.
>
> Why not use hex()?

> >
> > foreach my $config_file (@config_files) {
> > +   $config_file =~ s/\R*//g;
>
> Is there some reason you don't use chomp()?

Tobin, now you can see that I'm indeed a Perl newbie noob! :-)
Will follow your suggestions.. indeed, feel free to go ahead with Perl-stuff
that's more appropriate than my (very amateur) perl!

thanks, Kaiwan.

> thanks,
> Tobin.

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

2017-12-03 Thread Kaiwan N Billimoria

On Mon, Dec 4, 2017 at 10:25 AM, Tobin C. Harding  wrote:
>
> > With the 'eval', no warning, it's fine.
>
> Why not use hex()?

> >
> > foreach my $config_file (@config_files) {
> > +   $config_file =~ s/\R*//g;
>
> Is there some reason you don't use chomp()?

Tobin, now you can see that I'm indeed a Perl newbie noob! :-)
Will follow your suggestions.. indeed, feel free to go ahead with Perl-stuff
that's more appropriate than my (very amateur) perl!

thanks, Kaiwan.

> thanks,
> Tobin.

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

2017-12-01 Thread Kaiwan N Billimoria

Hi,

Pl see my re inline below..
Will also follow up this mail with a patch with (minor) fixes for the
last one Tobin sent, and,
hopefully, that should mostly have the whole thing done (for now at least!)..

Thanks,
Kaiwan.

On Thu, Nov 30, 2017 at 2:18 AM, Tobin C. Harding <m...@tobin.cc> wrote:
> On Wed, Nov 29, 2017 at 04:32:54PM +0530, Kaiwan N Billimoria wrote:

>> This "fallback to 0xc000" I don't really agree with.
>> Obviously, there are platforms out there that do not use a PAGE_OFFSET value 
>> of
>> 0xc000. So I think that defaulting to this is kinda presumptive;
>> much better, IMHO,
>> if we just fail and ask the user to pass the "correct" PAGE_OFFSET value via
>> the '--page-offset-32bit=' option switch.
>> What do you say?
>
> If we fallback to some sane value (it does not have to be 0xc000
> but that seems the most obvious) then the script has more chance of
> running by default. Why do I think it is better to run by default even
> with the wrong virtual address spilt, well since the correct value is
> basically just eliminating false positives (non-kernel addresses) it
> seems more right to run by default with extra false positives than to
> fail and place demands on the user. This will be especially useful if we
> get the script running in any continuous integration tools.
>
> We should definitely be noisy if we fallback to the default value
> though.

Yes, that's a valid argument. Will go with this..

> I just tried to save and apply it on my end and it works. How are you
> saving it? What email client are you using? Perhaps try to create a
> simple patch yourself, email to yourself, save it and apply it to a
> clean branch.
Huh.. wierd issues on my end, I guess.. will sort it out, thanks.

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

2017-12-01 Thread Kaiwan N Billimoria

Hi,

Pl see my re inline below..
Will also follow up this mail with a patch with (minor) fixes for the
last one Tobin sent, and,
hopefully, that should mostly have the whole thing done (for now at least!)..

Thanks,
Kaiwan.

On Thu, Nov 30, 2017 at 2:18 AM, Tobin C. Harding  wrote:
> On Wed, Nov 29, 2017 at 04:32:54PM +0530, Kaiwan N Billimoria wrote:

>> This "fallback to 0xc000" I don't really agree with.
>> Obviously, there are platforms out there that do not use a PAGE_OFFSET value 
>> of
>> 0xc000. So I think that defaulting to this is kinda presumptive;
>> much better, IMHO,
>> if we just fail and ask the user to pass the "correct" PAGE_OFFSET value via
>> the '--page-offset-32bit=' option switch.
>> What do you say?
>
> If we fallback to some sane value (it does not have to be 0xc000
> but that seems the most obvious) then the script has more chance of
> running by default. Why do I think it is better to run by default even
> with the wrong virtual address spilt, well since the correct value is
> basically just eliminating false positives (non-kernel addresses) it
> seems more right to run by default with extra false positives than to
> fail and place demands on the user. This will be especially useful if we
> get the script running in any continuous integration tools.
>
> We should definitely be noisy if we fallback to the default value
> though.

Yes, that's a valid argument. Will go with this..

> I just tried to save and apply it on my end and it works. How are you
> saving it? What email client are you using? Perhaps try to create a
> simple patch yourself, email to yourself, save it and apply it to a
> clean branch.
Huh.. wierd issues on my end, I guess.. will sort it out, thanks.

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

2017-11-29 Thread Kaiwan N Billimoria

Hi,

On Wed, Nov 29, 2017 at 3:46 PM, Tobin C. Harding  wrote:
> On Wed, Nov 29, 2017 at 09:59:59AM +0200, Alexander Kapshuk wrote:
>> On Tue, Nov 28, 2017 at 11:10 PM, Tobin C. Harding  wrote:
>> > On Tue, Nov 28, 2017 at 03:16:24PM +0200, Alexander Kapshuk wrote:
>> >> On Tue, Nov 28, 2017 at 8:32 AM, Tobin C. Harding  wrote:
>> >> >  Options:
>> >> >
>> >> > -   -o, --output-raw=  Save results for future processing.
>> >> > -   -i, --input-raw=   Read results from file instead of 
>> >> > scanning.
>> >> > -   --rawShow 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, --versionDisplay this help and exit.
>> >> > +   -o, --output-raw= Save results for future 
>> >> > processing.
>> >> > +   -i, --input-raw=  Read results from file instead 
>> >> > of scanning.
>> >> > + --raw   Show raw results (default).
>> >> > + --suppress-dmesgDo not show dmesg results.
>> >> > + --squash-by-pathShow one result per unique 
>> >> > path.
>> >> > + --squash-by-filenameShow one result per unique 
>> >> > filename.
>> >> > +   --page-offset-32bit=   PAGE_OFFSET value (for 32-bit 
>> >> > kernels).
>> >> > +   --kernel-config-file= Kernel configuration file (e.g 
>> >> > /boot/config)
>> >> > +   -d, --debug Display debugging output.
>> >> > +   -h, --help, --version   Display this help and exit.
>> >> >
Thanks for the whitespace fixes..

>> >>
>> >> Get_page_offset attempts to build a list of config files, which are
>> >> then passed into the parsing function for further processing.
>> >> This splits up the code to do with the config files between
>> >> get_page_offset() and parse_kernel_config_file().
>> >> May I suggest putting the kernel config file processing code into the
>> >> parse_kernel_config_file() instead, and let the parsing function
>> >> handle the config files and either return the page_offset or an empty
>> >> string.
>> >>
>> >> See below for the proposed implementation.

Thanks Alexander..

>> >
>> > Nice, this is much better! Thanks.
>> >
>> >> Apologies for the absence of indentation.
>> >
>> > Re-posting with indentation, comments in line.
>> >
>> >> Disclaimer: I did not test-run the code being proposed.
>> >
>> > I also did not test my comments ;)
>> >
>> >> sub get_page_offset
>> >> {
>> >>   my $default_offset = "0xc000";
>> >>   my $page_offset;
>> >>
>> >>   # Allow --page-offset-32bit to over ride.
>> >>   if ($page_offset_32bit != 0) {
>> >>   return $page_offset_32bit;
>> >>   }
>> >>
>> >>   $page_offset = parse_kernel_config_file();
>> >>   if ($page_offset ne "") {
>> >>   return $page_offset
>> >>   }
>> >>
>> >>   printf STDERR "Failed to parse kernel config files\n";
>> >>   printf STDERR "Falling back to %s\n", $default_offset;
>> >>
>> >>   return $default_offset;

This "fallback to 0xc000" I don't really agree with.
Obviously, there are platforms out there that do not use a PAGE_OFFSET value of
0xc000. So I think that defaulting to this is kinda presumptive;
much better, IMHO,
if we just fail and ask the user to pass the "correct" PAGE_OFFSET value via
the '--page-offset-32bit=' option switch.
What do you say?

>> >> }
>> >>

>> > perhaps
>> > my $tmpkconf = '/tmp/tmpkconf';
>>
>> my $tmpkconf is almost as long as /tmp/tmpkconf. The name of the tmp
>> file is self explanatory.
>> Using a variable instead of the filename in this particular context is
>> a matter of personal preference. If you prefer to use the variable
>> here, it's your call.
>
> I'm a stickler for no const strings or magic numbers but it's Kaiwan's
> patch, if he puts it in with const strings I'll apply it as is :)

I'd say in this case it's self-evident; IMO, we could leave it as a
const string..

>> >
>> >>   if (system("gunzip < /proc/config.gz > /tmp/tmpkconf") == 
>> >> 0) {
>> >>   push @config_files, "/tmp/tmpkconf";
>> >>   }
>> >>   }
>> >
>> > Won't there only ever be a single config file? So if /proc/config.gz is
>> > readable we could do
>>
>> The code above builds a list of config files.
>> Assigning to @config_files as shown below would wipe out the config
>> files appended to the list so far, would it not?
>> So $tmpkconf needs appending to the list.
>
> You are correct, since the beginning of this function that has been the
> algorithm. My observation is that if /proc/config.gz is present then we
> don't need to parse

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

2017-11-29 Thread Kaiwan N Billimoria

Hi,

On Wed, Nov 29, 2017 at 3:46 PM, Tobin C. Harding  wrote:
> On Wed, Nov 29, 2017 at 09:59:59AM +0200, Alexander Kapshuk wrote:
>> On Tue, Nov 28, 2017 at 11:10 PM, Tobin C. Harding  wrote:
>> > On Tue, Nov 28, 2017 at 03:16:24PM +0200, Alexander Kapshuk wrote:
>> >> On Tue, Nov 28, 2017 at 8:32 AM, Tobin C. Harding  wrote:
>> >> >  Options:
>> >> >
>> >> > -   -o, --output-raw=  Save results for future processing.
>> >> > -   -i, --input-raw=   Read results from file instead of 
>> >> > scanning.
>> >> > -   --rawShow 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, --versionDisplay this help and exit.
>> >> > +   -o, --output-raw= Save results for future 
>> >> > processing.
>> >> > +   -i, --input-raw=  Read results from file instead 
>> >> > of scanning.
>> >> > + --raw   Show raw results (default).
>> >> > + --suppress-dmesgDo not show dmesg results.
>> >> > + --squash-by-pathShow one result per unique 
>> >> > path.
>> >> > + --squash-by-filenameShow one result per unique 
>> >> > filename.
>> >> > +   --page-offset-32bit=   PAGE_OFFSET value (for 32-bit 
>> >> > kernels).
>> >> > +   --kernel-config-file= Kernel configuration file (e.g 
>> >> > /boot/config)
>> >> > +   -d, --debug Display debugging output.
>> >> > +   -h, --help, --version   Display this help and exit.
>> >> >
Thanks for the whitespace fixes..

>> >>
>> >> Get_page_offset attempts to build a list of config files, which are
>> >> then passed into the parsing function for further processing.
>> >> This splits up the code to do with the config files between
>> >> get_page_offset() and parse_kernel_config_file().
>> >> May I suggest putting the kernel config file processing code into the
>> >> parse_kernel_config_file() instead, and let the parsing function
>> >> handle the config files and either return the page_offset or an empty
>> >> string.
>> >>
>> >> See below for the proposed implementation.

Thanks Alexander..

>> >
>> > Nice, this is much better! Thanks.
>> >
>> >> Apologies for the absence of indentation.
>> >
>> > Re-posting with indentation, comments in line.
>> >
>> >> Disclaimer: I did not test-run the code being proposed.
>> >
>> > I also did not test my comments ;)
>> >
>> >> sub get_page_offset
>> >> {
>> >>   my $default_offset = "0xc000";
>> >>   my $page_offset;
>> >>
>> >>   # Allow --page-offset-32bit to over ride.
>> >>   if ($page_offset_32bit != 0) {
>> >>   return $page_offset_32bit;
>> >>   }
>> >>
>> >>   $page_offset = parse_kernel_config_file();
>> >>   if ($page_offset ne "") {
>> >>   return $page_offset
>> >>   }
>> >>
>> >>   printf STDERR "Failed to parse kernel config files\n";
>> >>   printf STDERR "Falling back to %s\n", $default_offset;
>> >>
>> >>   return $default_offset;

This "fallback to 0xc000" I don't really agree with.
Obviously, there are platforms out there that do not use a PAGE_OFFSET value of
0xc000. So I think that defaulting to this is kinda presumptive;
much better, IMHO,
if we just fail and ask the user to pass the "correct" PAGE_OFFSET value via
the '--page-offset-32bit=' option switch.
What do you say?

>> >> }
>> >>

>> > perhaps
>> > my $tmpkconf = '/tmp/tmpkconf';
>>
>> my $tmpkconf is almost as long as /tmp/tmpkconf. The name of the tmp
>> file is self explanatory.
>> Using a variable instead of the filename in this particular context is
>> a matter of personal preference. If you prefer to use the variable
>> here, it's your call.
>
> I'm a stickler for no const strings or magic numbers but it's Kaiwan's
> patch, if he puts it in with const strings I'll apply it as is :)

I'd say in this case it's self-evident; IMO, we could leave it as a
const string..

>> >
>> >>   if (system("gunzip < /proc/config.gz > /tmp/tmpkconf") == 
>> >> 0) {
>> >>   push @config_files, "/tmp/tmpkconf";
>> >>   }
>> >>   }
>> >
>> > Won't there only ever be a single config file? So if /proc/config.gz is
>> > readable we could do
>>
>> The code above builds a list of config files.
>> Assigning to @config_files as shown below would wipe out the config
>> files appended to the list so far, would it not?
>> So $tmpkconf needs appending to the list.
>
> You are correct, since the beginning of this function that has been the
> algorithm. My observation is that if /proc/config.gz is present then we
> don't need to parse the other files so it is better to blow them

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

2017-11-28 Thread Kaiwan N Billimoria

On Tue, Nov 28, 2017 at 11:58 AM, Tobin C. Harding  wrote:
>
> At this stage I am unsure
> how best to convey my ideas back to you. It seems that adding 32 bit x86
> support is making a big enough change to the script that rather than you
> patching and me maintaining we could see it more as co-developing the
> patch.

That's just great! Thanks for the vote of confidence..

>I am in no way trying to take away from your changes, I'm happy
> for all work we end up with being applied with you as the author.
Of course not..

> If you are happy with this, I will email a patch to you (and CC
> kernel-hardening).
Yes..

>You could then look at it and see what things you
> like and what things you don't. Also I have not got access to a 32 bit
> x86 machine so it has not been tested. Once you are happy with it
> perhaps you could re-send as v3 and then I can apply it to the tree with
> you as the author.

Certainly, will do..

> The main aims of my changes to your patch are:
>
> 1. Keep inline with current script as much as possible.
> 2. Keep code as clean as possible (Perl can go to spaghetti really fast).
> 3. Try to keep the architecture stuff un-entangled, assuming more
>architecture specific code will be needed in the future.
>
> And now I'll add a few comments inline intended to add clarity to my
> patch when it comes.

With even a quick glance, I can see you've definitely improved readability and
such...

> Thanks Kaiwan. If any of my methods are unclear or you don't like them
> please do say. I'm hear to learn also, we are shooting for the best
> Kernel possible.
Absolutely! Thanks again..
...

> I played around with these two subs for a while. Let me know what you
> think.
>
> thanks,
> Tobin.

Will look into your patch in detail and revert..

thanks v much,
Kaiwan.

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

2017-11-28 Thread Kaiwan N Billimoria

On Tue, Nov 28, 2017 at 11:58 AM, Tobin C. Harding  wrote:
>
> At this stage I am unsure
> how best to convey my ideas back to you. It seems that adding 32 bit x86
> support is making a big enough change to the script that rather than you
> patching and me maintaining we could see it more as co-developing the
> patch.

That's just great! Thanks for the vote of confidence..

>I am in no way trying to take away from your changes, I'm happy
> for all work we end up with being applied with you as the author.
Of course not..

> If you are happy with this, I will email a patch to you (and CC
> kernel-hardening).
Yes..

>You could then look at it and see what things you
> like and what things you don't. Also I have not got access to a 32 bit
> x86 machine so it has not been tested. Once you are happy with it
> perhaps you could re-send as v3 and then I can apply it to the tree with
> you as the author.

Certainly, will do..

> The main aims of my changes to your patch are:
>
> 1. Keep inline with current script as much as possible.
> 2. Keep code as clean as possible (Perl can go to spaghetti really fast).
> 3. Try to keep the architecture stuff un-entangled, assuming more
>architecture specific code will be needed in the future.
>
> And now I'll add a few comments inline intended to add clarity to my
> patch when it comes.

With even a quick glance, I can see you've definitely improved readability and
such...

> Thanks Kaiwan. If any of my methods are unclear or you don't like them
> please do say. I'm hear to learn also, we are shooting for the best
> Kernel possible.
Absolutely! Thanks again..
...

> I played around with these two subs for a while. Let me know what you
> think.
>
> thanks,
> Tobin.

Will look into your patch in detail and revert..

thanks v much,
Kaiwan.

Re: [kernel-hardening] Re: [RFC 0/3] kallsyms: don't leak address when printing symbol

2017-11-27 Thread Kaiwan N Billimoria

On Tue, Nov 28, 2017 at 7:20 AM, Tobin C. Harding  wrote:
>
> Noob question: how do we _know_ this. In other words how do we know no
> userland tools rely on the current behaviour? No stress to answer Kees,
> this is a pretty general kernel dev question.

Perhaps I'm reading this wrong, but anyway: besides ftrace, kprobes
will require a
symbol-to-address lookup. Specifically, in the function
kprobe_lookup_name() which
in turn invokes kallsyms_lookup_name().
AFAIK, SystemTap (userland) is built on top of the kprobes infrastructure..

thanks,
Kaiwan.

Re: [kernel-hardening] Re: [RFC 0/3] kallsyms: don't leak address when printing symbol

2017-11-27 Thread Kaiwan N Billimoria

On Tue, Nov 28, 2017 at 7:20 AM, Tobin C. Harding  wrote:
>
> Noob question: how do we _know_ this. In other words how do we know no
> userland tools rely on the current behaviour? No stress to answer Kees,
> this is a pretty general kernel dev question.

Perhaps I'm reading this wrong, but anyway: besides ftrace, kprobes
will require a
symbol-to-address lookup. Specifically, in the function
kprobe_lookup_name() which
in turn invokes kallsyms_lookup_name().
AFAIK, SystemTap (userland) is built on top of the kprobes infrastructure..

thanks,
Kaiwan.

Re: [PATCH 2/2] scripts: leaking_addresses: help screen updates

2017-11-23 Thread Kaiwan N Billimoria

On Fri, Nov 24, 2017 at 11:29 AM, Tobin C. Harding  wrote:

> Neither of these patches applies to my tree. Are you editing the diff's
> by hand? I noticed the patches don't end with the version signature, like 
> this:
>
> 
> 2.7.4

I cloned your tree from here: https://github.com/tcharding/linux/tree/leaks
is that right?

One thing i can think of: i have to copy across the script to a
cloud-based 32-bit system, work on it there, copy it back to your tree
on my laptop manually, then i do the 'git diff -r' and basically
copy-paste that. Is this causing issues?

thanks, Kaiwan.

> thanks,
> Tobin.

Re: [PATCH 2/2] scripts: leaking_addresses: help screen updates

2017-11-23 Thread Kaiwan N Billimoria

On Fri, Nov 24, 2017 at 11:29 AM, Tobin C. Harding  wrote:

> Neither of these patches applies to my tree. Are you editing the diff's
> by hand? I noticed the patches don't end with the version signature, like 
> this:
>
> 
> 2.7.4

I cloned your tree from here: https://github.com/tcharding/linux/tree/leaks
is that right?

One thing i can think of: i have to copy across the script to a
cloud-based 32-bit system, work on it there, copy it back to your tree
on my laptop manually, then i do the 'git diff -r' and basically
copy-paste that. Is this causing issues?

thanks, Kaiwan.

> thanks,
> Tobin.

Re: [PATCH v1] scripts: leaking_addresses.pl: add support for 32-bit kernel addresses

2017-11-21 Thread Kaiwan N Billimoria

Thanks Tobin, for your detailed comments.

On Wed, Nov 22, 2017 at 5:29 AM, Tobin C. Harding  wrote:
> You don't typically need [xxx v1] for version 1, the v1 is implicit.
>
> Please use the git brief description prefix that is already in use i.e
>
> leaking_addresses: add support for 32-bit kernel addresses

Ok..

> On Tue, Nov 21, 2017 at 01:28:14PM +0530, kaiwan.billimo...@gmail.com wrote:
>> - support for ARM-32
>
> Sure, we can do this later.

Righto

>
>> - programatically query and set the PAGE_OFFSET based on arch (it's currently
>> hard-coded)
>
> Let's do this straight away, it will be much nicer.

Yes, will work on it..

>> 2. Minor edit:
>> the '--raw', '--suppress-dmesg', '--squash-by-path' and
>> '--squash-by-filename' option switches are only meaningful
>> when the 'input-raw=' option is used. So, indent the 'Help' screen lines
>> to reflect the fact.
>
> This is a different change to the architecture stuff so should be in a
> separate patch. You could do both as a series if you like. Off the top
> of my head I have never seen options output like this, but if you have,
> I'm willing to accept your view. You are correct that the options
> mentioned are only use in conjuncture with '--input-raw' so some way of
> showing this would be nice.

I realize this; so, yeah, will make the next one a series and put this
in the 2nd..

>>
>> +my $bit_size = 64;   # Check 64-bit kernel addresses by default
>
> This global is unnecessary. You already have is_ix86_32() so you can just
> use that.

>From your later comments, I think you see that using this global is necessary.

> Please use kernel coding style
>
> $bit_size = 32;

Ok..

> Bonus points, you uncovered a bug in the current script `if (is_x86_64)`
> was missing the parenthesis!

Yeah :-)

>
>> + if ($match =~ '\b(0x)?(f|F){8}\b') {
>> + return 1;
>> + }
>
> So, may_leak_address() and is_false_positive() are tightly coupled and
> not really that nice. Once we add 32 bit support it gets worse. Going
> forwards, we can either add your 32 bit work then refactor both
> functions or you can refactor them as you add the 32 bit stuff. I'm open
> to either.

Yes I agree. Having said that, I'll leave it on the back burner for now..

>Some things to note
>
> - The mask stuff (all 1's) should have an all 0's regex also.

Well, once we determine the address is >= PAGE_OFFSET, it's
automatically apparent that it's not 0, yes?

> - The mask stuff should probably be closer to the mask stuff for 64
>   bit. It's not immediately apparent a clean way to do this though.
> - It's not immediately apparent if an address less that PAGE_OFFSET is a
>   false positive or should be caught in leaks_address().

Hmm only thing I can think of offhand- on many ARM-32's, the kernel
module space is below
PAGE_OFFSET; we'd have to take that into consideration of course.
Anything else < PAGE_OFFSET and a kernel address? Anyone?

> - Do we need 32 bit equivalents for
>
> if ($line =~ '\bKEY=[[:xdigit:]]{14} [[:xdigit:]]{16} 
> [[:xdigit:]]{16}\b' or
> $line =~ '\b[[:xdigit:]]{14} [[:xdigit:]]{16} 
> [[:xdigit:]]{16}\b') {
>
Ok am unclear on what exactly the above achieves.. could you pl throw
some light on it, thanks..

>
> Your patch did not apply, the problem looks to be in the code section
> above. You can see that there is no removed line. For next spin please
> check your patch applies on top of the 'leaks' branch (which now
> includes the fix for the bug you found).

Yes, sorry about that; will do..

> I have one more comment that should have been at the top but I did not
> want to confuse things. Typically, the git brief description should be
> limited to 50 characters. If you do decide to split this patch into two
> and use the prefix suggested you may like to change the git brief
> description but don't feel you have to. If you do decide to do this, your
> next patch set will be a version 1 again. I may be wrong but I never
> increment a patch version if the subject line changes (excluding
> contents of [] ).

Right. I plan to send the next one as a 2 patch series; will keep the
git prefix you suggest
(and as Sub changes, will not label the version).
>
> thanks,
> Tobin.

Thanks,
Kaiwan.

Re: [PATCH v1] scripts: leaking_addresses.pl: add support for 32-bit kernel addresses

2017-11-21 Thread Kaiwan N Billimoria

Thanks Tobin, for your detailed comments.

On Wed, Nov 22, 2017 at 5:29 AM, Tobin C. Harding  wrote:
> You don't typically need [xxx v1] for version 1, the v1 is implicit.
>
> Please use the git brief description prefix that is already in use i.e
>
> leaking_addresses: add support for 32-bit kernel addresses

Ok..

> On Tue, Nov 21, 2017 at 01:28:14PM +0530, kaiwan.billimo...@gmail.com wrote:
>> - support for ARM-32
>
> Sure, we can do this later.

Righto

>
>> - programatically query and set the PAGE_OFFSET based on arch (it's currently
>> hard-coded)
>
> Let's do this straight away, it will be much nicer.

Yes, will work on it..

>> 2. Minor edit:
>> the '--raw', '--suppress-dmesg', '--squash-by-path' and
>> '--squash-by-filename' option switches are only meaningful
>> when the 'input-raw=' option is used. So, indent the 'Help' screen lines
>> to reflect the fact.
>
> This is a different change to the architecture stuff so should be in a
> separate patch. You could do both as a series if you like. Off the top
> of my head I have never seen options output like this, but if you have,
> I'm willing to accept your view. You are correct that the options
> mentioned are only use in conjuncture with '--input-raw' so some way of
> showing this would be nice.

I realize this; so, yeah, will make the next one a series and put this
in the 2nd..

>>
>> +my $bit_size = 64;   # Check 64-bit kernel addresses by default
>
> This global is unnecessary. You already have is_ix86_32() so you can just
> use that.

>From your later comments, I think you see that using this global is necessary.

> Please use kernel coding style
>
> $bit_size = 32;

Ok..

> Bonus points, you uncovered a bug in the current script `if (is_x86_64)`
> was missing the parenthesis!

Yeah :-)

>
>> + if ($match =~ '\b(0x)?(f|F){8}\b') {
>> + return 1;
>> + }
>
> So, may_leak_address() and is_false_positive() are tightly coupled and
> not really that nice. Once we add 32 bit support it gets worse. Going
> forwards, we can either add your 32 bit work then refactor both
> functions or you can refactor them as you add the 32 bit stuff. I'm open
> to either.

Yes I agree. Having said that, I'll leave it on the back burner for now..

>Some things to note
>
> - The mask stuff (all 1's) should have an all 0's regex also.

Well, once we determine the address is >= PAGE_OFFSET, it's
automatically apparent that it's not 0, yes?

> - The mask stuff should probably be closer to the mask stuff for 64
>   bit. It's not immediately apparent a clean way to do this though.
> - It's not immediately apparent if an address less that PAGE_OFFSET is a
>   false positive or should be caught in leaks_address().

Hmm only thing I can think of offhand- on many ARM-32's, the kernel
module space is below
PAGE_OFFSET; we'd have to take that into consideration of course.
Anything else < PAGE_OFFSET and a kernel address? Anyone?

> - Do we need 32 bit equivalents for
>
> if ($line =~ '\bKEY=[[:xdigit:]]{14} [[:xdigit:]]{16} 
> [[:xdigit:]]{16}\b' or
> $line =~ '\b[[:xdigit:]]{14} [[:xdigit:]]{16} 
> [[:xdigit:]]{16}\b') {
>
Ok am unclear on what exactly the above achieves.. could you pl throw
some light on it, thanks..

>
> Your patch did not apply, the problem looks to be in the code section
> above. You can see that there is no removed line. For next spin please
> check your patch applies on top of the 'leaks' branch (which now
> includes the fix for the bug you found).

Yes, sorry about that; will do..

> I have one more comment that should have been at the top but I did not
> want to confuse things. Typically, the git brief description should be
> limited to 50 characters. If you do decide to split this patch into two
> and use the prefix suggested you may like to change the git brief
> description but don't feel you have to. If you do decide to do this, your
> next patch set will be a version 1 again. I may be wrong but I never
> increment a patch version if the subject line changes (excluding
> contents of [] ).

Right. I plan to send the next one as a 2 patch series; will keep the
git prefix you suggest
(and as Sub changes, will not label the version).
>
> thanks,
> Tobin.

Thanks,
Kaiwan.

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

2017-11-12 Thread Kaiwan N Billimoria

On Mon, Nov 13, 2017 at 11:38 AM, Tobin C. Harding  wrote:
> On Mon, Nov 13, 2017 at 11:16:28AM +0530, kaiwan.billimo...@gmail.com wrote:
>> On Mon, 2017-11-13 at 09:21 +1100, Tobin C. Harding wrote:
>> > On Fri, Nov 10, 2017 at 07:26:34PM +0530, kaiwan.billimo...@gmail.com
>> >  wrote:
>> > > On Tue, 2017-11-07 at 21:32 +1100, Tobin C. Harding wrote:
>> > > > Currently we are leaking addresses from the kernel to user space.
>> > > > This
...
>
> So, Linus has requested that I set up a tree for the development of
> this. I have to work out the details of how to do that and then I'll
> email you so you can get the pull the current version. I can then take
> your patch via LKML as per usual.
>
Super. Thanks.

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

2017-11-12 Thread Kaiwan N Billimoria

On Mon, Nov 13, 2017 at 11:38 AM, Tobin C. Harding  wrote:
> On Mon, Nov 13, 2017 at 11:16:28AM +0530, kaiwan.billimo...@gmail.com wrote:
>> On Mon, 2017-11-13 at 09:21 +1100, Tobin C. Harding wrote:
>> > On Fri, Nov 10, 2017 at 07:26:34PM +0530, kaiwan.billimo...@gmail.com
>> >  wrote:
>> > > On Tue, 2017-11-07 at 21:32 +1100, Tobin C. Harding wrote:
>> > > > Currently we are leaking addresses from the kernel to user space.
>> > > > This
...
>
> So, Linus has requested that I set up a tree for the development of
> this. I have to work out the details of how to do that and then I'll
> email you so you can get the pull the current version. I can then take
> your patch via LKML as per usual.
>
Super. Thanks.

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

2017-11-12 Thread Kaiwan N Billimoria

On Mon, Nov 13, 2017 at 10:05 AM, Tobin C. Harding  wrote:
> On Mon, Nov 13, 2017 at 06:37:28AM +0300, Kirill A. Shutemov wrote:
>> On Mon, Nov 13, 2017 at 10:06:46AM +1100, Tobin C. Harding wrote:
>> > On Sun, Nov 12, 2017 at 02:10:07AM +0300, Kirill A. Shutemov wrote:
...
>> >
>> > Thanks for the link. So it looks like we need to refactor the kernel
>> > address regular expression into a function that takes into account the
>> > machine architecture and the number of page table levels. We will need
>> > to add this to the false positive checks also.
>> >
>> > > Not sure if we care. It won't work too for other 64-bit architectrues 
>> > > that
>> > > have more than 256TB of virtual address space.
>> >
>> > Is this because of the virtual memory map?
>>
>> On x86 direct mapping is the nearest thing we have to userspace.
>>
>> > Did you mean 512TB?
>>
>> No, I mean 256TB.
>>
>> You have all kernel memory in the range from 0x to
>> 0x if you have 256 TB of virtual address space. If you
>> hvae more, some thing might be ouside the range.
>
> Doesn't 4-level paging already limit a system to 64TB of memory? So any
> system better equipped than this will use 5-level paging right? If I am
> totally talking rubbish please ignore, I'm appreciative that you pointed
> out the limitation already. Perhaps we can add a comment to the script
>
> # Script may miss some addresses on machines with more than 256TB of
> # memory.

I think the 256TB is wrt *virtual* address space not physical RAM.

Also, IMHO, the script should 'transparently' take into account the # of paging
levels (instead of the user needing to pass a parameter).
IOW it should be able to detect the same (say, from the .config file) and act
accordingly - in the sense, the regex's and associated logic would accordingly
differ.

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

2017-11-12 Thread Kaiwan N Billimoria

On Mon, Nov 13, 2017 at 10:05 AM, Tobin C. Harding  wrote:
> On Mon, Nov 13, 2017 at 06:37:28AM +0300, Kirill A. Shutemov wrote:
>> On Mon, Nov 13, 2017 at 10:06:46AM +1100, Tobin C. Harding wrote:
>> > On Sun, Nov 12, 2017 at 02:10:07AM +0300, Kirill A. Shutemov wrote:
...
>> >
>> > Thanks for the link. So it looks like we need to refactor the kernel
>> > address regular expression into a function that takes into account the
>> > machine architecture and the number of page table levels. We will need
>> > to add this to the false positive checks also.
>> >
>> > > Not sure if we care. It won't work too for other 64-bit architectrues 
>> > > that
>> > > have more than 256TB of virtual address space.
>> >
>> > Is this because of the virtual memory map?
>>
>> On x86 direct mapping is the nearest thing we have to userspace.
>>
>> > Did you mean 512TB?
>>
>> No, I mean 256TB.
>>
>> You have all kernel memory in the range from 0x to
>> 0x if you have 256 TB of virtual address space. If you
>> hvae more, some thing might be ouside the range.
>
> Doesn't 4-level paging already limit a system to 64TB of memory? So any
> system better equipped than this will use 5-level paging right? If I am
> totally talking rubbish please ignore, I'm appreciative that you pointed
> out the limitation already. Perhaps we can add a comment to the script
>
> # Script may miss some addresses on machines with more than 256TB of
> # memory.

I think the 256TB is wrt *virtual* address space not physical RAM.

Also, IMHO, the script should 'transparently' take into account the # of paging
levels (instead of the user needing to pass a parameter).
IOW it should be able to detect the same (say, from the .config file) and act
accordingly - in the sense, the regex's and associated logic would accordingly
differ.

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

2017-11-09 Thread Kaiwan N Billimoria

>
> Yes, profiling and tracing are similar. And you need to be root to run
> the recording anyway. Thus, as long as root user can read kallsyms,
> trace-cmd should be fine. As trace-cmd requires root access to do any
> ftrace tracing.
>
> -- Steve
Got it, thanks..

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

2017-11-09 Thread Kaiwan N Billimoria

>
> Yes, profiling and tracing are similar. And you need to be root to run
> the recording anyway. Thus, as long as root user can read kallsyms,
> trace-cmd should be fine. As trace-cmd requires root access to do any
> ftrace tracing.
>
> -- Steve
Got it, thanks..

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

2017-11-08 Thread Kaiwan N Billimoria

On Thu, Nov 9, 2017 at 10:13 AM, Kaiwan N Billimoria
<kaiwan.billimo...@gmail.com> wrote:
>> But I don't know if there is anything else than the profiling code
>> that _really_ wants access to /proc/kallsyms in user space as a
>> regular user.
>

Front-ends to ftrace, like trace-cmd?
[from the trace-cmd git repo (Steve Rostedt, pl stand up, pl stand up :-)
Documentation/trace-cmd-restore.1.txt :
...
*-k* kallsyms::
Used with *-c*, it overrides where to read the kallsyms file from.
By default, /proc/kallsyms is used. *-k* will override the file to
read the kallsyms from. This can be useful if the trace.dat file
to create is from another machine. Just copy the /proc/kallsyms
file locally, and use *-k* to point to that file.
...
]

> Am unsure about this, but kprobes? (/jprobes/kretprobes), and by
> extension, wrappers over this infra (like SystemTap)?
> I (hazily) recollect a script I once wrote (years back though) that
> collects kernel virtual addresses off of kallsyms for the purpose of
> passing them to a 'helper' kernel module that uses kprobes. I realize
> that 'modern' kprobes exposes APIs that just require the symbolic name
> & that they're anyway at kernel privilege... but the point is, a
> usermode script was picking up and passing the kernel addresses.
>
> Also, what about kernel addresses exposed via System.map?
> Oh, just checked, it's root rw only.. pl ignore.

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

2017-11-08 Thread Kaiwan N Billimoria

On Thu, Nov 9, 2017 at 10:13 AM, Kaiwan N Billimoria
 wrote:
>> But I don't know if there is anything else than the profiling code
>> that _really_ wants access to /proc/kallsyms in user space as a
>> regular user.
>

Front-ends to ftrace, like trace-cmd?
[from the trace-cmd git repo (Steve Rostedt, pl stand up, pl stand up :-)
Documentation/trace-cmd-restore.1.txt :
...
*-k* kallsyms::
Used with *-c*, it overrides where to read the kallsyms file from.
By default, /proc/kallsyms is used. *-k* will override the file to
read the kallsyms from. This can be useful if the trace.dat file
to create is from another machine. Just copy the /proc/kallsyms
file locally, and use *-k* to point to that file.
...
]

> Am unsure about this, but kprobes? (/jprobes/kretprobes), and by
> extension, wrappers over this infra (like SystemTap)?
> I (hazily) recollect a script I once wrote (years back though) that
> collects kernel virtual addresses off of kallsyms for the purpose of
> passing them to a 'helper' kernel module that uses kprobes. I realize
> that 'modern' kprobes exposes APIs that just require the symbolic name
> & that they're anyway at kernel privilege... but the point is, a
> usermode script was picking up and passing the kernel addresses.
>
> Also, what about kernel addresses exposed via System.map?
> Oh, just checked, it's root rw only.. pl ignore.

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

2017-11-08 Thread Kaiwan N Billimoria

> But I don't know if there is anything else than the profiling code
> that _really_ wants access to /proc/kallsyms in user space as a
> regular user.

Am unsure about this, but kprobes? (/jprobes/kretprobes), and by
extension, wrappers over this infra (like SystemTap)?
I (hazily) recollect a script I once wrote (years back though) that
collects kernel virtual addresses off of kallsyms for the purpose of
passing them to a 'helper' kernel module that uses kprobes. I realize
that 'modern' kprobes exposes APIs that just require the symbolic name
& that they're anyway at kernel privilege... but the point is, a
usermode script was picking up and passing the kernel addresses.

Also, what about kernel addresses exposed via System.map?
Oh, just checked, it's root rw only.. pl ignore.

> That said, that patch also fixes the /proc/kallsyms root check, in
> that now you can do:
>
> sudo head < /proc/kallsyms
>
> and it still shows all zeroes - because the file was *opened* as a
> normal user. That's how UNIX file access security works, and how it is
> fundamentally supposed to work (ie passing a file descriptor to a sui
> program doesn't magically make it gain privileges).

Indeed.

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

2017-11-08 Thread Kaiwan N Billimoria

> But I don't know if there is anything else than the profiling code
> that _really_ wants access to /proc/kallsyms in user space as a
> regular user.

Am unsure about this, but kprobes? (/jprobes/kretprobes), and by
extension, wrappers over this infra (like SystemTap)?
I (hazily) recollect a script I once wrote (years back though) that
collects kernel virtual addresses off of kallsyms for the purpose of
passing them to a 'helper' kernel module that uses kprobes. I realize
that 'modern' kprobes exposes APIs that just require the symbolic name
& that they're anyway at kernel privilege... but the point is, a
usermode script was picking up and passing the kernel addresses.

Also, what about kernel addresses exposed via System.map?
Oh, just checked, it's root rw only.. pl ignore.

> That said, that patch also fixes the /proc/kallsyms root check, in
> that now you can do:
>
> sudo head < /proc/kallsyms
>
> and it still shows all zeroes - because the file was *opened* as a
> normal user. That's how UNIX file access security works, and how it is
> fundamentally supposed to work (ie passing a file descriptor to a sui
> program doesn't magically make it gain privileges).

Indeed.

37 matches

Mail list logo