| Message ID | 1511921105-3647-5-git-send-email-me@tobin.cc |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | hash addresses printed with %p | expand |
On Tue, Nov 28, 2017 at 6:05 PM, Tobin C. Harding <me@tobin.cc> wrote: > > Let's add specifier %px as a > clear, opt-in, way to print a pointer and maintain some level of > isolation from all the other hex integer output within the Kernel. Yes, I like this model. It's easy and it's obvious ("'x' for hex"), and it gives people a good way to say "yes, I really want the actual address as hex" for if/when the hashed pointer doesn't work for some reason. So me likey. And as with the address leaking script, I'd like it even more if you made it a git tree and I'll pull it. Thanks, Linus
On Tue, Nov 28, 2017 at 06:29:02PM -0800, Linus Torvalds wrote: > On Tue, Nov 28, 2017 at 6:05 PM, Tobin C. Harding <me@tobin.cc> wrote: > > > > Let's add specifier %px as a > > clear, opt-in, way to print a pointer and maintain some level of > > isolation from all the other hex integer output within the Kernel. > > Yes, I like this model. It's easy and it's obvious ("'x' for hex"), > and it gives people a good way to say "yes, I really want the actual > address as hex" for if/when the hashed pointer doesn't work for some > reason. > > So me likey. BOOM! > And as with the address leaking script, I'd like it even more if you > made it a git tree and I'll pull it. Pull request to come. thanks, Tobin.
From: Linus Torvalds > Sent: 29 November 2017 02:29 > > On Tue, Nov 28, 2017 at 6:05 PM, Tobin C. Harding <me@tobin.cc> wrote: > > > > Let's add specifier %px as a > > clear, opt-in, way to print a pointer and maintain some level of > > isolation from all the other hex integer output within the Kernel. > > Yes, I like this model. It's easy and it's obvious ("'x' for hex"), > and it gives people a good way to say "yes, I really want the actual > address as hex" for if/when the hashed pointer doesn't work for some > reason. Remind me to change every %p to %px on kernels that support it. Although the absolute values of pointers may not be useful, knowing that two pointer differ by a small amount is useful. It is also useful to know whether pointers are to stack, code, static data or heap. This change to %p is going to make debugging a nightmare. David
On Wed, Nov 29, 2017 at 2:07 AM, David Laight <David.Laight@aculab.com> wrote: > From: Linus Torvalds >> Sent: 29 November 2017 02:29 >> >> On Tue, Nov 28, 2017 at 6:05 PM, Tobin C. Harding <me@tobin.cc> wrote: >> > >> > Let's add specifier %px as a >> > clear, opt-in, way to print a pointer and maintain some level of >> > isolation from all the other hex integer output within the Kernel. >> >> Yes, I like this model. It's easy and it's obvious ("'x' for hex"), >> and it gives people a good way to say "yes, I really want the actual >> address as hex" for if/when the hashed pointer doesn't work for some >> reason. > > Remind me to change every %p to %px on kernels that support it. > > Although the absolute values of pointers may not be useful, knowing > that two pointer differ by a small amount is useful. > It is also useful to know whether pointers are to stack, code, static > data or heap. > > This change to %p is going to make debugging a nightmare. In the future, maybe we could have a knob: unhashed, hashed (default), or zeroed. -Kees
> -----Original Message----- > From: keescook@google.com [mailto:keescook@google.com] On Behalf Of Kees > Cook > Sent: Wednesday, November 29, 2017 2:28 PM > To: David Laight <David.Laight@aculab.com> > Cc: Linus Torvalds <torvalds@linux-foundation.org>; Tobin C. Harding > <me@tobin.cc>; kernel-hardening@lists.openwall.com; Jason A. Donenfeld > <Jason@zx2c4.com>; Theodore Ts'o <tytso@mit.edu>; Paolo Bonzini > <pbonzini@redhat.com>; Tycho Andersen <tycho@tycho.ws>; Roberts, William C > <william.c.roberts@intel.com>; Tejun Heo <tj@kernel.org>; Jordan Glover > <Golden_Miller83@protonmail.ch>; Greg KH <gregkh@linuxfoundation.org>; > Petr Mladek <pmladek@suse.com>; Joe Perches <joe@perches.com>; Ian > Campbell <ijc@hellion.org.uk>; Sergey Senozhatsky > <sergey.senozhatsky@gmail.com>; Catalin Marinas <catalin.marinas@arm.com>; > Will Deacon <wilal.deacon@arm.com>; Steven Rostedt <rostedt@goodmis.org>; > Chris Fries <cfries@google.com>; Dave Weinstein <olorin@google.com>; Daniel > Micay <danielmicay@gmail.com>; Djalal Harouni <tixxdz@gmail.com>; Radim > Krcmár <rkrcmar@redhat.com>; Linux Kernel Mailing List <linux- > kernel@vger.kernel.org>; Network Development <netdev@vger.kernel.org>; > David Miller <davem@davemloft.net>; Stephen Rothwell > <sfr@canb.auug.org.au>; Andrey Ryabinin <aryabinin@virtuozzo.com>; > Alexander Potapenko <glider@google.com>; Dmitry Vyukov > <dvyukov@google.com>; Andrew Morton <akpm@linux-foundation.org> > Subject: Re: [PATCH V11 4/5] vsprintf: add printk specifier %px > > On Wed, Nov 29, 2017 at 2:07 AM, David Laight <David.Laight@aculab.com> > wrote: > > From: Linus Torvalds > >> Sent: 29 November 2017 02:29 > >> > >> On Tue, Nov 28, 2017 at 6:05 PM, Tobin C. Harding <me@tobin.cc> wrote: > >> > > >> > Let's add specifier %px as a > >> > clear, opt-in, way to print a pointer and maintain some level of > >> > isolation from all the other hex integer output within the Kernel. > >> > >> Yes, I like this model. It's easy and it's obvious ("'x' for hex"), > >> and it gives people a good way to say "yes, I really want the actual > >> address as hex" for if/when the hashed pointer doesn't work for some > >> reason. > > > > Remind me to change every %p to %px on kernels that support it. > > > > Although the absolute values of pointers may not be useful, knowing > > that two pointer differ by a small amount is useful. > > It is also useful to know whether pointers are to stack, code, static > > data or heap. > > > > This change to %p is going to make debugging a nightmare. > > In the future, maybe we could have a knob: unhashed, hashed (default), or > zeroed. Isn't that just kptr_restrict and get us right back to the simpler patches I proposed? > > -Kees > > -- > Kees Cook > Pixel Security
On Wed, Nov 29, 2017 at 2:28 PM, Kees Cook <keescook@chromium.org> wrote: > > In the future, maybe we could have a knob: unhashed, hashed (default), > or zeroed. I haven't actually seen a case for that yet. Let's see if there are actually any debug issues at all, and how big they are before worrying about it. Linus
On Wed, 29 Nov 2017 13:05:04 +1100 "Tobin C. Harding" <me@tobin.cc> wrote: > printk specifier %p now hashes all addresses before printing. Sometimes > we need to see the actual unmodified address. This can be achieved using > %lx but then we face the risk that if in future we want to change the > way the Kernel handles printing of pointers we will have to grep through > the already existent 50 000 %lx call sites. Let's add specifier %px as a > clear, opt-in, way to print a pointer and maintain some level of > isolation from all the other hex integer output within the Kernel. > > Add printk specifier %px to print the actual unmodified address. > > ... > > +Unmodified Addresses > +==================== > + > +:: > + > + %px 01234567 or 0123456789abcdef > + > +For printing pointers when you _really_ want to print the address. Please > +consider whether or not you are leaking sensitive information about the > +Kernel layout in memory before printing pointers with %px. %px is > +functionally equivalent to %lx. %px is preferred to %lx because it is more > +uniquely grep'able. If, in the future, we need to modify the way the Kernel > +handles printing pointers it will be nice to be able to find the call > +sites. > + You might want to add a checkpatch rule which emits a stern do-you-really-want-to-do-this warning when someone uses %px.
On Wed, Nov 29, 2017 at 03:20:58PM -0800, Andrew Morton wrote: > On Wed, 29 Nov 2017 13:05:04 +1100 "Tobin C. Harding" <me@tobin.cc> wrote: > > > printk specifier %p now hashes all addresses before printing. Sometimes > > we need to see the actual unmodified address. This can be achieved using > > %lx but then we face the risk that if in future we want to change the > > way the Kernel handles printing of pointers we will have to grep through > > the already existent 50 000 %lx call sites. Let's add specifier %px as a > > clear, opt-in, way to print a pointer and maintain some level of > > isolation from all the other hex integer output within the Kernel. > > > > Add printk specifier %px to print the actual unmodified address. > > > > ... > > > > +Unmodified Addresses > > +==================== > > + > > +:: > > + > > + %px 01234567 or 0123456789abcdef > > + > > +For printing pointers when you _really_ want to print the address. Please > > +consider whether or not you are leaking sensitive information about the > > +Kernel layout in memory before printing pointers with %px. %px is > > +functionally equivalent to %lx. %px is preferred to %lx because it is more > > +uniquely grep'able. If, in the future, we need to modify the way the Kernel > > +handles printing pointers it will be nice to be able to find the call > > +sites. > > + > > You might want to add a checkpatch rule which emits a stern > do-you-really-want-to-do-this warning when someone uses %px. > Oh, nice idea. It has to be a CHECK but right? By stern, you mean use stern language? thanks, Tobin.
On Thu, 2017-11-30 at 10:26 +1100, Tobin C. Harding wrote: > On Wed, Nov 29, 2017 at 03:20:58PM -0800, Andrew Morton wrote: > > On Wed, 29 Nov 2017 13:05:04 +1100 "Tobin C. Harding" <me@tobin.cc> wrote: > > > > > printk specifier %p now hashes all addresses before printing. Sometimes > > > we need to see the actual unmodified address. This can be achieved using > > > %lx but then we face the risk that if in future we want to change the > > > way the Kernel handles printing of pointers we will have to grep through > > > the already existent 50 000 %lx call sites. Let's add specifier %px as a > > > clear, opt-in, way to print a pointer and maintain some level of > > > isolation from all the other hex integer output within the Kernel. > > > > > > Add printk specifier %px to print the actual unmodified address. > > > > > > ... > > > > > > +Unmodified Addresses > > > +==================== > > > + > > > +:: > > > + > > > + %px 01234567 or 0123456789abcdef > > > + > > > +For printing pointers when you _really_ want to print the address. Please > > > +consider whether or not you are leaking sensitive information about the > > > +Kernel layout in memory before printing pointers with %px. %px is > > > +functionally equivalent to %lx. %px is preferred to %lx because it is more > > > +uniquely grep'able. If, in the future, we need to modify the way the Kernel > > > +handles printing pointers it will be nice to be able to find the call > > > +sites. > > > + > > > > You might want to add a checkpatch rule which emits a stern > > do-you-really-want-to-do-this warning when someone uses %px. > > > > Oh, nice idea. It has to be a CHECK but right? No, it has to be something that's not --strict so a WARN would probably be best. > By stern, you mean use stern language? I hope he doesn't mean tweet. Something like: --- scripts/checkpatch.pl | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 0ce249f157a1..9d789cbe7df5 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -5758,21 +5758,40 @@ sub process { defined $stat && $stat =~ /^\+(?![^\{]*\{\s*).*\b(\w+)\s*\(.*$String\s*,/s && $1 !~ /^_*volatile_*$/) { + my $complete_extension = ""; + my $extension = ""; my $bad_extension = ""; my $lc = $stat =~ tr@\n@@; $lc = $lc + $linenr; + my $stat_real; for (my $count = $linenr; $count <= $lc; $count++) { my $fmt = get_quoted_string($lines[$count - 1], raw_line($count, 0)); $fmt =~ s/%%//g; - if ($fmt =~ /(\%[\*\d\.]*p(?![\WFfSsBKRraEhMmIiUDdgVCbGNO]).)/) { - $bad_extension = $1; - last; + while ($fmt =~ /(\%[\*\d\.]*p(\w))/g) { + $complete_extension = $1; + $extension = $2; + if ($extension !~ /[FfSsBKRraEhMmIiUDdgVCbGNOx]/) { + $bad_extension = $complete_extension; + last; + } + if ($extension eq "x") { + if (!defined($stat_real)) { + $stat_real = raw_line($linenr, 0); + for (my $count = $linenr + 1; $count <= $lc; $count++) { + $stat_real = $stat_real . "\n" . raw_line($count, 0); + } + } + WARN("VSPRINTF_POINTER_PX", + "Using vsprintf pointer extension '$complete_extension' exposes kernel address for possible hacking\n" . "$here\n$stat_real\n"); + } } } if ($bad_extension ne "") { - my $stat_real = raw_line($linenr, 0); - for (my $count = $linenr + 1; $count <= $lc; $count++) { - $stat_real = $stat_real . "\n" . raw_line($count, 0); + if (!defined($stat_real)) { + $stat_real = raw_line($linenr, 0); + for (my $count = $linenr + 1; $count <= $lc; $count++) { + $stat_real = $stat_real . "\n" . raw_line($count, 0); + } } WARN("VSPRINTF_POINTER_EXTENSION", "Invalid vsprintf pointer extension '$bad_extension'\n" . "$here\n$stat_real\n");
On Wed, Nov 29, 2017 at 07:58:26PM -0800, Joe Perches wrote: > On Thu, 2017-11-30 at 10:26 +1100, Tobin C. Harding wrote: > > On Wed, Nov 29, 2017 at 03:20:58PM -0800, Andrew Morton wrote: > > > On Wed, 29 Nov 2017 13:05:04 +1100 "Tobin C. Harding" <me@tobin.cc> wrote: > > > > > > > printk specifier %p now hashes all addresses before printing. Sometimes > > > > we need to see the actual unmodified address. This can be achieved using > > > > %lx but then we face the risk that if in future we want to change the > > > > way the Kernel handles printing of pointers we will have to grep through > > > > the already existent 50 000 %lx call sites. Let's add specifier %px as a > > > > clear, opt-in, way to print a pointer and maintain some level of > > > > isolation from all the other hex integer output within the Kernel. > > > > > > > > Add printk specifier %px to print the actual unmodified address. > > > > > > > > ... > > > > > > > > +Unmodified Addresses > > > > +==================== > > > > + > > > > +:: > > > > + > > > > + %px 01234567 or 0123456789abcdef > > > > + > > > > +For printing pointers when you _really_ want to print the address. Please > > > > +consider whether or not you are leaking sensitive information about the > > > > +Kernel layout in memory before printing pointers with %px. %px is > > > > +functionally equivalent to %lx. %px is preferred to %lx because it is more > > > > +uniquely grep'able. If, in the future, we need to modify the way the Kernel > > > > +handles printing pointers it will be nice to be able to find the call > > > > +sites. > > > > + > > > > > > You might want to add a checkpatch rule which emits a stern > > > do-you-really-want-to-do-this warning when someone uses %px. > > > > > > > Oh, nice idea. It has to be a CHECK but right? > > No, it has to be something that's not --strict > so a WARN would probably be best. > > > By stern, you mean use stern language? > > I hope he doesn't mean tweet. /me says tweet tweet (like a bird) > Something like: > --- > scripts/checkpatch.pl | 31 +++++++++++++++++++++++++------ > 1 file changed, 25 insertions(+), 6 deletions(-) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 0ce249f157a1..9d789cbe7df5 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -5758,21 +5758,40 @@ sub process { > defined $stat && > $stat =~ /^\+(?![^\{]*\{\s*).*\b(\w+)\s*\(.*$String\s*,/s && > $1 !~ /^_*volatile_*$/) { > + my $complete_extension = ""; > + my $extension = ""; > my $bad_extension = ""; > my $lc = $stat =~ tr@\n@@; > $lc = $lc + $linenr; > + my $stat_real; > for (my $count = $linenr; $count <= $lc; $count++) { > my $fmt = get_quoted_string($lines[$count - 1], raw_line($count, 0)); > $fmt =~ s/%%//g; > - if ($fmt =~ /(\%[\*\d\.]*p(?![\WFfSsBKRraEhMmIiUDdgVCbGNO]).)/) { > - $bad_extension = $1; > - last; > + while ($fmt =~ /(\%[\*\d\.]*p(\w))/g) { > + $complete_extension = $1; > + $extension = $2; > + if ($extension !~ /[FfSsBKRraEhMmIiUDdgVCbGNOx]/) { > + $bad_extension = $complete_extension; > + last; > + } > + if ($extension eq "x") { > + if (!defined($stat_real)) { > + $stat_real = raw_line($linenr, 0); > + for (my $count = $linenr + 1; $count <= $lc; $count++) { > + $stat_real = $stat_real . "\n" . raw_line($count, 0); > + } > + } > + WARN("VSPRINTF_POINTER_PX", > + "Using vsprintf pointer extension '$complete_extension' exposes kernel address for possible hacking\n" . "$here\n$stat_real\n"); > + } > } > } > if ($bad_extension ne "") { > - my $stat_real = raw_line($linenr, 0); > - for (my $count = $linenr + 1; $count <= $lc; $count++) { > - $stat_real = $stat_real . "\n" . raw_line($count, 0); > + if (!defined($stat_real)) { > + $stat_real = raw_line($linenr, 0); > + for (my $count = $linenr + 1; $count <= $lc; $count++) { > + $stat_real = $stat_real . "\n" . raw_line($count, 0); > + } > } > WARN("VSPRINTF_POINTER_EXTENSION", > "Invalid vsprintf pointer extension '$bad_extension'\n" . "$here\n$stat_real\n"); > Awesome. So moving forward, I should apply this code. Test it, commit it with a log message stating you wrote it and I just tested it then submit the patch, right? thanks, Tobin.
On Thu, 2017-11-30 at 15:18 +1100, Tobin C. Harding wrote: > On Wed, Nov 29, 2017 at 07:58:26PM -0800, Joe Perches wrote: > > On Thu, 2017-11-30 at 10:26 +1100, Tobin C. Harding wrote: > > > On Wed, Nov 29, 2017 at 03:20:58PM -0800, Andrew Morton wrote: > > > > On Wed, 29 Nov 2017 13:05:04 +1100 "Tobin C. Harding" <me@tobin.cc> wrote: > > > > > > > > > printk specifier %p now hashes all addresses before printing. Sometimes > > > > > we need to see the actual unmodified address. This can be achieved using > > > > > %lx but then we face the risk that if in future we want to change the > > > > > way the Kernel handles printing of pointers we will have to grep through > > > > > the already existent 50 000 %lx call sites. Let's add specifier %px as a > > > > > clear, opt-in, way to print a pointer and maintain some level of > > > > > isolation from all the other hex integer output within the Kernel. > > > > > > > > > > Add printk specifier %px to print the actual unmodified address. > > > > > > > > > > ... > > > > > > > > > > +Unmodified Addresses > > > > > +==================== > > > > > + > > > > > +:: > > > > > + > > > > > + %px 01234567 or 0123456789abcdef > > > > > + > > > > > +For printing pointers when you _really_ want to print the address. Please > > > > > +consider whether or not you are leaking sensitive information about the > > > > > +Kernel layout in memory before printing pointers with %px. %px is > > > > > +functionally equivalent to %lx. %px is preferred to %lx because it is more > > > > > +uniquely grep'able. If, in the future, we need to modify the way the Kernel > > > > > +handles printing pointers it will be nice to be able to find the call > > > > > +sites. > > > > > + > > > > > > > > You might want to add a checkpatch rule which emits a stern > > > > do-you-really-want-to-do-this warning when someone uses %px. > > > > > > > > > > Oh, nice idea. It has to be a CHECK but right? > > > > No, it has to be something that's not --strict > > so a WARN would probably be best. > > > > > By stern, you mean use stern language? > > > > I hope he doesn't mean tweet. > > /me says tweet tweet (like a bird) > > > Something like: > > --- > > scripts/checkpatch.pl | 31 +++++++++++++++++++++++++------ > > 1 file changed, 25 insertions(+), 6 deletions(-) > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > > index 0ce249f157a1..9d789cbe7df5 100755 > > --- a/scripts/checkpatch.pl > > +++ b/scripts/checkpatch.pl > > @@ -5758,21 +5758,40 @@ sub process { > > defined $stat && > > $stat =~ /^\+(?![^\{]*\{\s*).*\b(\w+)\s*\(.*$String\s*,/s && > > $1 !~ /^_*volatile_*$/) { > > + my $complete_extension = ""; > > + my $extension = ""; > > my $bad_extension = ""; > > my $lc = $stat =~ tr@\n@@; > > $lc = $lc + $linenr; > > + my $stat_real; > > for (my $count = $linenr; $count <= $lc; $count++) { > > my $fmt = get_quoted_string($lines[$count - 1], raw_line($count, 0)); > > $fmt =~ s/%%//g; > > - if ($fmt =~ /(\%[\*\d\.]*p(?![\WFfSsBKRraEhMmIiUDdgVCbGNO]).)/) { > > - $bad_extension = $1; > > - last; > > + while ($fmt =~ /(\%[\*\d\.]*p(\w))/g) { > > + $complete_extension = $1; > > + $extension = $2; > > + if ($extension !~ /[FfSsBKRraEhMmIiUDdgVCbGNOx]/) { > > + $bad_extension = $complete_extension; > > + last; > > + } > > + if ($extension eq "x") { > > + if (!defined($stat_real)) { > > + $stat_real = raw_line($linenr, 0); > > + for (my $count = $linenr + 1; $count <= $lc; $count++) { > > + $stat_real = $stat_real . "\n" . raw_line($count, 0); > > + } > > + } > > + WARN("VSPRINTF_POINTER_PX", > > + "Using vsprintf pointer extension '$complete_extension' exposes kernel address for possible hacking\n" . "$here\n$stat_real\n"); > > + } > > } > > } > > if ($bad_extension ne "") { > > - my $stat_real = raw_line($linenr, 0); > > - for (my $count = $linenr + 1; $count <= $lc; $count++) { > > - $stat_real = $stat_real . "\n" . raw_line($count, 0); > > + if (!defined($stat_real)) { > > + $stat_real = raw_line($linenr, 0); > > + for (my $count = $linenr + 1; $count <= $lc; $count++) { > > + $stat_real = $stat_real . "\n" . raw_line($count, 0); > > + } > > } > > WARN("VSPRINTF_POINTER_EXTENSION", > > "Invalid vsprintf pointer extension '$bad_extension'\n" . "$here\n$stat_real\n"); > > > > Awesome. So moving forward, I should apply this code. Test it, I didn't sign it and just trivially tested it. So test it locally, see if it doesn't work and check if the wording could be improved. One possible negative is that if the format contains multiple %px uses, then each use is warned. Maybe it should be if ($extension eq "x" && !defined($stat_real)) { ... WARN("VSPRINTF_POINTER_PX", ...) } so that only the first %px is warned. If/when the %px series is applied, then this can go in via whatever tree.
On Wed, Nov 29, 2017 at 08:41:36PM -0800, Joe Perches wrote: > On Thu, 2017-11-30 at 15:18 +1100, Tobin C. Harding wrote: > > On Wed, Nov 29, 2017 at 07:58:26PM -0800, Joe Perches wrote: > > > On Thu, 2017-11-30 at 10:26 +1100, Tobin C. Harding wrote: > > > > On Wed, Nov 29, 2017 at 03:20:58PM -0800, Andrew Morton wrote: > > > > > On Wed, 29 Nov 2017 13:05:04 +1100 "Tobin C. Harding" <me@tobin.cc> wrote: > > > > > > > > > > > printk specifier %p now hashes all addresses before printing. Sometimes > > > > > > we need to see the actual unmodified address. This can be achieved using > > > > > > %lx but then we face the risk that if in future we want to change the > > > > > > way the Kernel handles printing of pointers we will have to grep through > > > > > > the already existent 50 000 %lx call sites. Let's add specifier %px as a > > > > > > clear, opt-in, way to print a pointer and maintain some level of > > > > > > isolation from all the other hex integer output within the Kernel. > > > > > > > > > > > > Add printk specifier %px to print the actual unmodified address. > > > > > > > > > > > > ... > > > > > > > > > > > > +Unmodified Addresses > > > > > > +==================== > > > > > > + > > > > > > +:: > > > > > > + > > > > > > + %px 01234567 or 0123456789abcdef > > > > > > + > > > > > > +For printing pointers when you _really_ want to print the address. Please > > > > > > +consider whether or not you are leaking sensitive information about the > > > > > > +Kernel layout in memory before printing pointers with %px. %px is > > > > > > +functionally equivalent to %lx. %px is preferred to %lx because it is more > > > > > > +uniquely grep'able. If, in the future, we need to modify the way the Kernel > > > > > > +handles printing pointers it will be nice to be able to find the call > > > > > > +sites. > > > > > > + > > > > > > > > > > You might want to add a checkpatch rule which emits a stern > > > > > do-you-really-want-to-do-this warning when someone uses %px. > > > > > > > > > > > > > Oh, nice idea. It has to be a CHECK but right? > > > > > > No, it has to be something that's not --strict > > > so a WARN would probably be best. > > > > > > > By stern, you mean use stern language? > > > > > > I hope he doesn't mean tweet. > > > > /me says tweet tweet (like a bird) > > > > > Something like: > > > --- > > > scripts/checkpatch.pl | 31 +++++++++++++++++++++++++------ > > > 1 file changed, 25 insertions(+), 6 deletions(-) > > > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > > > index 0ce249f157a1..9d789cbe7df5 100755 > > > --- a/scripts/checkpatch.pl > > > +++ b/scripts/checkpatch.pl > > > @@ -5758,21 +5758,40 @@ sub process { > > > defined $stat && > > > $stat =~ /^\+(?![^\{]*\{\s*).*\b(\w+)\s*\(.*$String\s*,/s && > > > $1 !~ /^_*volatile_*$/) { > > > + my $complete_extension = ""; > > > + my $extension = ""; > > > my $bad_extension = ""; > > > my $lc = $stat =~ tr@\n@@; > > > $lc = $lc + $linenr; > > > + my $stat_real; > > > for (my $count = $linenr; $count <= $lc; $count++) { > > > my $fmt = get_quoted_string($lines[$count - 1], raw_line($count, 0)); > > > $fmt =~ s/%%//g; > > > - if ($fmt =~ /(\%[\*\d\.]*p(?![\WFfSsBKRraEhMmIiUDdgVCbGNO]).)/) { > > > - $bad_extension = $1; > > > - last; > > > + while ($fmt =~ /(\%[\*\d\.]*p(\w))/g) { > > > + $complete_extension = $1; > > > + $extension = $2; > > > + if ($extension !~ /[FfSsBKRraEhMmIiUDdgVCbGNOx]/) { > > > + $bad_extension = $complete_extension; > > > + last; > > > + } > > > + if ($extension eq "x") { > > > + if (!defined($stat_real)) { > > > + $stat_real = raw_line($linenr, 0); > > > + for (my $count = $linenr + 1; $count <= $lc; $count++) { > > > + $stat_real = $stat_real . "\n" . raw_line($count, 0); > > > + } > > > + } > > > + WARN("VSPRINTF_POINTER_PX", > > > + "Using vsprintf pointer extension '$complete_extension' exposes kernel address for possible hacking\n" . "$here\n$stat_real\n"); > > > + } > > > } > > > } > > > if ($bad_extension ne "") { > > > - my $stat_real = raw_line($linenr, 0); > > > - for (my $count = $linenr + 1; $count <= $lc; $count++) { > > > - $stat_real = $stat_real . "\n" . raw_line($count, 0); > > > + if (!defined($stat_real)) { > > > + $stat_real = raw_line($linenr, 0); > > > + for (my $count = $linenr + 1; $count <= $lc; $count++) { > > > + $stat_real = $stat_real . "\n" . raw_line($count, 0); > > > + } > > > } > > > WARN("VSPRINTF_POINTER_EXTENSION", > > > "Invalid vsprintf pointer extension '$bad_extension'\n" . "$here\n$stat_real\n"); > > > > > > > Awesome. So moving forward, I should apply this code. Test it, > > I didn't sign it and just trivially tested it. > > So test it locally, see if it doesn't work > and check if the wording could be improved. > > One possible negative is that if the format > contains multiple %px uses, then each use is > warned. > > Maybe it should be > if ($extension eq "x" && !defined($stat_real)) { > ... > WARN("VSPRINTF_POINTER_PX", ...) > } > so that only the first %px is warned. Ok, will do as suggested. > If/when the %px series is applied, then this > can go in via whatever tree. The %px series is in Linus' mainline now. I'll get this stuff to you and Andy for ack'ing (and LKML) soon as its done. thanks, Tobin.
From: Kees Cook > Sent: 29 November 2017 22:28 > On Wed, Nov 29, 2017 at 2:07 AM, David Laight <David.Laight@aculab.com> wrote: > > From: Linus Torvalds > >> Sent: 29 November 2017 02:29 > >> > >> On Tue, Nov 28, 2017 at 6:05 PM, Tobin C. Harding <me@tobin.cc> wrote: > >> > > >> > Let's add specifier %px as a > >> > clear, opt-in, way to print a pointer and maintain some level of > >> > isolation from all the other hex integer output within the Kernel. > >> > >> Yes, I like this model. It's easy and it's obvious ("'x' for hex"), > >> and it gives people a good way to say "yes, I really want the actual > >> address as hex" for if/when the hashed pointer doesn't work for some > >> reason. > > > > Remind me to change every %p to %px on kernels that support it. > > > > Although the absolute values of pointers may not be useful, knowing > > that two pointer differ by a small amount is useful. > > It is also useful to know whether pointers are to stack, code, static > > data or heap. > > > > This change to %p is going to make debugging a nightmare. > > In the future, maybe we could have a knob: unhashed, hashed (default), > or zeroed. Add a 4th, hashed_page+offset. Isn't there already a knob for %pK, bits in the same value could be used. That would make it easy to ensure that %pK is more restructive than %p. David
On 11/30/2017 02:38 AM, David Laight wrote: > From: Kees Cook >> Sent: 29 November 2017 22:28 >> On Wed, Nov 29, 2017 at 2:07 AM, David Laight <David.Laight@aculab.com> wrote: >>> From: Linus Torvalds >>>> Sent: 29 November 2017 02:29 >>>> >>>> On Tue, Nov 28, 2017 at 6:05 PM, Tobin C. Harding <me@tobin.cc> wrote: >>>>> >>>>> Let's add specifier %px as a >>>>> clear, opt-in, way to print a pointer and maintain some level of >>>>> isolation from all the other hex integer output within the Kernel. >>>> >>>> Yes, I like this model. It's easy and it's obvious ("'x' for hex"), >>>> and it gives people a good way to say "yes, I really want the actual >>>> address as hex" for if/when the hashed pointer doesn't work for some >>>> reason. >>> >>> Remind me to change every %p to %px on kernels that support it. >>> >>> Although the absolute values of pointers may not be useful, knowing >>> that two pointer differ by a small amount is useful. >>> It is also useful to know whether pointers are to stack, code, static >>> data or heap. >>> >>> This change to %p is going to make debugging a nightmare. >> >> In the future, maybe we could have a knob: unhashed, hashed (default), >> or zeroed. > > Add a 4th, hashed_page+offset. > > Isn't there already a knob for %pK, bits in the same value could be used. > That would make it easy to ensure that %pK is more restructive than %p. (yeah, I'm kind of behind on this thread.) This kind of option (with default hashed) is what I was just thinking of after having seen a few unhelpful traces. But then the knob might not be changed in time for the traces either. :(
On Tue, Dec 5, 2017 at 1:08 PM, Randy Dunlap <rdunlap@infradead.org> wrote: > > This kind of option (with default hashed) is what I was just thinking of > after having seen a few unhelpful traces. But then the knob might not be > changed in time for the traces either. :( .. I really dislike the idea of such a knob. First off, the traces I've seen that had the new %p behavior, the hashing didn't actually matter AT ALL. The only values that were hashed were values that weren't actually useful for debugging the oops. Secondly, the notion that "we want a unhashed knob for debugging" is exactly the wrong kind of mentality. 99% of all bug reports happen in the wild - not on developer boxes. So by default, those bug reports had better happen with hashing enabled, or it's all entirely pointless. If you have an oops that happens on your own box due to code that you're writing yourself (and expect to debug yourself), then honestly, the hashing is going to be the least of your issues. If you can't find out the bug under those circumstances, and you're confused by the tiny detail of hashing, you're doing something wrong. So the case that matters is when an oops comes from some outside source that won't have turned the knob off anyway. So no. We're not adding a knob. It is fundamentally pointless. It's not like those hex numbers were really helping people anyway. We've turned off most of them on x86 oops reports long ago (and entirely independently of the pointer hashing). Having stared at a lot of oopses in my time, the only hex numbers that tend to be really relevant are (a) the register contents (which aren't %p anyway), and things like the faulting address (which is not, and never has been, %p on x86, but might be on some other architecture). Honestly, the next time anybody says "hashing makes debugging harder", I'm going to require some actual proof of an actual oops where it mattered that a particular value was hashed. Not hand-waving. Not "it surprised and confused me" because it looked different. You'll get used to it. So an actual "this was critical information that mattered for this particular bug, and it was missing due to the hashing of this particular value and debugging was harder in actual reality due to that". Because the actual example I have seen so far, not only didn't the hashing matter AT ALL, most of the _unhashed_ values shouldn't have been there either, and were due to arm still printing stuff that shouldn't have been printed at all and just made the oops more complex and harder to read and report. Linus
Hello, On (12/05/17 13:22), Linus Torvalds wrote: [..] > It's not like those hex numbers were really helping people anyway. > We've turned off most of them on x86 oops reports long ago (and > entirely independently of the pointer hashing). Having stared at a lot > of oopses in my time, the only hex numbers that tend to be really > relevant are (a) the register contents (which aren't %p anyway), and > things like the faulting address (which is not, and never has been, %p > on x86, but might be on some other architecture). I see some %p-s being used in _supposedly_ important output, like arch/x86/mm/fault.c show_fault_oops(struct pt_regs *regs, unsigned long error_code, unsigned long address) ... printk(KERN_CONT " at %p\n", (void *) address); printk(KERN_ALERT "IP: %pS\n", (void *)regs->ip); a quick %p grep gives me the following list: arch/arm/mm/fault.c: pr_alert("pgd = %p\n", mm->pgd); arch/arm64/mm/fault.c: pr_alert("%s pgtable: %luk pages, %u-bit VAs, pgd = %p\n", arch/arm64/mm/fault.c: pr_info_ratelimited("%s[%d]: %s exception: pc=%p sp=%p\n", arch/m68k/mm/fault.c: pr_debug("send_fault_sig: %p,%d,%d\n", siginfo.si_addr, arch/m68k/mm/fault.c: pr_cont(" at virtual address %p\n", siginfo.si_addr); arch/m68k/mm/fault.c: pr_debug("do page fault:\nregs->sr=%#x, regs->pc=%#lx, address=%#lx, %ld, %p\n", arch/microblaze/mm/fault.c: pr_emerg("Page fault in user mode with faulthandler_disabled(), mm = %p\n", arch/mn10300/mm/fault.c: printk(KERN_DEBUG "pgd entry %p: %016Lx\n", arch/mn10300/mm/fault.c: printk(KERN_DEBUG "pmd entry %p: %016Lx\n", arch/mn10300/mm/fault.c: printk(KERN_DEBUG "pte entry %p: %016Lx\n", arch/mn10300/mm/fault.c: printk(KERN_DEBUG "--- do_page_fault(%p,%s:%04lx,%08lx)\n", arch/powerpc/mm/fault.c: " mm=%p\n", arch/sh/mm/fault.c: printk(KERN_ALERT "pgd = %p\n", pgd); arch/unicore32/mm/fault.c: printk(KERN_ALERT "pgd = %p\n", mm->pgd); arch/x86/mm/fault.c: printk(KERN_CONT " at %p\n", (void *) address); arch/x86/mm/fault.c: printk(KERN_ALERT "IP: %pS\n", (void *)regs->ip); arch/x86/mm/fault.c: printk("%s%s[%d]: segfault at %lx ip %p sp %p error %lx", or is it OK to show hashes instead of pgd or pmd pointers? -ss
On Tue, Dec 5, 2017 at 5:36 PM, Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote: > I see some %p-s being used in _supposedly_ important output, > like arch/x86/mm/fault.c > > show_fault_oops(struct pt_regs *regs, unsigned long error_code, > unsigned long address) > ... > printk(KERN_CONT " at %p\n", (void *) address); > printk(KERN_ALERT "IP: %pS\n", (void *)regs->ip); So %pS isn't %p, and shows the symbolic name. But yes, that "at %p" should definitely be %px. In fact, it used to be a "%08lx" - and the value we print out is "unsigned long - but then when we unified the 32- and 64-bit architectures, using "%p" and a cast was a convenient way to unify the 32-bit %08lx and the 16-bit %016lx formats. Will fix. > a quick %p grep gives me the following list: ... > or is it OK to show hashes instead of pgd or pmd pointers? So my gut feel is that those printouts should probably just be removed. They have some very old historical reasons: we've printed out the page directory pointers (and followed the page tables) since at least back in the 1.1.x days. This is from the 1.1.7 patch, back when mm/memory.c was all about x86: + printk(KERN_ALERT "current->tss.cr3 = %08lx, %%cr3 = %08lx\n", + current->tss.cr3, user_esp); + user_esp = ((unsigned long *) user_esp)[address >> 22]; + printk(KERN_ALERT "*pde = %08lx\n", user_esp); so it's more historical than sensible, I think. Linus
On (12/05/17 17:59), Linus Torvalds wrote: [..] > On Tue, Dec 5, 2017 at 5:36 PM, Sergey Senozhatsky > <sergey.senozhatsky.work@gmail.com> wrote: > > I see some %p-s being used in _supposedly_ important output, > > like arch/x86/mm/fault.c > > > > show_fault_oops(struct pt_regs *regs, unsigned long error_code, > > unsigned long address) > > ... > > printk(KERN_CONT " at %p\n", (void *) address); > > printk(KERN_ALERT "IP: %pS\n", (void *)regs->ip); > > So %pS isn't %p, and shows the symbolic name. sure, agreed. by "some %p-s being used" I meant the grep result, not just x86 show_fault_oops(). > But yes, that "at %p" should definitely be %px. more %p grepping [filtering out all `%ps %pf %pb' variants] gives a huge number of print outs that potentially can be broken now arch/x86/kernel/kprobes/core.c: printk(KERN_WARNING "Unrecoverable kprobe detected at %p.\n", arch/x86/kernel/kprobes/core.c: "current sp %p does not match saved sp %p\n", arch/x86/kernel/kprobes/core.c: printk(KERN_ERR "Saved registers for jprobe %p\n", jp); arch/x86/kernel/head_32.S: .asciz "Unknown interrupt or fault at: %p %p %p\n" arch/x86/kernel/irq_32.c: printk(KERN_DEBUG "CPU %u irqstacks, hard=%p soft=%p\n", arch/x86/kernel/smpboot.c: pr_debug("Stack at about %p\n", &cpuid); arch/x86/kernel/traps.c: printk(KERN_EMERG "BUG: stack guard page was hit at %p (stack is %p..%p)\n", so I'm not in position to suggest the removal of those print outs or to decide if those are important at all, just saying that that "I'm confused by pointer values and can't debug" might be more likely that we thought. > So my gut feel is that those printouts should probably just be > removed. They have some very old historical reasons: we've printed out > the page directory pointers (and followed the page tables) since at > least back in the 1.1.x days. This is from the 1.1.7 patch, back when > mm/memory.c was all about x86: I see, thanks. -ss
Hi Linus, On Wed, Dec 6, 2017 at 2:59 AM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Tue, Dec 5, 2017 at 5:36 PM, Sergey Senozhatsky > <sergey.senozhatsky.work@gmail.com> wrote: >> I see some %p-s being used in _supposedly_ important output, >> like arch/x86/mm/fault.c >> >> show_fault_oops(struct pt_regs *regs, unsigned long error_code, >> unsigned long address) >> ... >> printk(KERN_CONT " at %p\n", (void *) address); >> printk(KERN_ALERT "IP: %pS\n", (void *)regs->ip); > > So %pS isn't %p, and shows the symbolic name. If the symbolic name is available. Else it prints the non-hashed pointer value (FTR). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On (12/06/17 09:32), Geert Uytterhoeven wrote: [..] > >> show_fault_oops(struct pt_regs *regs, unsigned long error_code, > >> unsigned long address) > >> ... > >> printk(KERN_CONT " at %p\n", (void *) address); > >> printk(KERN_ALERT "IP: %pS\n", (void *)regs->ip); > > > > So %pS isn't %p, and shows the symbolic name. > > If the symbolic name is available. > Else it prints the non-hashed pointer value (FTR). hm, indeed. and !CONFIG_KALLSYMS config turns %pS/%ps into special_hex_number(). -ss
On Wed, Dec 06, 2017 at 09:32:14AM +0100, Geert Uytterhoeven wrote: > Hi Linus, > > On Wed, Dec 6, 2017 at 2:59 AM, Linus Torvalds > <torvalds@linux-foundation.org> wrote: > > On Tue, Dec 5, 2017 at 5:36 PM, Sergey Senozhatsky > > <sergey.senozhatsky.work@gmail.com> wrote: > >> I see some %p-s being used in _supposedly_ important output, > >> like arch/x86/mm/fault.c > >> > >> show_fault_oops(struct pt_regs *regs, unsigned long error_code, > >> unsigned long address) > >> ... > >> printk(KERN_CONT " at %p\n", (void *) address); > >> printk(KERN_ALERT "IP: %pS\n", (void *)regs->ip); > > > > So %pS isn't %p, and shows the symbolic name. > > If the symbolic name is available. > Else it prints the non-hashed pointer value (FTR). I'm trying to fix this :) [RFC 0/3] kallsyms: don't leak address when printing symbol thanks, Tobin.
On Wed, Dec 06, 2017 at 05:45:37PM +0900, Sergey Senozhatsky wrote: > On (12/06/17 09:32), Geert Uytterhoeven wrote: > [..] > > >> show_fault_oops(struct pt_regs *regs, unsigned long error_code, > > >> unsigned long address) > > >> ... > > >> printk(KERN_CONT " at %p\n", (void *) address); > > >> printk(KERN_ALERT "IP: %pS\n", (void *)regs->ip); > > > > > > So %pS isn't %p, and shows the symbolic name. > > > > If the symbolic name is available. > > Else it prints the non-hashed pointer value (FTR). Well, this [RFC 0/3] kallsyms: don't leak address when printing symbol _trys_ to fix that > hm, indeed. and !CONFIG_KALLSYMS config turns %pS/%ps > into special_hex_number(). But totally misses this :( "<no-sym>" would be better returned when !CONFIG_KALLSYMS, right? thanks, Tobin.
On (12/07/17 16:17), Tobin C. Harding wrote: [..] > > hm, indeed. and !CONFIG_KALLSYMS config turns %pS/%ps > > into special_hex_number(). > > But totally misses this :( > > "<no-sym>" would be better returned when !CONFIG_KALLSYMS, right? I guess I'll take back my comment. I assume there are tons of embedded devices that have !CONFIG_KALLSYMS in 'release' builds, yet those devices still warn/oops sometimes; having pointers/hex numbers is really the only way to make any sense out of backtraces... yet it, basically, means that we are leaking kernel pointers. -ss
diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt index b4e668ac4fe3..aa0a776c817a 100644 --- a/Documentation/printk-formats.txt +++ b/Documentation/printk-formats.txt @@ -49,7 +49,8 @@ Pointer Types Pointers printed without a specifier extension (i.e unadorned %p) are hashed to give a unique identifier without leaking kernel addresses to user -space. On 64 bit machines the first 32 bits are zeroed. +space. On 64 bit machines the first 32 bits are zeroed. If you _really_ +want the address see %px below. :: @@ -106,6 +107,21 @@ For printing kernel pointers which should be hidden from unprivileged users. The behaviour of ``%pK`` depends on the ``kptr_restrict sysctl`` - see Documentation/sysctl/kernel.txt for more details. +Unmodified Addresses +==================== + +:: + + %px 01234567 or 0123456789abcdef + +For printing pointers when you _really_ want to print the address. Please +consider whether or not you are leaking sensitive information about the +Kernel layout in memory before printing pointers with %px. %px is +functionally equivalent to %lx. %px is preferred to %lx because it is more +uniquely grep'able. If, in the future, we need to modify the way the Kernel +handles printing pointers it will be nice to be able to find the call +sites. + Struct Resources ================ diff --git a/lib/vsprintf.c b/lib/vsprintf.c index d69452a0f2fa..d960aead0336 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -1646,6 +1646,20 @@ char *device_node_string(char *buf, char *end, struct device_node *dn, return widen_string(buf, buf - buf_start, end, spec); } +static noinline_for_stack +char *pointer_string(char *buf, char *end, const void *ptr, + struct printf_spec spec) +{ + spec.base = 16; + spec.flags |= SMALL; + if (spec.field_width == -1) { + spec.field_width = 2 * sizeof(ptr); + spec.flags |= ZEROPAD; + } + + return number(buf, end, (unsigned long int)ptr, spec); +} + static bool have_filled_random_ptr_key __read_mostly; static siphash_key_t ptr_key __read_mostly; @@ -1818,6 +1832,8 @@ static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec) * c major compatible string * C full compatible string * + * - 'x' For printing the address. Equivalent to "%lx". + * * ** Please update also Documentation/printk-formats.txt when making changes ** * * Note: The difference between 'S' and 'F' is that on ia64 and ppc64 @@ -1940,6 +1956,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, case 'F': return device_node_string(buf, end, ptr, spec, fmt + 1); } + case 'x': + return pointer_string(buf, end, ptr, spec); } /* default is to _not_ leak addresses, hash before printing */ diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 95cda3ecc66b..040aa79e1d9d 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -5753,7 +5753,7 @@ sub process { for (my $count = $linenr; $count <= $lc; $count++) { my $fmt = get_quoted_string($lines[$count - 1], raw_line($count, 0)); $fmt =~ s/%%//g; - if ($fmt =~ /(\%[\*\d\.]*p(?![\WFfSsBKRraEhMmIiUDdgVCbGNO]).)/) { + if ($fmt =~ /(\%[\*\d\.]*p(?![\WFfSsBKRraEhMmIiUDdgVCbGNOx]).)/) { $bad_extension = $1; last; }
printk specifier %p now hashes all addresses before printing. Sometimes we need to see the actual unmodified address. This can be achieved using %lx but then we face the risk that if in future we want to change the way the Kernel handles printing of pointers we will have to grep through the already existent 50 000 %lx call sites. Let's add specifier %px as a clear, opt-in, way to print a pointer and maintain some level of isolation from all the other hex integer output within the Kernel. Add printk specifier %px to print the actual unmodified address. Signed-off-by: Tobin C. Harding <me@tobin.cc> --- Documentation/printk-formats.txt | 18 +++++++++++++++++- lib/vsprintf.c | 18 ++++++++++++++++++ scripts/checkpatch.pl | 2 +- 3 files changed, 36 insertions(+), 2 deletions(-)