Re: [PATCH V11 4/5] vsprintf: add printk specifier %px
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
Re: [PATCH V11 4/5] vsprintf: add printk specifier %px
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
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
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
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
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
On Tue, Dec 5, 2017 at 8:19 PM, Alexey Dobriyan wrote: > Because people who introduce this stupid %p hashing can't be bothered > to actually audit users: I don't see you having offered to audit the 12k+ cases, did you? So maybe it's easier to just change them all, and then the handful that breaks can be fixed? Do you maybe want to re-think that "stupid" comment? Or are you going to spend the time to audit all those other %p users, particularly considering that we _know_ a number of them are bad? Once you've done that, and offered to keep doing it, we can undo the hashing. I'll be waiting. Linus
Re: [PATCH V11 4/5] vsprintf: add printk specifier %px
> more %p grepping [filtering out all `%ps %pf %pb' variants] gives > a huge number of print outs that potentially can be broken now Because people who introduce this stupid %p hashing can't be bothered to actually audit users: static int show_timer(struct seq_file *m, void *v) { ... seq_printf(m, "signal: %d/%p\n", timer->sigq->info.si_signo, timer->sigq->info.si_value.sival_ptr); Overall, this %px thing doesn't matter. Developers will quickly learn than %p gives some useless irreversible values and start using %px everywhere. Soon someone will use %px in the wrong place and new non-standard format specifier will be added.
Re: [PATCH V11 4/5] vsprintf: add printk specifier %px
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", &cpuid); arch/x86/kernel/traps.c:printk(KERN_EMERG "BUG: stack guard page was hit at %p (stack is %p..%p)\n", so I'm not in position to suggest the removal of those print outs or to decide if those are important at all, just saying that that "I'm confused by pointer values and can't debug" might be more likely that we thought. > So my gut feel is that those printouts should probably just be > removed. They have some very old historical reasons: we've printed out > the page directory pointers (and followed the page tables) since at > least back in the 1.1.x days. This is from the 1.1.7 patch, back when > mm/memory.c was all about x86: I see, thanks. -ss
Re: [PATCH V11 4/5] vsprintf: add printk specifier %px
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
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
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
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
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
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 '$complete_extension' expo
Re: [PATCH V11 4/5] vsprintf: add printk specifier %px
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
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" . > raw_line($count, 0); > +
Re: [PATCH V11 4/5] vsprintf: add printk specifier %px
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
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
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
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
> -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 > Cc: Linus Torvalds ; Tobin C. Harding > ; kernel-harden...@lists.openwall.com; 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 ker...@vger.kernel.org>; Network Development ; > David Miller ; Stephen Rothwell > ; Andrey Ryabinin ; > Alexander Potapenko ; Dmitry Vyukov > ; Andrew Morton > Subject: Re: [PATCH V11 4/5] vsprintf: add printk specifier %px > > 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. 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
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
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
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
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