diff mbox series

leaking_addresses: Provide mechanism to scan binary files

Message ID 20240218173809.work.286-kees@kernel.org
State New
Headers
Series leaking_addresses: Provide mechanism to scan binary files |

Commit Message

Kees Cook Feb. 18, 2024, 5:38 p.m. UTC 
  Introduce --kallsyms argument for scanning binary files for known symbol
addresses. This would have found the exposure in /sys/kernel/notes:

$ scripts/leaking_addresses.pl --kallsyms=<(sudo cat /proc/kallsyms)
/sys/kernel/notes: hypercall_page @ 156
/sys/kernel/notes: xen_hypercall_set_trap_table @ 156
/sys/kernel/notes: startup_xen @ 132

Signed-off-by: Kees Cook <keescook@chromium.org>
---
Cc: "Tobin C. Harding" <me@tobin.cc>
Cc: Tycho Andersen <tycho@tycho.pizza>
Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: Guixiong Wei <guixiongwei@gmail.com>
Cc: linux-hardening@vger.kernel.org
---
 scripts/leaking_addresses.pl | 53 ++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

Comments

Greg KH Feb. 18, 2024, 6:07 p.m. UTC | #1 
On Sun, Feb 18, 2024 at 09:38:12AM -0800, Kees Cook wrote:
> Introduce --kallsyms argument for scanning binary files for known symbol
> addresses. This would have found the exposure in /sys/kernel/notes:
> 
> $ scripts/leaking_addresses.pl --kallsyms=<(sudo cat /proc/kallsyms)
> /sys/kernel/notes: hypercall_page @ 156
> /sys/kernel/notes: xen_hypercall_set_trap_table @ 156
> /sys/kernel/notes: startup_xen @ 132
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> Cc: "Tobin C. Harding" <me@tobin.cc>
> Cc: Tycho Andersen <tycho@tycho.pizza>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Cc: Guixiong Wei <guixiongwei@gmail.com>
> Cc: linux-hardening@vger.kernel.org
> ---
>  scripts/leaking_addresses.pl | 53 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)

Nice!

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Tycho Andersen Feb. 22, 2024, 3:24 p.m. UTC | #2 
Hi Kees,

On Sun, Feb 18, 2024 at 09:38:12AM -0800, Kees Cook wrote:
> Introduce --kallsyms argument for scanning binary files for known symbol
> addresses. This would have found the exposure in /sys/kernel/notes:
> 
> $ scripts/leaking_addresses.pl --kallsyms=<(sudo cat /proc/kallsyms)
> /sys/kernel/notes: hypercall_page @ 156
> /sys/kernel/notes: xen_hypercall_set_trap_table @ 156
> /sys/kernel/notes: startup_xen @ 132
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>

Patch itself is

Reviewed-by: Tycho Andersen <tandersen@netflix.com>

And if you can carry it, that would be great (see below :).

This does bring up some interesting questions. From off-list
discussions with Tobin, I believe he is not particularly interested in
maintaining this script any more. I was never set up to do the PRs
myself, I agreed to be a reviewer to help Tobin out. I'm happy to
adopt it if that makes sense, but I'm curious about the future of the
script:

