2024-02-18 17:38:26

by Kees Cook

[permalink] [raw]
Subject: [PATCH] leaking_addresses: Provide mechanism to scan binary files

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 <[email protected]>
---
Cc: "Tobin C. Harding" <[email protected]>
Cc: Tycho Andersen <[email protected]>
Cc: Greg KH <[email protected]>
Cc: Guixiong Wei <[email protected]>
Cc: [email protected]
---
scripts/leaking_addresses.pl | 53 ++++++++++++++++++++++++++++++++++++
1 file changed, 53 insertions(+)

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;
}

--
2.34.1



2024-02-18 18:07:40

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] leaking_addresses: Provide mechanism to scan binary files

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 <[email protected]>
> ---
> Cc: "Tobin C. Harding" <[email protected]>
> Cc: Tycho Andersen <[email protected]>
> Cc: Greg KH <[email protected]>
> Cc: Guixiong Wei <[email protected]>
> Cc: [email protected]
> ---
> scripts/leaking_addresses.pl | 53 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 53 insertions(+)

Nice!

Acked-by: Greg Kroah-Hartman <[email protected]>

2024-02-22 15:31:47

by Tycho Andersen

[permalink] [raw]
Subject: Re: [PATCH] leaking_addresses: Provide mechanism to scan binary files

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 <[email protected]>

Patch itself is

Reviewed-by: Tycho Andersen <[email protected]>

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

2024-02-22 21:01:25

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH] leaking_addresses: Provide mechanism to scan binary files

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 <[email protected]>
>
> Patch itself is
>
> Reviewed-by: Tycho Andersen <[email protected]>
>
> 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

2024-02-22 21:11:17

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH] leaking_addresses: Provide mechanism to scan binary files

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 <[email protected]>
> >
> > Patch itself is
> >
> > Reviewed-by: Tycho Andersen <[email protected]>
> >
> > 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.

--
Kees Cook

2024-02-22 23:50:38

by Tycho Andersen

[permalink] [raw]
Subject: Re: [PATCH] leaking_addresses: Provide mechanism to scan binary files

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

2024-02-29 04:40:55

by Tobin C. Harding

[permalink] [raw]
Subject: Re: [PATCH] leaking_addresses: Provide mechanism to scan binary files

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.

2024-02-29 05:30:21

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH] leaking_addresses: Provide mechanism to scan binary files

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

--
Kees Cook