[PATCH V11 4/5] vsprintf: add printk specifier %px

[PATCH V11 4/5] vsprintf: add printk specifier %px

[Tobin C. Harding]

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(-)

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

Re: [PATCH V11 4/5] vsprintf: add printk specifier %px

[Linus Torvalds]

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

Re: [PATCH V11 4/5] vsprintf: add printk specifier %px

[Tobin C. Harding]

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.

RE: [PATCH V11 4/5] vsprintf: add printk specifier %px

[David Laight]

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

Re: [PATCH V11 4/5] vsprintf: add printk specifier %px

[Kees Cook]

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

-- 
Kees Cook
Pixel Security

RE: [PATCH V11 4/5] vsprintf: add printk specifier %px

[Roberts, William C]

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

Re: [PATCH V11 4/5] vsprintf: add printk specifier %px

[Linus Torvalds]

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

Re: [PATCH V11 4/5] vsprintf: add printk specifier %px

[Andrew Morton]

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.

Re: [PATCH V11 4/5] vsprintf: add printk specifier %px

[Tobin C. Harding]

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.

Re: [PATCH V11 4/5] vsprintf: add printk specifier %px

[Joe Perches]

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");

Re: [PATCH V11 4/5] vsprintf: add printk specifier %px

[Tobin C. Harding]

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.

Re: [PATCH V11 4/5] vsprintf: add printk specifier %px

[Joe Perches]

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.

Re: [PATCH V11 4/5] vsprintf: add printk specifier %px

[Tobin C. Harding]

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.

RE: [PATCH V11 4/5] vsprintf: add printk specifier %px

[David Laight]

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

Re: [PATCH V11 4/5] vsprintf: add printk specifier %px

[Randy Dunlap]

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


-- 
~Randy

Re: [PATCH V11 4/5] vsprintf: add printk specifier %px

[Linus Torvalds]

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

Re: [PATCH V11 4/5] vsprintf: add printk specifier %px

[Sergey Senozhatsky]

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

Re: [PATCH V11 4/5] vsprintf: add printk specifier %px

[Linus Torvalds]

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

Re: [PATCH V11 4/5] vsprintf: add printk specifier %px

[Sergey Senozhatsky]

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

Re: [PATCH V11 4/5] vsprintf: add printk specifier %px

[Alexey Dobriyan]

> more %p grepping [filtering out all `%ps %pf %pb' variants] gives
> a huge number of print outs that potentially can be broken now

Because people who introduce this stupid %p hashing can't be bothered
to actually audit users:

	static int show_timer(struct seq_file *m, void *v)
	{
		...
	        seq_printf(m, "signal: %d/%p\n",
                   timer->sigq->info.si_signo,
                   timer->sigq->info.si_value.sival_ptr);

Overall, this %px thing doesn't matter. Developers will quickly learn
than %p gives some useless irreversible values and start using %px
everywhere. Soon someone will use %px in the wrong place and new
non-standard format specifier will be added.

Re: [PATCH V11 4/5] vsprintf: add printk specifier %px

[Linus Torvalds]

On Tue, Dec 5, 2017 at 8:19 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> Because people who introduce this stupid %p hashing can't be bothered
> to actually audit users:

I don't see you having offered to audit the 12k+ cases, did you?

So maybe it's easier to just change them all, and then the handful
that breaks can be fixed?

Do you maybe want to re-think that "stupid" comment?

Or are you going to spend the time to audit all those other %p users,
particularly considering that we _know_ a number of them are bad?

Once you've done that, and offered to keep doing it, we can undo the hashing.

I'll be waiting.

             Linus

Re: [PATCH V11 4/5] vsprintf: add printk specifier %px

[Geert Uytterhoeven]

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

Re: [PATCH V11 4/5] vsprintf: add printk specifier %px

[Sergey Senozhatsky]

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

Re: [PATCH V11 4/5] vsprintf: add printk specifier %px

[Tobin C. Harding]

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.

Re: [PATCH V11 4/5] vsprintf: add printk specifier %px

[Tobin C. Harding]

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.

Re: [PATCH V11 4/5] vsprintf: add printk specifier %px

[Sergey Senozhatsky]

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

Re: [PATCH V11 4/5] vsprintf: add printk specifier %px

[Tobin C. Harding]

On Wed, Dec 06, 2017 at 07:19:24AM +0300, Alexey Dobriyan wrote:
> > more %p grepping [filtering out all `%ps %pf %pb' variants] gives
> > a huge number of print outs that potentially can be broken now
> 
> Because people who introduce this stupid %p hashing can't be bothered
> to actually audit users:

lol, by 'stupid' he meant you Linus

	Tobin