1. is it useful? (seems like yes if you're adding features)
2. does it make sense to live here as a separate thing? should we
   perhaps run it as part of kselftests or similar? I think that e.g.
   681ff0181bbf ("x86/mm/init/32: Stop printing the virtual memory
   layout") was not discovered with this script, but maybe if we put it
   inline with some other stuff people regularly run more of these would
   fall out? Maybe it makes sense to live somewhere else entirely
   (syzkaller)? I can probably set up some x86/arm64 infra to run it
   regularly, but that won't catch other less popular arches.
3. perl. I'm mostly not a perl programmer, but would be happy to
   rewrite it in python pending the outcome of discussion above.

Thoughts?

Tycho
Kees Cook Feb. 22, 2024, 9 p.m. UTC | #3 
On Thu, Feb 22, 2024 at 08:24:02AM -0700, Tycho Andersen wrote:
> Hi Kees,
> 
> On Sun, Feb 18, 2024 at 09:38:12AM -0800, Kees Cook wrote:
> > Introduce --kallsyms argument for scanning binary files for known symbol
> > addresses. This would have found the exposure in /sys/kernel/notes:
> > 
> > $ scripts/leaking_addresses.pl --kallsyms=<(sudo cat /proc/kallsyms)
> > /sys/kernel/notes: hypercall_page @ 156
> > /sys/kernel/notes: xen_hypercall_set_trap_table @ 156
> > /sys/kernel/notes: startup_xen @ 132
> > 
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> Patch itself is
> 
> Reviewed-by: Tycho Andersen <tandersen@netflix.com>
> 
> And if you can carry it, that would be great (see below :).

Sure!

> This does bring up some interesting questions. From off-list
> discussions with Tobin, I believe he is not particularly interested in
> maintaining this script any more. I was never set up to do the PRs
> myself, I agreed to be a reviewer to help Tobin out. I'm happy to
> adopt it if that makes sense, but I'm curious about the future of the
> script:
> 
> 1. is it useful? (seems like yes if you're adding features)

Yes, LKP runs it as part of 0-day, and it's found leaks in the past[1].
(Though its usage could be improved.)

> 2. does it make sense to live here as a separate thing? should we
>    perhaps run it as part of kselftests or similar? I think that e.g.
>    681ff0181bbf ("x86/mm/init/32: Stop printing the virtual memory
>    layout") was not discovered with this script, but maybe if we put it
>    inline with some other stuff people regularly run more of these would
>    fall out? Maybe it makes sense to live somewhere else entirely
>    (syzkaller)? I can probably set up some x86/arm64 infra to run it
>    regularly, but that won't catch other less popular arches.

We could certainly do that. It would need some work to clean it up,
though -- it seems like it wasn't designed to run as root (which is how
LKP runs it, and likely how at least some CIs would run it).

> 3. perl. I'm mostly not a perl programmer, but would be happy to
>    rewrite it in python pending the outcome of discussion above.

I am not a Perl fan either. It does work as-is, though. Address leaks,
while worth fixing, are relatively low priority over all, so I wouldn't
prioritize a rewrite very highly.

-Kees

[1] https://lore.kernel.org/all/20210103142726.GC30643@xsang-OptiPlex-9020/
Kees Cook Feb. 22, 2024, 9:10 p.m. UTC | #4 
On Thu, Feb 22, 2024 at 01:00:40PM -0800, Kees Cook wrote:
> On Thu, Feb 22, 2024 at 08:24:02AM -0700, Tycho Andersen wrote:
> > Hi Kees,
> > 
> > On Sun, Feb 18, 2024 at 09:38:12AM -0800, Kees Cook wrote:
> > > Introduce --kallsyms argument for scanning binary files for known symbol
> > > addresses. This would have found the exposure in /sys/kernel/notes:
> > > 
> > > $ scripts/leaking_addresses.pl --kallsyms=<(sudo cat /proc/kallsyms)
> > > /sys/kernel/notes: hypercall_page @ 156
> > > /sys/kernel/notes: xen_hypercall_set_trap_table @ 156
> > > /sys/kernel/notes: startup_xen @ 132
> > > 
> > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > 
> > Patch itself is
> > 
> > Reviewed-by: Tycho Andersen <tandersen@netflix.com>
> > 
> > And if you can carry it, that would be great (see below :).
> 
> Sure!
> 
> > This does bring up some interesting questions. From off-list
> > discussions with Tobin, I believe he is not particularly interested in
> > maintaining this script any more. I was never set up to do the PRs
> > myself, I agreed to be a reviewer to help Tobin out. I'm happy to
> > adopt it if that makes sense, but I'm curious about the future of the
> > script:
> > 
> > 1. is it useful? (seems like yes if you're adding features)
> 
> Yes, LKP runs it as part of 0-day, and it's found leaks in the past[1].
> (Though its usage could be improved.)
> 
> > 2. does it make sense to live here as a separate thing? should we
> >    perhaps run it as part of kselftests or similar? I think that e.g.
> >    681ff0181bbf ("x86/mm/init/32: Stop printing the virtual memory
> >    layout") was not discovered with this script, but maybe if we put it
> >    inline with some other stuff people regularly run more of these would
> >    fall out? Maybe it makes sense to live somewhere else entirely
> >    (syzkaller)? I can probably set up some x86/arm64 infra to run it
> >    regularly, but that won't catch other less popular arches.
> 
> We could certainly do that. It would need some work to clean it up,
> though -- it seems like it wasn't designed to run as root (which is how
> LKP runs it, and likely how at least some CIs would run it).

This is wrong -- it's not run as root. It was fall over very badly. I'm
not sure why the CI output looks strange.
Tycho Andersen Feb. 22, 2024, 11:49 p.m. UTC | #5 
On Thu, Feb 22, 2024 at 01:00:40PM -0800, Kees Cook wrote:
> > This does bring up some interesting questions. From off-list
> > discussions with Tobin, I believe he is not particularly interested in
> > maintaining this script any more. I was never set up to do the PRs
> > myself, I agreed to be a reviewer to help Tobin out. I'm happy to
> > adopt it if that makes sense, but I'm curious about the future of the
> > script:
> > 
> > 1. is it useful? (seems like yes if you're adding features)
> 
> Yes, LKP runs it as part of 0-day, and it's found leaks in the past[1].
> (Though its usage could be improved.)

Oh! That is good news :)

> > 2. does it make sense to live here as a separate thing? should we
> >    perhaps run it as part of kselftests or similar? I think that e.g.
> >    681ff0181bbf ("x86/mm/init/32: Stop printing the virtual memory
> >    layout") was not discovered with this script, but maybe if we put it
> >    inline with some other stuff people regularly run more of these would
> >    fall out? Maybe it makes sense to live somewhere else entirely
> >    (syzkaller)? I can probably set up some x86/arm64 infra to run it
> >    regularly, but that won't catch other less popular arches.
> 
> We could certainly do that. It would need some work to clean it up,
> though -- it seems like it wasn't designed to run as root (which is how
> LKP runs it, and likely how at least some CIs would run it).
> 
> > 3. perl. I'm mostly not a perl programmer, but would be happy to
> >    rewrite it in python pending the outcome of discussion above.
> 
> I am not a Perl fan either. It does work as-is, though. Address leaks,
> while worth fixing, are relatively low priority over all, so I wouldn't
> prioritize a rewrite very highly.

Yep, fair enough.

Tycho
Tobin Harding Feb. 29, 2024, 4:40 a.m. UTC | #6 
On Thu, Feb 22, 2024 at 04:49:26PM -0700, Tycho Andersen wrote:
> On Thu, Feb 22, 2024 at 01:00:40PM -0800, Kees Cook wrote:
> > > This does bring up some interesting questions. From off-list
> > > discussions with Tobin, I believe he is not particularly interested in
> > > maintaining this script any more. I was never set up to do the PRs
> > > myself, I agreed to be a reviewer to help Tobin out. I'm happy to
> > > adopt it if that makes sense, but I'm curious about the future of the
> > > script:
> > > 
> > > 1. is it useful? (seems like yes if you're adding features)
> > 
> > Yes, LKP runs it as part of 0-day, and it's found leaks in the past[1].
> > (Though its usage could be improved.)
> 
> Oh! That is good news :)
> 
> > > 2. does it make sense to live here as a separate thing? should we
> > >    perhaps run it as part of kselftests or similar? I think that e.g.
> > >    681ff0181bbf ("x86/mm/init/32: Stop printing the virtual memory
> > >    layout") was not discovered with this script, but maybe if we put it
> > >    inline with some other stuff people regularly run more of these would
> > >    fall out? Maybe it makes sense to live somewhere else entirely
> > >    (syzkaller)? I can probably set up some x86/arm64 infra to run it
> > >    regularly, but that won't catch other less popular arches.
> > 
> > We could certainly do that. It would need some work to clean it up,
> > though -- it seems like it wasn't designed to run as root (which is how
> > LKP runs it, and likely how at least some CIs would run it).
> > 
> > > 3. perl. I'm mostly not a perl programmer, but would be happy to
> > >    rewrite it in python pending the outcome of discussion above.
> > 
> > I am not a Perl fan either. It does work as-is, though. Address leaks,
> > while worth fixing, are relatively low priority over all, so I wouldn't
> > prioritize a rewrite very highly.
> 
> Yep, fair enough.

Thanks for taking this in through your tree Kees! And Tycho for
picking up the pieces I dropped :)

I can help review a re-write if it helps though I don't write Python
daily and I am long way away from kernel work these days so I doubt
I'd be all that much help.

I originally wrote it in Perl because I figured it would be easier to
get past the old guys, six moths later I regretted the decision when I
tried to re-read it.

Thanks for ping'ing me.

All the best,
Tobin.
Kees Cook Feb. 29, 2024, 5:30 a.m. UTC | #7 
On Thu, Feb 29, 2024 at 03:40:13PM +1100, Tobin Harding wrote:
> On Thu, Feb 22, 2024 at 04:49:26PM -0700, Tycho Andersen wrote:
> > On Thu, Feb 22, 2024 at 01:00:40PM -0800, Kees Cook wrote:
> > > > This does bring up some interesting questions. From off-list
> > > > discussions with Tobin, I believe he is not particularly interested in
> > > > maintaining this script any more. I was never set up to do the PRs
> > > > myself, I agreed to be a reviewer to help Tobin out. I'm happy to
> > > > adopt it if that makes sense, but I'm curious about the future of the
> > > > script:
> > > > 
> > > > 1. is it useful? (seems like yes if you're adding features)
> > > 
> > > Yes, LKP runs it as part of 0-day, and it's found leaks in the past[1].
> > > (Though its usage could be improved.)
> > 
> > Oh! That is good news :)
> > 
> > > > 2. does it make sense to live here as a separate thing? should we
> > > >    perhaps run it as part of kselftests or similar? I think that e.g.
> > > >    681ff0181bbf ("x86/mm/init/32: Stop printing the virtual memory
> > > >    layout") was not discovered with this script, but maybe if we put it
> > > >    inline with some other stuff people regularly run more of these would
> > > >    fall out? Maybe it makes sense to live somewhere else entirely
> > > >    (syzkaller)? I can probably set up some x86/arm64 infra to run it
> > > >    regularly, but that won't catch other less popular arches.
> > > 
> > > We could certainly do that. It would need some work to clean it up,
> > > though -- it seems like it wasn't designed to run as root (which is how
> > > LKP runs it, and likely how at least some CIs would run it).
> > > 
> > > > 3. perl. I'm mostly not a perl programmer, but would be happy to
> > > >    rewrite it in python pending the outcome of discussion above.
> > > 
> > > I am not a Perl fan either. It does work as-is, though. Address leaks,
> > > while worth fixing, are relatively low priority over all, so I wouldn't
> > > prioritize a rewrite very highly.
> > 
> > Yep, fair enough.
> 
> Thanks for taking this in through your tree Kees! And Tycho for
> picking up the pieces I dropped :)

Hi! You're welcome; I'm glad I have this script to build on!

> I can help review a re-write if it helps though I don't write Python
> daily and I am long way away from kernel work these days so I doubt
> I'd be all that much help.

No worries. I suppose we could rewrite it in Rust! :)

> I originally wrote it in Perl because I figured it would be easier to
> get past the old guys, six moths later I regretted the decision when I
> tried to re-read it.
> 
> Thanks for ping'ing me.

Good to hear from you!

Take care,

-Kees
diff mbox series

Patch

diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
index e695634d153d..cbaa17c244cc 100755
--- a/scripts/leaking_addresses.pl
+++ b/scripts/leaking_addresses.pl
@@ -51,10 +51,13 @@  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 $kallsyms_file = "";		# Kernel symbols file.
 my $kernel_config_file = "";	# Kernel configuration file.
 my $opt_32bit = 0;		# Scan 32-bit kernel.
 my $page_offset_32bit = 0;	# Page offset for 32-bit kernel.
 
+my @kallsyms = ();
+
 # Skip these absolute paths.
 my @skip_abs = (
 	'/proc/kmsg',
@@ -95,6 +98,8 @@  Options:
 	      --squash-by-path		Show one result per unique path.
 	      --squash-by-filename	Show one result per unique filename.
 	--kernel-config-file=<file>     Kernel configuration file (e.g /boot/config)
+	--kallsyms=<file>		Read kernel symbol addresses from file (for
+						scanning binary files).
 	--32-bit			Scan 32-bit kernel.
 	--page-offset-32-bit=o		Page offset (for 32-bit kernel 0xABCD1234).
 	-d, --debug			Display debugging output.
@@ -115,6 +120,7 @@  GetOptions(
 	'squash-by-path'        => \$squash_by_path,
 	'squash-by-filename'    => \$squash_by_filename,
 	'raw'                   => \$raw,
+	'kallsyms=s'            => \$kallsyms_file,
 	'kernel-config-file=s'	=> \$kernel_config_file,
 	'32-bit'		=> \$opt_32bit,
 	'page-offset-32-bit=o'	=> \$page_offset_32bit,
@@ -155,6 +161,25 @@  if ($output_raw) {
 	select $fh;
 }
 
+if ($kallsyms_file) {
+	open my $fh, '<', $kallsyms_file or die "$0: $kallsyms_file: $!\n";
+	while (<$fh>) {
+		chomp;
+		my @entry = split / /, $_;
+		my $addr_text = $entry[0];
+		# TODO: Why is hex() so impossibly slow?
+		my $addr = hex($addr_text);
+		my $symbol = $entry[2];
+		# Only keep kernel text addresses.
+		if ($addr_text !~ /^0/) {
+			my $long = pack("J", $addr);
+			my $entry = [$long, $symbol];
+			push @kallsyms, $entry;
+		}
+	}
+	close $fh;
+}
+
 parse_dmesg();
 walk(@DIRS);
 
@@ -442,6 +467,25 @@  sub timed_parse_file
 	}
 }
 
+sub parse_binary
+{
+	my ($file) = @_;
+
+	open my $fh, "<:raw", $file or return;
+	local $/ = undef;
+	my $bytes = <$fh>;
+	close $fh;
+
+	foreach my $entry (@kallsyms) {
+		my $addr = $entry->[0];
+		my $symbol = $entry->[1];
+		my $offset = index($bytes, $addr);
+		if ($offset != -1) {
+			printf("$file: $symbol @ $offset\n");
+		}
+	}
+}
+
 sub parse_file
 {
 	my ($file) = @_;
@@ -451,6 +495,15 @@  sub parse_file
 	}
 
 	if (! -T $file) {
+		if ($file =~ m|^/sys/kernel/btf/| or
+		    $file =~ m|^/sys/devices/pci| or
+		    $file =~ m|^/sys/firmware/efi/efivars/| or
+		    $file =~ m|^/proc/bus/pci/|) {
+			return;
+		}
+		if (scalar @kallsyms > 0) {
+			parse_binary($file);
+		}
 		return;
 	}