* Re: [PATCH V11 4/5] vsprintf: add printk specifier %px
@ 2017-12-06 4:19 Alexey Dobriyan
2017-12-06 4:40 ` Linus Torvalds
2017-12-16 20:26 ` Tobin C. Harding
0 siblings, 2 replies; 27+ messages in thread
From: Alexey Dobriyan @ 2017-12-06 4:19 UTC (permalink / raw)
To: sergey.senozhatsky.work; +Cc: linux-kernel, torvalds, me
> 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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V11 4/5] vsprintf: add printk specifier %px 2017-12-06 4:19 [PATCH V11 4/5] vsprintf: add printk specifier %px Alexey Dobriyan @ 2017-12-06 4:40 ` Linus Torvalds 2017-12-16 20:26 ` Tobin C. Harding 1 sibling, 0 replies; 27+ messages in thread From: Linus Torvalds @ 2017-12-06 4:40 UTC (permalink / raw) To: Alexey Dobriyan; +Cc: Sergey Senozhatsky, Linux Kernel Mailing List, tcharding 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V11 4/5] vsprintf: add printk specifier %px 2017-12-06 4:19 [PATCH V11 4/5] vsprintf: add printk specifier %px Alexey Dobriyan 2017-12-06 4:40 ` Linus Torvalds @ 2017-12-16 20:26 ` Tobin C. Harding 1 sibling, 0 replies; 27+ messages in thread From: Tobin C. Harding @ 2017-12-16 20:26 UTC (permalink / raw) To: Alexey Dobriyan; +Cc: sergey.senozhatsky.work, linux-kernel, torvalds 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH V11 0/5] hash addresses printed with %p @ 2017-11-29 2:05 Tobin C. Harding 2017-11-29 2:05 ` [PATCH V11 4/5] vsprintf: add printk specifier %px Tobin C. Harding 0 siblings, 1 reply; 27+ messages in thread From: Tobin C. Harding @ 2017-11-29 2:05 UTC (permalink / raw) To: kernel-hardening Cc: Tobin C. Harding, Linus Torvalds, Jason A. Donenfeld, Theodore Ts'o, Kees Cook, Paolo Bonzini, Tycho Andersen, Roberts, William C, Tejun Heo, Jordan Glover, Greg KH, Petr Mladek, Joe Perches, Ian Campbell, Sergey Senozhatsky, Catalin Marinas, Will Deacon, Steven Rostedt, Chris Fries, Dave Weinstein, Daniel Micay, Djalal Harouni, Radim Krčmář, linux-kernel, Network Development, David Miller, Stephen Rothwell, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, Andrew Morton Currently there exist approximately 14 000 places in the Kernel where addresses are being printed using an unadorned %p. This potentially leaks sensitive information regarding the Kernel layout in memory. Many of these calls are stale, instead of fixing every call lets hash the address by default before printing. This will of course break some users, forcing code printing needed addresses to be updated. We can add a printk specifier for this purpose (%px) to give developers a clear upgrade path for breakages caused by applying this patch set. The added advantage of hashing %p is that security is now opt-out, if you _really_ want the address you have to work a little harder and use %px. The idea for creating the printk specifier %px to print the actual address was suggested by Kees Cook (see below for email threads by subject). Newbie question: I don't know who is potentially going to want to apply this, I've CC'd Andrew Morton. I'm guessing this should go into linux-next so we can see what breaks? I do not know exactly how code gets into linux-next. I've CC'd Stephen Rothwell. Here is the behaviour that this series implements. For kpt_restrict==0 Randomness not ready: printed with %p: (ptrval) # NOTE: with padding Valid pointer: printed with %pK: deadbeefdeadbeef printed with %p: 00000000deadbeef malformed specifier (eg %i): 00000000deadbeef NULL pointer: printed with %pK: 0000000000000000 printed with %p: (null) # NOTE: with padding malformed specifier (eg %i): (null) For kpt_restrict==2 Valid pointer: printed with %pK: 0000000000000000 All other output as for kptr_restrict==0 V11: - Add patch fixing %pK documentation. - Refactor %pK as a separate patch. - Add patch adding printk specifier %px, which prints the actual address (i.e replaces original %p behaviour). - Use %px for KASAN patch. V10: - Add patch so KASAN uses %pK instead of %p. - Add documentation to Documentation/printk-formats.txt - Add tests to lib/test_printf.c - Change "(pointer value)" -> "(ptrval)" to fit within columns on 32 bit machines. V9: - Drop the initial patch from V8, leaving null pointer handling as is. - Print the hashed ID _without_ a '0x' suffix. - Mask the first 32 bits of the hashed ID to all zeros on 64 bit architectures. V8: - Add second patch cleaning up null pointer printing in pointer() - Move %pK handling to separate function, further cleaning up pointer() - Move ptr_to_id() call outside of switch statement making hashing the default behaviour (including malformed specifiers). - Remove use of static_key, replace with simple boolean. V7: - Use tabs instead of spaces (ouch!). V6: - Use __early_initcall() to fill the SipHash key. - Use static keys to guard hashing before the key is available. V5: - Remove spin lock. - Add Jason A. Donenfeld to CC list by request. - Add Theodore Ts'o to CC list due to comment on previous version. V4: - Remove changes to siphash.{ch} - Do word size check, and return value cast, directly in ptr_to_id(). - Use add_ready_random_callback() to guard call to get_random_bytes() V3: - Use atomic_xchg() to guard setting [random] key. - Remove erroneous white space change. V2: - Use SipHash to do the hashing. The discussion related to this patch has been fragmented. There are three threads associated with this patch. Email threads by subject: [PATCH 0/5] add printk specifier %px, unique identifier [PATCH] printk: hash addresses printed with %p [PATCH 0/3] add %pX specifier [kernel-hardening] [RFC V2 0/6] add more kernel pointer filter options Tobin C. Harding (5): docs: correct documentation for %pK vsprintf: refactor %pK code out of pointer() printk: hash addresses printed with %p vsprintf: add printk specifier %px kasan: use %px to print addresses instead of %p Documentation/printk-formats.txt | 31 ++++++- lib/test_printf.c | 108 ++++++++++++++-------- lib/vsprintf.c | 194 +++++++++++++++++++++++++++++---------- mm/kasan/report.c | 8 +- scripts/checkpatch.pl | 2 +- 5 files changed, 248 insertions(+), 95 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH V11 4/5] vsprintf: add printk specifier %px 2017-11-29 2:05 [PATCH V11 0/5] hash addresses printed with %p Tobin C. Harding @ 2017-11-29 2:05 ` Tobin C. Harding 2017-11-29 2:29 ` Linus Torvalds 2017-11-29 23:20 ` Andrew Morton 0 siblings, 2 replies; 27+ messages in thread From: Tobin C. Harding @ 2017-11-29 2:05 UTC (permalink / raw) To: kernel-hardening Cc: Tobin C. Harding, Linus Torvalds, Jason A. Donenfeld, Theodore Ts'o, Kees Cook, Paolo Bonzini, Tycho Andersen, Roberts, William C, Tejun Heo, Jordan Glover, Greg KH, Petr Mladek, Joe Perches, Ian Campbell, Sergey Senozhatsky, Catalin Marinas, Will Deacon, Steven Rostedt, Chris Fries, Dave Weinstein, Daniel Micay, Djalal Harouni, Radim Krčmář, linux-kernel, Network Development, David Miller, Stephen Rothwell, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, Andrew Morton 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 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH V11 4/5] vsprintf: add printk specifier %px 2017-11-29 2:05 ` [PATCH V11 4/5] vsprintf: add printk specifier %px Tobin C. Harding @ 2017-11-29 2:29 ` Linus Torvalds 2017-11-29 4:29 ` Tobin C. Harding 2017-11-29 10:07 ` David Laight 2017-11-29 23:20 ` Andrew Morton 1 sibling, 2 replies; 27+ messages in thread From: Linus Torvalds @ 2017-11-29 2:29 UTC (permalink / raw) To: Tobin C. Harding Cc: kernel-hardening, Jason A. Donenfeld, Theodore Ts'o, Kees Cook, Paolo Bonzini, Tycho Andersen, Roberts, William C, Tejun Heo, Jordan Glover, Greg KH, Petr Mladek, Joe Perches, Ian Campbell, Sergey Senozhatsky, Catalin Marinas, Will Deacon, Steven Rostedt, Chris Fries, Dave Weinstein, Daniel Micay, Djalal Harouni, Radim Krčmář, Linux Kernel Mailing List, Network Development, David Miller, Stephen Rothwell, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, Andrew Morton 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V11 4/5] vsprintf: add printk specifier %px 2017-11-29 2:29 ` Linus Torvalds @ 2017-11-29 4:29 ` Tobin C. Harding 2017-11-29 10:07 ` David Laight 1 sibling, 0 replies; 27+ messages in thread From: Tobin C. Harding @ 2017-11-29 4:29 UTC (permalink / raw) To: Linus Torvalds Cc: kernel-hardening, Jason A. Donenfeld, Theodore Ts'o, Kees Cook, Paolo Bonzini, Tycho Andersen, Roberts, William C, Tejun Heo, Jordan Glover, Greg KH, Petr Mladek, Joe Perches, Ian Campbell, Sergey Senozhatsky, Catalin Marinas, Will Deacon, Steven Rostedt, Chris Fries, Dave Weinstein, Daniel Micay, Djalal Harouni, Radim Krčmář, Linux Kernel Mailing List, Network Development, David Miller, Stephen Rothwell, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, Andrew Morton 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. ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH V11 4/5] vsprintf: add printk specifier %px 2017-11-29 2:29 ` Linus Torvalds 2017-11-29 4:29 ` Tobin C. Harding @ 2017-11-29 10:07 ` David Laight 2017-11-29 22:28 ` Kees Cook 1 sibling, 1 reply; 27+ messages in thread From: David Laight @ 2017-11-29 10:07 UTC (permalink / raw) To: 'Linus Torvalds', Tobin C. Harding Cc: kernel-hardening, Jason A. Donenfeld, Theodore Ts'o, Kees Cook, Paolo Bonzini, Tycho Andersen, Roberts, William C, Tejun Heo, Jordan Glover, Greg KH, Petr Mladek, Joe Perches, Ian Campbell, Sergey Senozhatsky, Catalin Marinas, Will Deacon, Steven Rostedt, Chris Fries, Dave Weinstein, Daniel Micay, Djalal Harouni, Radim Krcmár, Linux Kernel Mailing List, Network Development, David Miller, Stephen Rothwell, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, Andrew Morton 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V11 4/5] vsprintf: add printk specifier %px 2017-11-29 10:07 ` David Laight @ 2017-11-29 22:28 ` Kees Cook 2017-11-29 22:36 ` Roberts, William C ` (2 more replies) 0 siblings, 3 replies; 27+ messages in thread From: Kees Cook @ 2017-11-29 22:28 UTC (permalink / raw) To: David Laight Cc: Linus Torvalds, Tobin C. Harding, kernel-hardening, Jason A. Donenfeld, Theodore Ts'o, Paolo Bonzini, Tycho Andersen, Roberts, William C, Tejun Heo, Jordan Glover, Greg KH, Petr Mladek, Joe Perches, Ian Campbell, Sergey Senozhatsky, Catalin Marinas, Will Deacon, Steven Rostedt, Chris Fries, Dave Weinstein, Daniel Micay, Djalal Harouni, Radim Krcmár, Linux Kernel Mailing List, Network Development, David Miller, Stephen Rothwell, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, Andrew Morton 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH V11 4/5] vsprintf: add printk specifier %px 2017-11-29 22:28 ` Kees Cook @ 2017-11-29 22:36 ` Roberts, William C 2017-11-29 22:47 ` Linus Torvalds 2017-11-30 10:38 ` David Laight 2 siblings, 0 replies; 27+ messages in thread From: Roberts, William C @ 2017-11-29 22:36 UTC (permalink / raw) To: Kees Cook, David Laight Cc: Linus Torvalds, Tobin C. Harding, kernel-hardening, Jason A. Donenfeld, Theodore Ts'o, Paolo Bonzini, Tycho Andersen, Tejun Heo, Jordan Glover, Greg KH, Petr Mladek, Joe Perches, Ian Campbell, Sergey Senozhatsky, Catalin Marinas, Will Deacon, Steven Rostedt, Chris Fries, Dave Weinstein, Daniel Micay, Djalal Harouni, Radim Krcmár, Linux Kernel Mailing List, Network Development, David Miller, Stephen Rothwell, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, Andrew Morton > -----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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V11 4/5] vsprintf: add printk specifier %px 2017-11-29 22:28 ` Kees Cook 2017-11-29 22:36 ` Roberts, William C @ 2017-11-29 22:47 ` Linus Torvalds 2017-11-30 10:38 ` David Laight 2 siblings, 0 replies; 27+ messages in thread From: Linus Torvalds @ 2017-11-29 22:47 UTC (permalink / raw) To: Kees Cook Cc: David Laight, Tobin C. Harding, kernel-hardening, Jason A. Donenfeld, Theodore Ts'o, Paolo Bonzini, Tycho Andersen, Roberts, William C, Tejun Heo, Jordan Glover, Greg KH, Petr Mladek, Joe Perches, Ian Campbell, Sergey Senozhatsky, Catalin Marinas, Will Deacon, Steven Rostedt, Chris Fries, Dave Weinstein, Daniel Micay, Djalal Harouni, Radim Krcmár, Linux Kernel Mailing List, Network Development, David Miller, Stephen Rothwell, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, Andrew Morton 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH V11 4/5] vsprintf: add printk specifier %px 2017-11-29 22:28 ` Kees Cook 2017-11-29 22:36 ` Roberts, William C 2017-11-29 22:47 ` Linus Torvalds @ 2017-11-30 10:38 ` David Laight 2017-12-05 21:08 ` Randy Dunlap 2 siblings, 1 reply; 27+ messages in thread From: David Laight @ 2017-11-30 10:38 UTC (permalink / raw) To: 'Kees Cook' Cc: Linus Torvalds, Tobin C. Harding, kernel-hardening, Jason A. Donenfeld, Theodore Ts'o, Paolo Bonzini, Tycho Andersen, Roberts, William C, Tejun Heo, Jordan Glover, Greg KH, Petr Mladek, Joe Perches, Ian Campbell, Sergey Senozhatsky, Catalin Marinas, Will Deacon, Steven Rostedt, Chris Fries, Dave Weinstein, Daniel Micay, Djalal Harouni, Radim Krcmár, Linux Kernel Mailing List, Network Development, David Miller, Stephen Rothwell, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, Andrew Morton 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V11 4/5] vsprintf: add printk specifier %px 2017-11-30 10:38 ` David Laight @ 2017-12-05 21:08 ` Randy Dunlap 2017-12-05 21:22 ` Linus Torvalds 0 siblings, 1 reply; 27+ messages in thread From: Randy Dunlap @ 2017-12-05 21:08 UTC (permalink / raw) To: David Laight, 'Kees Cook' Cc: Linus Torvalds, Tobin C. Harding, kernel-hardening, Jason A. Donenfeld, Theodore Ts'o, Paolo Bonzini, Tycho Andersen, Roberts, William C, Tejun Heo, Jordan Glover, Greg KH, Petr Mladek, Joe Perches, Ian Campbell, Sergey Senozhatsky, Catalin Marinas, Will Deacon, Steven Rostedt, Chris Fries, Dave Weinstein, Daniel Micay, Djalal Harouni, Radim Krcmár, Linux Kernel Mailing List, Network Development, David Miller, Stephen Rothwell, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, Andrew Morton 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V11 4/5] vsprintf: add printk specifier %px 2017-12-05 21:08 ` Randy Dunlap @ 2017-12-05 21:22 ` Linus Torvalds 2017-12-06 1:36 ` Sergey Senozhatsky 0 siblings, 1 reply; 27+ messages in thread From: Linus Torvalds @ 2017-12-05 21:22 UTC (permalink / raw) To: Randy Dunlap Cc: David Laight, Kees Cook, Tobin C. Harding, kernel-hardening, Jason A. Donenfeld, Theodore Ts'o, Paolo Bonzini, Tycho Andersen, Roberts, William C, Tejun Heo, Jordan Glover, Greg KH, Petr Mladek, Joe Perches, Ian Campbell, Sergey Senozhatsky, Catalin Marinas, Will Deacon, Steven Rostedt, Chris Fries, Dave Weinstein, Daniel Micay, Djalal Harouni, Radim Krcmár, Linux Kernel Mailing List, Network Development, David Miller, Stephen Rothwell, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, Andrew Morton 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V11 4/5] vsprintf: add printk specifier %px 2017-12-05 21:22 ` Linus Torvalds @ 2017-12-06 1:36 ` Sergey Senozhatsky 2017-12-06 1:59 ` Linus Torvalds 0 siblings, 1 reply; 27+ messages in thread From: Sergey Senozhatsky @ 2017-12-06 1:36 UTC (permalink / raw) To: Linus Torvalds Cc: Randy Dunlap, David Laight, Kees Cook, Tobin C. Harding, kernel-hardening, Jason A. Donenfeld, Theodore Ts'o, Paolo Bonzini, Tycho Andersen, Roberts, William C, Tejun Heo, Jordan Glover, Greg KH, Petr Mladek, Joe Perches, Ian Campbell, Sergey Senozhatsky, Catalin Marinas, Will Deacon, Steven Rostedt, Chris Fries, Dave Weinstein, Daniel Micay, Djalal Harouni, Radim Krcmár, Linux Kernel Mailing List, Network Development, David Miller, Stephen Rothwell, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, Andrew Morton 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V11 4/5] vsprintf: add printk specifier %px 2017-12-06 1:36 ` Sergey Senozhatsky @ 2017-12-06 1:59 ` Linus Torvalds 2017-12-06 2:15 ` Sergey Senozhatsky 2017-12-06 8:32 ` Geert Uytterhoeven 0 siblings, 2 replies; 27+ messages in thread From: Linus Torvalds @ 2017-12-06 1:59 UTC (permalink / raw) To: Sergey Senozhatsky Cc: Randy Dunlap, David Laight, Kees Cook, Tobin C. Harding, kernel-hardening, Jason A. Donenfeld, Theodore Ts'o, Paolo Bonzini, Tycho Andersen, Roberts, William C, Tejun Heo, Jordan Glover, Greg KH, Petr Mladek, Joe Perches, Ian Campbell, Sergey Senozhatsky, Catalin Marinas, Will Deacon, Steven Rostedt, Chris Fries, Dave Weinstein, Daniel Micay, Djalal Harouni, Radim Krcmár, Linux Kernel Mailing List, Network Development, David Miller, Stephen Rothwell, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, Andrew Morton 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V11 4/5] vsprintf: add printk specifier %px 2017-12-06 1:59 ` Linus Torvalds @ 2017-12-06 2:15 ` Sergey Senozhatsky 2017-12-06 8:32 ` Geert Uytterhoeven 1 sibling, 0 replies; 27+ messages in thread From: Sergey Senozhatsky @ 2017-12-06 2:15 UTC (permalink / raw) To: Linus Torvalds Cc: Sergey Senozhatsky, Randy Dunlap, David Laight, Kees Cook, Tobin C. Harding, kernel-hardening, Jason A. Donenfeld, Theodore Ts'o, Paolo Bonzini, Tycho Andersen, Roberts, William C, Tejun Heo, Jordan Glover, Greg KH, Petr Mladek, Joe Perches, Ian Campbell, Sergey Senozhatsky, Catalin Marinas, Will Deacon, Steven Rostedt, Chris Fries, Dave Weinstein, Daniel Micay, Djalal Harouni, Radim Krcmár, Linux Kernel Mailing List, Network Development, David Miller, Stephen Rothwell, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, Andrew Morton 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V11 4/5] vsprintf: add printk specifier %px 2017-12-06 1:59 ` Linus Torvalds 2017-12-06 2:15 ` Sergey Senozhatsky @ 2017-12-06 8:32 ` Geert Uytterhoeven 2017-12-06 8:45 ` Sergey Senozhatsky 2017-12-07 5:12 ` Tobin C. Harding 1 sibling, 2 replies; 27+ messages in thread From: Geert Uytterhoeven @ 2017-12-06 8:32 UTC (permalink / raw) To: Linus Torvalds Cc: Sergey Senozhatsky, Randy Dunlap, David Laight, Kees Cook, Tobin C. Harding, kernel-hardening, Jason A. Donenfeld, Theodore Ts'o, Paolo Bonzini, Tycho Andersen, Roberts, William C, Tejun Heo, Jordan Glover, Greg KH, Petr Mladek, Joe Perches, Ian Campbell, Sergey Senozhatsky, Catalin Marinas, Will Deacon, Steven Rostedt, Chris Fries, Dave Weinstein, Daniel Micay, Djalal Harouni, Radim Krcmár, Linux Kernel Mailing List, Network Development, David Miller, Stephen Rothwell, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, Andrew Morton 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V11 4/5] vsprintf: add printk specifier %px 2017-12-06 8:32 ` Geert Uytterhoeven @ 2017-12-06 8:45 ` Sergey Senozhatsky 2017-12-07 5:17 ` Tobin C. Harding 2017-12-07 5:12 ` Tobin C. Harding 1 sibling, 1 reply; 27+ messages in thread From: Sergey Senozhatsky @ 2017-12-06 8:45 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Linus Torvalds, Sergey Senozhatsky, Randy Dunlap, David Laight, Kees Cook, Tobin C. Harding, kernel-hardening, Jason A. Donenfeld, Theodore Ts'o, Paolo Bonzini, Tycho Andersen, Roberts, William C, Tejun Heo, Jordan Glover, Greg KH, Petr Mladek, Joe Perches, Ian Campbell, Sergey Senozhatsky, Catalin Marinas, Will Deacon, Steven Rostedt, Chris Fries, Dave Weinstein, Daniel Micay, Djalal Harouni, Radim Krcmár, Linux Kernel Mailing List, Network Development, David Miller, Stephen Rothwell, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, Andrew Morton 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V11 4/5] vsprintf: add printk specifier %px 2017-12-06 8:45 ` Sergey Senozhatsky @ 2017-12-07 5:17 ` Tobin C. Harding 2017-12-07 5:37 ` Sergey Senozhatsky 0 siblings, 1 reply; 27+ messages in thread From: Tobin C. Harding @ 2017-12-07 5:17 UTC (permalink / raw) To: Sergey Senozhatsky Cc: Geert Uytterhoeven, Linus Torvalds, Randy Dunlap, David Laight, Kees Cook, kernel-hardening, Jason A. Donenfeld, Theodore Ts'o, Paolo Bonzini, Tycho Andersen, Roberts, William C, Tejun Heo, Jordan Glover, Greg KH, Petr Mladek, Joe Perches, Ian Campbell, Sergey Senozhatsky, Catalin Marinas, Will Deacon, Steven Rostedt, Chris Fries, Dave Weinstein, Daniel Micay, Djalal Harouni, Radim Krcmár, Linux Kernel Mailing List, Network Development, David Miller, Stephen Rothwell, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, Andrew Morton 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. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V11 4/5] vsprintf: add printk specifier %px 2017-12-07 5:17 ` Tobin C. Harding @ 2017-12-07 5:37 ` Sergey Senozhatsky 0 siblings, 0 replies; 27+ messages in thread From: Sergey Senozhatsky @ 2017-12-07 5:37 UTC (permalink / raw) To: Tobin C. Harding Cc: Sergey Senozhatsky, Geert Uytterhoeven, Linus Torvalds, Randy Dunlap, David Laight, Kees Cook, kernel-hardening, Jason A. Donenfeld, Theodore Ts'o, Paolo Bonzini, Tycho Andersen, Roberts, William C, Tejun Heo, Jordan Glover, Greg KH, Petr Mladek, Joe Perches, Ian Campbell, Sergey Senozhatsky, Catalin Marinas, Will Deacon, Steven Rostedt, Chris Fries, Dave Weinstein, Daniel Micay, Djalal Harouni, Radim Krcmár, Linux Kernel Mailing List, Network Development, David Miller, Stephen Rothwell, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, Andrew Morton 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V11 4/5] vsprintf: add printk specifier %px 2017-12-06 8:32 ` Geert Uytterhoeven 2017-12-06 8:45 ` Sergey Senozhatsky @ 2017-12-07 5:12 ` Tobin C. Harding 1 sibling, 0 replies; 27+ messages in thread From: Tobin C. Harding @ 2017-12-07 5:12 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Linus Torvalds, Sergey Senozhatsky, Randy Dunlap, David Laight, Kees Cook, kernel-hardening, Jason A. Donenfeld, Theodore Ts'o, Paolo Bonzini, Tycho Andersen, Roberts, William C, Tejun Heo, Jordan Glover, Greg KH, Petr Mladek, Joe Perches, Ian Campbell, Sergey Senozhatsky, Catalin Marinas, Will Deacon, Steven Rostedt, Chris Fries, Dave Weinstein, Daniel Micay, Djalal Harouni, Radim Krcmár, Linux Kernel Mailing List, Network Development, David Miller, Stephen Rothwell, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, Andrew Morton 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. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V11 4/5] vsprintf: add printk specifier %px 2017-11-29 2:05 ` [PATCH V11 4/5] vsprintf: add printk specifier %px Tobin C. Harding 2017-11-29 2:29 ` Linus Torvalds @ 2017-11-29 23:20 ` Andrew Morton 2017-11-29 23:26 ` Tobin C. Harding 1 sibling, 1 reply; 27+ messages in thread From: Andrew Morton @ 2017-11-29 23:20 UTC (permalink / raw) To: Tobin C. Harding Cc: kernel-hardening, Linus Torvalds, Jason A. Donenfeld, Theodore Ts'o, Kees Cook, Paolo Bonzini, Tycho Andersen, Roberts, William C, Tejun Heo, Jordan Glover, Greg KH, Petr Mladek, Joe Perches, Ian Campbell, Sergey Senozhatsky, Catalin Marinas, Will Deacon, Steven Rostedt, Chris Fries, Dave Weinstein, Daniel Micay, Djalal Harouni, Radim Krčmář, linux-kernel, Network Development, David Miller, Stephen Rothwell, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov 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. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V11 4/5] vsprintf: add printk specifier %px 2017-11-29 23:20 ` Andrew Morton @ 2017-11-29 23:26 ` Tobin C. Harding 2017-11-30 3:58 ` Joe Perches 0 siblings, 1 reply; 27+ messages in thread From: Tobin C. Harding @ 2017-11-29 23:26 UTC (permalink / raw) To: Andrew Morton Cc: kernel-hardening, Linus Torvalds, Jason A. Donenfeld, Theodore Ts'o, Kees Cook, Paolo Bonzini, Tycho Andersen, Roberts, William C, Tejun Heo, Jordan Glover, Greg KH, Petr Mladek, Joe Perches, Ian Campbell, Sergey Senozhatsky, Catalin Marinas, Will Deacon, Steven Rostedt, Chris Fries, Dave Weinstein, Daniel Micay, Djalal Harouni, Radim Krčmář, linux-kernel, Network Development, David Miller, Stephen Rothwell, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov 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. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V11 4/5] vsprintf: add printk specifier %px 2017-11-29 23:26 ` Tobin C. Harding @ 2017-11-30 3:58 ` Joe Perches 2017-11-30 4:18 ` Tobin C. Harding 0 siblings, 1 reply; 27+ messages in thread From: Joe Perches @ 2017-11-30 3:58 UTC (permalink / raw) To: Tobin C. Harding, Andrew Morton Cc: kernel-hardening, Linus Torvalds, Jason A. Donenfeld, Theodore Ts'o, Kees Cook, Paolo Bonzini, Tycho Andersen, Roberts, William C, Tejun Heo, Jordan Glover, Greg KH, Petr Mladek, Ian Campbell, Sergey Senozhatsky, Catalin Marinas, Will Deacon, Steven Rostedt, Chris Fries, Dave Weinstein, Daniel Micay, Djalal Harouni, Radim Krčmář, linux-kernel, Network Development, David Miller, Stephen Rothwell, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov 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"); ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH V11 4/5] vsprintf: add printk specifier %px 2017-11-30 3:58 ` Joe Perches @ 2017-11-30 4:18 ` Tobin C. Harding 2017-11-30 4:41 ` Joe Perches 0 siblings, 1 reply; 27+ messages in thread From: Tobin C. Harding @ 2017-11-30 4:18 UTC (permalink / raw) To: Joe Perches Cc: Andrew Morton, kernel-hardening, Linus Torvalds, Jason A. Donenfeld, Theodore Ts'o, Kees Cook, Paolo Bonzini, Tycho Andersen, Roberts, William C, Tejun Heo, Jordan Glover, Greg KH, Petr Mladek, Ian Campbell, Sergey Senozhatsky, Catalin Marinas, Will Deacon, Steven Rostedt, Chris Fries, Dave Weinstein, Daniel Micay, Djalal Harouni, Radim Krčmář, linux-kernel, Network Development, David Miller, Stephen Rothwell, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov 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. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V11 4/5] vsprintf: add printk specifier %px 2017-11-30 4:18 ` Tobin C. Harding @ 2017-11-30 4:41 ` Joe Perches 2017-11-30 5:00 ` Tobin C. Harding 0 siblings, 1 reply; 27+ messages in thread From: Joe Perches @ 2017-11-30 4:41 UTC (permalink / raw) To: Tobin C. Harding Cc: Andrew Morton, kernel-hardening, Linus Torvalds, Jason A. Donenfeld, Theodore Ts'o, Kees Cook, Paolo Bonzini, Tycho Andersen, Roberts, William C, Tejun Heo, Jordan Glover, Greg KH, Petr Mladek, Ian Campbell, Sergey Senozhatsky, Catalin Marinas, Will Deacon, Steven Rostedt, Chris Fries, Dave Weinstein, Daniel Micay, Djalal Harouni, Radim Krčmář, linux-kernel, Network Development, David Miller, Stephen Rothwell, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov 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. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V11 4/5] vsprintf: add printk specifier %px 2017-11-30 4:41 ` Joe Perches @ 2017-11-30 5:00 ` Tobin C. Harding 0 siblings, 0 replies; 27+ messages in thread From: Tobin C. Harding @ 2017-11-30 5:00 UTC (permalink / raw) To: Joe Perches Cc: Andrew Morton, kernel-hardening, Linus Torvalds, Jason A. Donenfeld, Theodore Ts'o, Kees Cook, Paolo Bonzini, Tycho Andersen, Roberts, William C, Tejun Heo, Jordan Glover, Greg KH, Petr Mladek, Ian Campbell, Sergey Senozhatsky, Catalin Marinas, Will Deacon, Steven Rostedt, Chris Fries, Dave Weinstein, Daniel Micay, Djalal Harouni, Radim Krčmář, linux-kernel, Network Development, David Miller, Stephen Rothwell, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov 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. ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2017-12-16 20:26 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-12-06 4:19 [PATCH V11 4/5] vsprintf: add printk specifier %px Alexey Dobriyan 2017-12-06 4:40 ` Linus Torvalds 2017-12-16 20:26 ` Tobin C. Harding -- strict thread matches above, loose matches on Subject: below -- 2017-11-29 2:05 [PATCH V11 0/5] hash addresses printed with %p Tobin C. Harding 2017-11-29 2:05 ` [PATCH V11 4/5] vsprintf: add printk specifier %px Tobin C. Harding 2017-11-29 2:29 ` Linus Torvalds 2017-11-29 4:29 ` Tobin C. Harding 2017-11-29 10:07 ` David Laight 2017-11-29 22:28 ` Kees Cook 2017-11-29 22:36 ` Roberts, William C 2017-11-29 22:47 ` Linus Torvalds 2017-11-30 10:38 ` David Laight 2017-12-05 21:08 ` Randy Dunlap 2017-12-05 21:22 ` Linus Torvalds 2017-12-06 1:36 ` Sergey Senozhatsky 2017-12-06 1:59 ` Linus Torvalds 2017-12-06 2:15 ` Sergey Senozhatsky 2017-12-06 8:32 ` Geert Uytterhoeven 2017-12-06 8:45 ` Sergey Senozhatsky 2017-12-07 5:17 ` Tobin C. Harding 2017-12-07 5:37 ` Sergey Senozhatsky 2017-12-07 5:12 ` Tobin C. Harding 2017-11-29 23:20 ` Andrew Morton 2017-11-29 23:26 ` Tobin C. Harding 2017-11-30 3:58 ` Joe Perches 2017-11-30 4:18 ` Tobin C. Harding 2017-11-30 4:41 ` Joe Perches 2017-11-30 5:00 ` Tobin C. Harding
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).