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

"" would be better returned when !CONFIG_KALLSYMS, right?

thanks,
Tobin.

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
>  wrote:
> > On Tue, Dec 5, 2017 at 5:36 PM, Sergey Senozhatsky
> >  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

[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

[Geert Uytterhoeven]

Hi Linus,

On Wed, Dec 6, 2017 at 2:59 AM, Linus Torvalds
 wrote:
> On Tue, Dec 5, 2017 at 5:36 PM, Sergey Senozhatsky
>  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 -- ge...@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/05/17 17:59), Linus Torvalds wrote:
[..]
> On Tue, Dec 5, 2017 at 5:36 PM, Sergey Senozhatsky
>  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", );
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

[Linus Torvalds]

On Tue, Dec 5, 2017 at 5:36 PM, Sergey Senozhatsky
 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]

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 1:08 PM, Randy Dunlap  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

[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  
>> wrote:
>>> From: Linus Torvalds
 Sent: 29 November 2017 02:29

 On Tue, Nov 28, 2017 at 6:05 PM, Tobin C. Harding  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

[David Laight]

From: Kees Cook
> Sent: 29 November 2017 22:28
> On Wed, Nov 29, 2017 at 2:07 AM, David Laight  wrote:
> > From: Linus Torvalds
> >> Sent: 29 November 2017 02:29
> >>
> >> On Tue, Nov 28, 2017 at 6:05 PM, Tobin C. Harding  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

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

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

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

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"  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++) {
+

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

[Andrew Morton]

On Wed, 29 Nov 2017 13:05:04 +1100 "Tobin C. Harding"  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

[Linus Torvalds]

On Wed, Nov 29, 2017 at 2:28 PM, Kees Cook  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

[Roberts, William C]

> -Original Message-
> From: keesc...@google.com [mailto:keesc...@google.com] On Behalf Of Kees
> Cook
> Sent: Wednesday, November 29, 2017 2:28 PM
> To: David Laight <david.lai...@aculab.com>
> Cc: Linus Torvalds <torva...@linux-foundation.org>; Tobin C. Harding
> <m...@tobin.cc>; kernel-harden...@lists.openwall.com; Jason A. Donenfeld
> <ja...@zx2c4.com>; Theodore Ts'o <ty...@mit.edu>; Paolo Bonzini
> <pbonz...@redhat.com>; Tycho Andersen <ty...@tycho.ws>; Roberts, William C
> <william.c.robe...@intel.com>; Tejun Heo <t...@kernel.org>; Jordan Glover
> <golden_mille...@protonmail.ch>; Greg KH <gre...@linuxfoundation.org>;
> Petr Mladek <pmla...@suse.com>; Joe Perches <j...@perches.com>; Ian
> Campbell <i...@hellion.org.uk>; Sergey Senozhatsky
> <sergey.senozhat...@gmail.com>; Catalin Marinas <catalin.mari...@arm.com>;
> Will Deacon <wilal.dea...@arm.com>; Steven Rostedt <rost...@goodmis.org>;
> Chris Fries <cfr...@google.com>; Dave Weinstein <olo...@google.com>; Daniel
> Micay <danielmi...@gmail.com>; Djalal Harouni <tix...@gmail.com>; Radim
> Krcmár <rkrc...@redhat.com>; Linux Kernel Mailing List  ker...@vger.kernel.org>; Network Development <netdev@vger.kernel.org>;
> David Miller <da...@davemloft.net>; Stephen Rothwell
> <s...@canb.auug.org.au>; Andrey Ryabinin <aryabi...@virtuozzo.com>;
> Alexander Potapenko <gli...@google.com>; Dmitry Vyukov
> <dvyu...@google.com>; Andrew Morton <a...@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.lai...@aculab.com>
> wrote:
> > From: Linus Torvalds
> >> Sent: 29 November 2017 02:29
> >>
> >> On Tue, Nov 28, 2017 at 6:05 PM, Tobin C. Harding <m...@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

[Kees Cook]

On Wed, Nov 29, 2017 at 2:07 AM, David Laight  wrote:
> From: Linus Torvalds
>> Sent: 29 November 2017 02:29
>>
>> On Tue, Nov 28, 2017 at 6:05 PM, Tobin C. Harding  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

[David Laight]

From: Linus Torvalds
> Sent: 29 November 2017 02:29
> 
> On Tue, Nov 28, 2017 at 6:05 PM, Tobin C. Harding  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

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

[Linus Torvalds]

On Tue, Nov 28, 2017 at 6:05 PM, Tobin C. Harding  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