Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1469579 > unrolled thread

[PATCH 3/6] x86/dumpstack: make printk_stack_address() more generally useful

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/6] x86/dumpstack: make printk_stack_address() more generally useful Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-24 19:00 +0200
    Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more  generally useful Joe Perches <joe@perches.com> - 2016-08-24 19:30 +0200
      Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more  generally useful Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-24 20:50 +0200
        Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more  generally useful Joe Perches <joe@perches.com> - 2016-08-24 21:20 +0200
          Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more  generally useful Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-24 21:30 +0200
            Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more  generally useful Joe Perches <joe@perches.com> - 2016-08-24 22:10 +0200
    Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more  generally useful Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-24 20:40 +0200
      Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more  generally useful Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-24 21:50 +0200
        Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more  generally useful Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-25 19:50 +0200
          Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more  generally useful Kees Cook <keescook@chromium.org> - 2016-08-25 23:00 +0200
            Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more  generally useful Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-25 23:10 +0200
              Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more  generally useful Kees Cook <keescook@chromium.org> - 2016-08-26 04:30 +0200
              Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more  generally useful Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-26 05:30 +0200
                Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more  generally useful Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-26 06:50 +0200
                  Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more  generally useful Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-26 08:00 +0200
                    Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more  generally useful Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-26 15:40 +0200
      Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more  generally useful Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-31 19:00 +0200
        Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more  generally useful Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-31 19:20 +0200
          Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more  generally useful Josh Poimboeuf <jpoimboe@redhat.com> - 2016-09-01 15:10 +0200
            Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more  generally useful Linus Torvalds <torvalds@linux-foundation.org> - 2016-09-01 18:40 +0200
    Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more  generally useful Peter Zijlstra <peterz@infradead.org> - 2016-08-24 20:50 +0200
    Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more  generally useful Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-24 21:20 +0200
    [tip:x86/asm] x86/dumpstack: Make printk_stack_address() more  generally useful tip-bot for Josh Poimboeuf <tipbot@zytor.com> - 2016-09-08 12:00 +0200

Page 1 of 2  [1] 2  Next page →

#1469579 — [PATCH 3/6] x86/dumpstack: make printk_stack_address() more generally useful

Change printk_stack_address() to be useful when called by an unwinder
outside the context of dump_trace().

Specifically:

- printk_stack_address()'s 'data' argument is always used as the log
  level string.  Make that explicit.

- Call touch_nmi_watchdog().

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/dumpstack.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 01072e9..f0ddf85 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -26,10 +26,11 @@ int kstack_depth_to_print = 3 * STACKSLOTS_PER_LINE;
 static int die_counter;
 
 static void printk_stack_address(unsigned long address, int reliable,
-		void *data)
+				 char *log_lvl)
 {
+	touch_nmi_watchdog();
 	printk("%s [<%p>] %s%pB\n",
-		(char *)data, (void *)address, reliable ? "" : "? ",
+		log_lvl, (void *)address, reliable ? "" : "? ",
 		(void *)address);
 }
 
@@ -148,7 +149,6 @@ static int print_trace_stack(void *data, char *name)
  */
 static int print_trace_address(void *data, unsigned long addr, int reliable)
 {
-	touch_nmi_watchdog();
 	printk_stack_address(addr, reliable, data);
 	return 0;
 }
-- 
2.7.4

[toc] | [next] | [standalone]

#1469596 — Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more generally useful

On Wed, 2016-08-24 at 11:50 -0500, Josh Poimboeuf wrote:
> Change printk_stack_address() to be useful when called by an unwinder
> outside the context of dump_trace().
> 
> Specifically:
> 
> - printk_stack_address()'s 'data' argument is always used as the log
>   level string.  Make that explicit.

If this is true, and I'm not sure it is as I believe
there are static strings emitted like EOE and IRQ,
shouldn't this bubble up through the calling tree?

> - Call touch_nmi_watchdog().
[]
> diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
[]
> @@ -26,10 +26,11 @@ int kstack_depth_to_print = 3 * STACKSLOTS_PER_LINE;
>  static int die_counter;
>  
>  static void printk_stack_address(unsigned long address, int reliable,
> -		void *data)
> +				 char *log_lvl)
>  {
> +	touch_nmi_watchdog();
>  	printk("%s [<%p>] %s%pB\n",
> -		(char *)data, (void *)address, reliable ? "" : "? ",
> +		log_lvl, (void *)address, reliable ? "" : "? ",
>  		(void *)address);
>  }
>  
> @@ -148,7 +149,6 @@ static int print_trace_stack(void *data, char *name)
>   */
>  static int print_trace_address(void *data, unsigned long addr, int reliable)
>  {
> -	touch_nmi_watchdog();
>  	printk_stack_address(addr, reliable, data);
>  	return 0;
>  }

like for data here?

[toc] | [prev] | [next] | [standalone]

#1469646 — Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more generally useful

On Wed, Aug 24, 2016 at 10:28:38AM -0700, Joe Perches wrote:
> On Wed, 2016-08-24 at 11:50 -0500, Josh Poimboeuf wrote:
> > Change printk_stack_address() to be useful when called by an unwinder
> > outside the context of dump_trace().
> > 
> > Specifically:
> > 
> > - printk_stack_address()'s 'data' argument is always used as the log
> >   level string.  Make that explicit.
> 
> If this is true, and I'm not sure it is as I believe
> there are static strings emitted like EOE and IRQ,
> shouldn't this bubble up through the calling tree?
>
> > - Call touch_nmi_watchdog().
> []
> > diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
> []
> > @@ -26,10 +26,11 @@ int kstack_depth_to_print = 3 * STACKSLOTS_PER_LINE;
> >  static int die_counter;
> >  
> >  static void printk_stack_address(unsigned long address, int reliable,
> > -		void *data)
> > +				 char *log_lvl)
> >  {
> > +	touch_nmi_watchdog();
> >  	printk("%s [<%p>] %s%pB\n",
> > -		(char *)data, (void *)address, reliable ? "" : "? ",
> > +		log_lvl, (void *)address, reliable ? "" : "? ",
> >  		(void *)address);
> >  }
> >  
> > @@ -148,7 +149,6 @@ static int print_trace_stack(void *data, char *name)
> >   */
> >  static int print_trace_address(void *data, unsigned long addr, int reliable)
> >  {
> > -	touch_nmi_watchdog();
> >  	printk_stack_address(addr, reliable, data);
> >  	return 0;
> >  }
> 
> like for data here?

This function needs to keep its 'void *data' argument because it's a
callback for stacktrace_ops, so it has to conform to the callback
interface.  'data' is used for passing a pointer to an opaque data
structure to the callback.

Also this is the only caller of printk_stack_address(), so there's
nowhere else to bubble it up to.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1469664 — Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more generally useful

On Wed, 2016-08-24 at 13:43 -0500, Josh Poimboeuf wrote:
> On Wed, Aug 24, 2016 at 10:28:38AM -0700, Joe Perches wrote:
> > On Wed, 2016-08-24 at 11:50 -0500, Josh Poimboeuf wrote:
> > > Change printk_stack_address() to be useful when called by an unwinder
> > > outside the context of dump_trace().
> > > 
> > > Specifically:
> > > 
> > > - printk_stack_address()'s 'data' argument is always used as the log
> > >   level string.  Make that explicit.
[]
> > If this is true, and I'm not sure it is as I believe
> > there are static strings emitted like EOE and IRQ,
> > shouldn't this bubble up through the calling tree?

> []
> This function needs to keep its 'void *data' argument because it's a
> callback for stacktrace_ops, so it has to conform to the callback
> interface.  'data' is used for passing a pointer to an opaque data
> structure to the callback.
> 
> Also this is the only caller of printk_stack_address(), so there's
> nowhere else to bubble it up to.

And that shows that print_stack_address(data is not always a log level.
ie: walk_stack uses it to print a string not a log level.

[toc] | [prev] | [next] | [standalone]

#1469666 — Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more generally useful

On Wed, Aug 24, 2016 at 12:07:06PM -0700, Joe Perches wrote:
> On Wed, 2016-08-24 at 13:43 -0500, Josh Poimboeuf wrote:
> > On Wed, Aug 24, 2016 at 10:28:38AM -0700, Joe Perches wrote:
> > > On Wed, 2016-08-24 at 11:50 -0500, Josh Poimboeuf wrote:
> > > > Change printk_stack_address() to be useful when called by an unwinder
> > > > outside the context of dump_trace().
> > > > 
> > > > Specifically:
> > > > 
> > > > - printk_stack_address()'s 'data' argument is always used as the log
> > > >   level string.  Make that explicit.
> []
> > > If this is true, and I'm not sure it is as I believe
> > > there are static strings emitted like EOE and IRQ,
> > > shouldn't this bubble up through the calling tree?
> 
> > []
> > This function needs to keep its 'void *data' argument because it's a
> > callback for stacktrace_ops, so it has to conform to the callback
> > interface.  'data' is used for passing a pointer to an opaque data
> > structure to the callback.
> > 
> > Also this is the only caller of printk_stack_address(), so there's
> > nowhere else to bubble it up to.
> 
> And that shows that print_stack_address(data is not always a log level.
> ie: walk_stack uses it to print a string not a log level.

Hm, can you be more specific?  As far as I can tell, here's the only
possible call path to print_trace_address() and printk_stack_address():

show_trace_log_lvl()
  dump_trace()				// ops is print_trace_op
    print_context_stack()		// ops->walk_stack
      print_trace_address()		// ops->address
          printk_stack_address()

So 'data' is a sneaky way to pass 'log_lvl' from show_trace_log_lvl() to
print_trace_address(), without dump_trace() and print_context_stack()
knowing what it is, because they're used in other places where 'data'
means something else.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1469685 — Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more generally useful

On Wed, 2016-08-24 at 14:24 -0500, Josh Poimboeuf wrote:
> On Wed, Aug 24, 2016 at 12:07:06PM -0700, Joe Perches wrote:
> > On Wed, 2016-08-24 at 13:43 -0500, Josh Poimboeuf wrote:
> > > On Wed, Aug 24, 2016 at 10:28:38AM -0700, Joe Perches wrote:
> > > > On Wed, 2016-08-24 at 11:50 -0500, Josh Poimboeuf wrote:
> > > > > Change printk_stack_address() to be useful when called by an unwinder
> > > > > outside the context of dump_trace().
> > > > > 
> > > > > Specifically:
> > > > > 
> > > > > - printk_stack_address()'s 'data' argument is always used as the log
> > > > >   level string.  Make that explicit.
> > []
> > > > If this is true, and I'm not sure it is as I believe
> > > > there are static strings emitted like EOE and IRQ,
> > > > shouldn't this bubble up through the calling tree?
> > > []
> > > This function needs to keep its 'void *data' argument because it's a
> > > callback for stacktrace_ops, so it has to conform to the callback
> > > interface.  'data' is used for passing a pointer to an opaque data
> > > structure to the callback.
> > > 
> > > Also this is the only caller of printk_stack_address(), so there's
> > > nowhere else to bubble it up to.
> > And that shows that print_stack_address(data is not always a log level.
> > ie: walk_stack uses it to print a string not a log level.
> Hm, can you be more specific?  As far as I can tell, here's the only
> possible call path to print_trace_address() and printk_stack_address():
> 
> show_trace_log_lvl()
>   dump_trace()				// ops is print_trace_op
>     print_context_stack()		// ops->walk_stack
>       print_trace_address()		// ops->address
>           printk_stack_address()
> 
> So 'data' is a sneaky way to pass 'log_lvl' from show_trace_log_lvl() to
> print_trace_address(), without dump_trace() and print_context_stack()
> knowing what it is, because they're used in other places where 'data'
> means something else.

hmm, perhaps I got lost in a twisting maze of little callbacks.
I'll drop some stuff and see where I am next time.

[toc] | [prev] | [next] | [standalone]

#1469637 — Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more generally useful

On Wed, Aug 24, 2016 at 2:22 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> I actively disable KASLR on my dev box and feed these hex numbers into
> addr2line -ie vmlinux to find where in the function we are.
>
> Having the option to make %pB generate them works for me.

Yeah, considering that this is the only place this is used, changing
%pB sounds quite reasonable.

We could perhaps make %pB show the hex numbers and address (so pB
would expand to "[<hex>] symbolname".if

 (a) not randomizing (so the hex numbers _may_ be useful)

 (b) kptr_restrict is 0 (so the hex numbers are "safe" in the dmesg)

and fall back to just the symbolic name if either of those aren't true?

And obviously, if KALLSYMS isn't enabled, you always show hex
numbers.. That's already the case (but we might want to add the "[<>}'
markers around the hex numbers just to make the user space automation
we do have work).

            Linus

[toc] | [prev] | [next] | [standalone]

#1469671 — Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more generally useful

On Wed, Aug 24, 2016 at 02:37:21PM -0400, Linus Torvalds wrote:
> On Wed, Aug 24, 2016 at 2:22 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > I actively disable KASLR on my dev box and feed these hex numbers into
> > addr2line -ie vmlinux to find where in the function we are.
> >
> > Having the option to make %pB generate them works for me.
> 
> Yeah, considering that this is the only place this is used, changing
> %pB sounds quite reasonable.

There's now another use of '%pB' in proc_pid_stack() in the tip tree: I
changed it to '%pB' from '%pS'.  But I think the modified '%pB' would
work there as well.

> We could perhaps make %pB show the hex numbers and address (so pB
> would expand to "[<hex>] symbolname".if
> 
>  (a) not randomizing (so the hex numbers _may_ be useful)
> 
>  (b) kptr_restrict is 0 (so the hex numbers are "safe" in the dmesg)
> 
> and fall back to just the symbolic name if either of those aren't true?

Do we really need to check for both?  '%pK' only checks kptr_restrict.
I'd think we should be consistent with that.  And maybe there are some
scenarios where the actual text addresses provide useful debug
information if KASLR is enabled and kptr_restrict is zero.

> And obviously, if KALLSYMS isn't enabled, you always show hex
> numbers.. That's already the case (but we might want to add the "[<>}'
> markers around the hex numbers just to make the user space automation
> we do have work).

Even if kptr_restrict is set?

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1470303 — Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more generally useful

On Wed, Aug 24, 2016 at 02:37:07PM -0500, Josh Poimboeuf wrote:
> On Wed, Aug 24, 2016 at 02:37:21PM -0400, Linus Torvalds wrote:
> > On Wed, Aug 24, 2016 at 2:22 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > I actively disable KASLR on my dev box and feed these hex numbers into
> > > addr2line -ie vmlinux to find where in the function we are.
> > >
> > > Having the option to make %pB generate them works for me.
> > 
> > Yeah, considering that this is the only place this is used, changing
> > %pB sounds quite reasonable.
> 
> There's now another use of '%pB' in proc_pid_stack() in the tip tree: I
> changed it to '%pB' from '%pS'.  But I think the modified '%pB' would
> work there as well.
> 
> > We could perhaps make %pB show the hex numbers and address (so pB
> > would expand to "[<hex>] symbolname".if
> > 
> >  (a) not randomizing (so the hex numbers _may_ be useful)
> > 
> >  (b) kptr_restrict is 0 (so the hex numbers are "safe" in the dmesg)
> > 
> > and fall back to just the symbolic name if either of those aren't true?
> 
> Do we really need to check for both?  '%pK' only checks kptr_restrict.
> I'd think we should be consistent with that.  And maybe there are some
> scenarios where the actual text addresses provide useful debug
> information if KASLR is enabled and kptr_restrict is zero.

So I was looking at implementing this, and I noticed that '%pK' prints
"pK-error" if it's called from interrupt context when kptr_restrict==1.
Because checking CAP_SYSLOG would be meaningless in that case.

I don't really understand the point of the "pK-error" thing.  Any reason
why we can't print zero, i.e., just degrade the kptr_restrict from 1 to
2 in an interrupt?

That would make the '%pK' code simpler and usable from interrupt
context.  Also it would make its behavior consistent with the proposed
'%pB' changes, and the kptr_restrict code could be shared between '%pK'
and '%pB'.

Kess (or others), any objections if I make that change?

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1470415 — Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more generally useful

On Thu, Aug 25, 2016 at 1:49 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Wed, Aug 24, 2016 at 02:37:07PM -0500, Josh Poimboeuf wrote:
>> On Wed, Aug 24, 2016 at 02:37:21PM -0400, Linus Torvalds wrote:
>> > On Wed, Aug 24, 2016 at 2:22 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>> > >
>> > > I actively disable KASLR on my dev box and feed these hex numbers into
>> > > addr2line -ie vmlinux to find where in the function we are.
>> > >
>> > > Having the option to make %pB generate them works for me.
>> >
>> > Yeah, considering that this is the only place this is used, changing
>> > %pB sounds quite reasonable.
>>
>> There's now another use of '%pB' in proc_pid_stack() in the tip tree: I
>> changed it to '%pB' from '%pS'.  But I think the modified '%pB' would
>> work there as well.
>>
>> > We could perhaps make %pB show the hex numbers and address (so pB
>> > would expand to "[<hex>] symbolname".if
>> >
>> >  (a) not randomizing (so the hex numbers _may_ be useful)
>> >
>> >  (b) kptr_restrict is 0 (so the hex numbers are "safe" in the dmesg)
>> >
>> > and fall back to just the symbolic name if either of those aren't true?
>>
>> Do we really need to check for both?  '%pK' only checks kptr_restrict.
>> I'd think we should be consistent with that.  And maybe there are some
>> scenarios where the actual text addresses provide useful debug
>> information if KASLR is enabled and kptr_restrict is zero.
>
> So I was looking at implementing this, and I noticed that '%pK' prints
> "pK-error" if it's called from interrupt context when kptr_restrict==1.
> Because checking CAP_SYSLOG would be meaningless in that case.
>
> I don't really understand the point of the "pK-error" thing.  Any reason
> why we can't print zero, i.e., just degrade the kptr_restrict from 1 to
> 2 in an interrupt?
>
> That would make the '%pK' code simpler and usable from interrupt
> context.  Also it would make its behavior consistent with the proposed
> '%pB' changes, and the kptr_restrict code could be shared between '%pK'
> and '%pB'.
>
> Kess (or others), any objections if I make that change?

I don't mind this becoming "0" on error. I suspect the rationale was
to make it a discoverable condition and to avoid confusion.

As far as expanding the usage, I'm still in favor, though there is
work planned to make kptr_restrict go away in favor of having
blacklisted destination buffers, etc. I'm hoping to have this as part
of the continuing usercopy hardening work.

Regardless, aren't these values being written to dmesg buffer?
Traditionally we've not bothered censoring values that go there, as
"dmesg_restrict" exists to protect those contents.

-Kees

-- 
Kees Cook
Nexus Security

[toc] | [prev] | [next] | [standalone]

#1470421 — Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more generally useful

On Thu, Aug 25, 2016 at 04:41:29PM -0400, Kees Cook wrote:
> On Thu, Aug 25, 2016 at 1:49 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Wed, Aug 24, 2016 at 02:37:07PM -0500, Josh Poimboeuf wrote:
> >> On Wed, Aug 24, 2016 at 02:37:21PM -0400, Linus Torvalds wrote:
> >> > On Wed, Aug 24, 2016 at 2:22 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> >> > >
> >> > > I actively disable KASLR on my dev box and feed these hex numbers into
> >> > > addr2line -ie vmlinux to find where in the function we are.
> >> > >
> >> > > Having the option to make %pB generate them works for me.
> >> >
> >> > Yeah, considering that this is the only place this is used, changing
> >> > %pB sounds quite reasonable.
> >>
> >> There's now another use of '%pB' in proc_pid_stack() in the tip tree: I
> >> changed it to '%pB' from '%pS'.  But I think the modified '%pB' would
> >> work there as well.
> >>
> >> > We could perhaps make %pB show the hex numbers and address (so pB
> >> > would expand to "[<hex>] symbolname".if
> >> >
> >> >  (a) not randomizing (so the hex numbers _may_ be useful)
> >> >
> >> >  (b) kptr_restrict is 0 (so the hex numbers are "safe" in the dmesg)
> >> >
> >> > and fall back to just the symbolic name if either of those aren't true?
> >>
> >> Do we really need to check for both?  '%pK' only checks kptr_restrict.
> >> I'd think we should be consistent with that.  And maybe there are some
> >> scenarios where the actual text addresses provide useful debug
> >> information if KASLR is enabled and kptr_restrict is zero.
> >
> > So I was looking at implementing this, and I noticed that '%pK' prints
> > "pK-error" if it's called from interrupt context when kptr_restrict==1.
> > Because checking CAP_SYSLOG would be meaningless in that case.
> >
> > I don't really understand the point of the "pK-error" thing.  Any reason
> > why we can't print zero, i.e., just degrade the kptr_restrict from 1 to
> > 2 in an interrupt?
> >
> > That would make the '%pK' code simpler and usable from interrupt
> > context.  Also it would make its behavior consistent with the proposed
> > '%pB' changes, and the kptr_restrict code could be shared between '%pK'
> > and '%pB'.
> >
> > Kess (or others), any objections if I make that change?

Ahem, Kees, sorry :-)

> I don't mind this becoming "0" on error. I suspect the rationale was
> to make it a discoverable condition and to avoid confusion.
> 
> As far as expanding the usage, I'm still in favor, though there is
> work planned to make kptr_restrict go away in favor of having
> blacklisted destination buffers, etc. I'm hoping to have this as part
> of the continuing usercopy hardening work.
> 
> Regardless, aren't these values being written to dmesg buffer?
> Traditionally we've not bothered censoring values that go there, as
> "dmesg_restrict" exists to protect those contents.

Ah, the plot thickens.  I didn't know about 'dmesg_restrict'.  So I
guess we don't have to restrict the stack dump addresses after all,
since the entire dmesg buffer is protected by syslog()?

If so, I'm thinking that expanding '%pB' wouldn't be worthwhile after
all, because its two users would have two different requirements for
printing the address: /proc/<pid>/stack needs to use kptr_restrict but
the unwinder doesn't.

In which case I think the current code is fine.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1470509 — Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more generally useful

On Thu, Aug 25, 2016 at 5:23 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Aug 25, 2016 2:08 PM, "Josh Poimboeuf" <jpoimboe@redhat.com> wrote:
>>
>> Ah, the plot thickens.  I didn't know about 'dmesg_restrict'.  So I
>> guess we don't have to restrict the stack dump addresses after all,
>> since the entire dmesg buffer is protected by syslog()?
>
> No.
>
> Guys, the whole dmesg_restrict thing is a joke. You can't restrict access to
> system messages in general. It's just a stupid idea.

I'm not advocating that it's a globally useful protection, I was just
trying to point out that so much stuff is already exposed in the
system log that it's likely not a great use of time to think about
censoring things there right now. Obviously if it both improves
debuggability _and_ removes raw addresses from the log, I'm all for
it. :)

-Kees

-- 
Kees Cook
Nexus Security

[toc] | [prev] | [next] | [standalone]

#1470528 — Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more generally useful

On Thu, Aug 25, 2016 at 02:23:35PM -0700, Linus Torvalds wrote:
> On Aug 25, 2016 2:08 PM, "Josh Poimboeuf" <jpoimboe@redhat.com> wrote:
> >
> > Ah, the plot thickens.  I didn't know about 'dmesg_restrict'.  So I
> > guess we don't have to restrict the stack dump addresses after all,
> > since the entire dmesg buffer is protected by syslog()?
> 
> No.
> 
> Guys, the whole dmesg_restrict thing is a joke. You can't restrict access
> to system messages in general. It's just a stupid idea.
> 
> So stop thinking like some theoretical security people - we have more than
> enough of those. Start thinking like *practical* people that actually care
> about the user experience, and interfaces, and then within those confined,
> think about security.
> 
> Because remember: if you don't get that security comes *second*, you're not
> a security person, you're just a joke. People actually *using* the system
> come first.

So I'm not all that familiar with this pointer restriction stuff.  And I
don't claim to be a security person.  I'm just trying to understand what
the real issues are so we can fix them.

And anyway I thought this whole discussion was trying to resolve a
security concern which you brought up: not leaking kernel addresses to
unprivileged user space.

> So stop with the idiotic security flags that same people cannot actually
> use, either because they restrict use too much, or because they are so
> expensive that they aren't practical by default.
> 
> The things is, we should strive to do something *useful*. And
> dmesg_restrict is not that.

For an oops, there are other opportunities for address leakage besides
the stack trace function addresses.  There's the raw stack dump which
dumps the first 12 stack entries.  And there's the register dump.  I'm
pretty sure we don't want to get rid of those.

I suppose we could come up with some innovative way to filter or
sanitize kernel addresses from the stack dump and the registers.  But
that probably hurts usability for kernel developers.

Another issue is that there are a lot of duplicate symbol names in the
kernel.  So the symbol name alone might not be enough to disambiguate
the function address.

Not to mention the fact that today there are a gazillion uses of
printk() with '%p' in the kernel.

So yes, dmesg_restrict sounds useful to me.  It's a way to prevent users
from seeing kernel addresses without affecting my ability to debug
issues.  For a locked down system, why would non-root users need to
access dmesg anyway?

> In contrast, striving to just get the symbol names - but not the hex
> addresses - is actually both more useful and more secure than what we have
> now.

How exactly does removing data from the stack dump make it more useful?

> And dammit, even addr2line should just be able to understand the "symbol+
> offset" format, so even that is a really bad reason to show the hex number.

Maybe addr2line *should* be able to understand "symbol+offset", but it
doesn't, so it breaks my workflow.  Is there another lightweight tool
out there (i.e., not gdb) which does it?

I also wonder if other tools might have similar issues.  Though I can't
think of anything off the top of my head.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1470539 — Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more generally useful

On Thu, Aug 25, 2016 at 8:19 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> For an oops, there are other opportunities for address leakage besides
> the stack trace function addresses.  There's the raw stack dump which
> dumps the first 12 stack entries.  And there's the register dump.  I'm
> pretty sure we don't want to get rid of those.

We actually probably *do* want to get rid of the stack dump. It's
likely not really useful any more, and more legacy noise.

The register contents we definitely don't want to remove, obviously.
But apart from EIP itself (and LR etc on other architectures), those
actually rather seldom contain code addresses, so that kind of data is
rather harder to misuse.

That said, if you can trigger an oops, you do quite likely have a
security problem already.

So oopses aren't necessarily the first thing to worry about. I'd worry
more about things like WARN_ON_ONCE() that are much more likely things
that might be triggerable (ie we do occasionally have things like
warning for legacy system calls that shouldn't be used any more)

> I suppose we could come up with some innovative way to filter or
> sanitize kernel addresses from the stack dump and the registers.  But
> that probably hurts usability for kernel developers.

Yeah, but see above: an oops really does tend to often be a security
issue on its own, especially if it can be triggered arbitrarily by an
attacker.

> Another issue is that there are a lot of duplicate symbol names in the
> kernel.  So the symbol name alone might not be enough to disambiguate
> the function address.

That is true. It's seldom a big issue, though. Nobody actually uses
the address for anything _anyway_, so people end up disambiguating
those things based on context regardless.

Again, something like addr2line could be an exception, but (a) that
thing is useless in most situations due to randomization and (b) if
you don't randomize then the whole discussion is moot.

Plus add2line could just show all options, and then you end up doing
human disambiguation anyway.

> Not to mention the fact that today there are a gazillion uses of
> printk() with '%p' in the kernel.

Yes. And some of them have been stupiud security issues on their own,
and have nothing to do with symbol names. See for example commit
31b0b385f69d ("nf_conntrack: avoid kernel pointer value leak in slab
name")

> So yes, dmesg_restrict sounds useful to me.  It's a way to prevent users
> from seeing kernel addresses without affecting my ability to debug
> issues.  For a locked down system, why would non-root users need to
> access dmesg anyway?

That's the point. It is only useful for locked-down systems.

But that also means that IT IS NOT USEFUL AS A SECURITY ARGUMENT -
since it's simply not relevant to most systems out there.

Most systems aren't locked down.

> How exactly does removing data from the stack dump make it more useful?

I actually spend time cleaning up commit messages in logs, because
useless data that isn't actually information (random hex numbers) is
actively detrimental.

It makes commit logs less legible.

It also makes it harder to parse dumps.

It's not useful. That makes it actively bad.

I probably look at more oops reports than most people. I have not
found the hex numbers useful for the last five years, because they are
just randomized crap.

The stack content thing just makes code scroll off the screen etc, for example.

So in order for data to be useful, it has to be more than "data". It
has to be "information". More random useless hex noise is not good.

                   Linus

[toc] | [prev] | [next] | [standalone]

#1470554 — Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more generally useful

On Thu, Aug 25, 2016 at 09:40:12PM -0700, Linus Torvalds wrote:
> On Thu, Aug 25, 2016 at 8:19 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > So yes, dmesg_restrict sounds useful to me.  It's a way to prevent users
> > from seeing kernel addresses without affecting my ability to debug
> > issues.  For a locked down system, why would non-root users need to
> > access dmesg anyway?
> 
> That's the point. It is only useful for locked-down systems.
> 
> But that also means that IT IS NOT USEFUL AS A SECURITY ARGUMENT -
> since it's simply not relevant to most systems out there.
> 
> Most systems aren't locked down.

Ok, so maybe removing kernel text addresses from the stack dump wouldn't
be the end of the world.

But I still don't quite understand your statement that dmesg_restrict is
only useful for locked down systems.

To prevent kernel address disclosure, it seems we already rely on the
user setting kptr_restrict today, otherwise I can do cat
/proc/self/stack and the game is already lost, right?

So what's the difference between expecting the user to set kptr_restrict
vs dmesg_restrict?

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1470725 — Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more generally useful

On Thu, Aug 25, 2016 at 11:12:40PM -0700, Linus Torvalds wrote:
> On Aug 25, 2016 10:57 PM, "Josh Poimboeuf" <jpoimboe@redhat.com> wrote:
> >
> > But I still don't quite understand your statement that dmesg_restrict is
> > only useful for locked down systems.
> >
> > To prevent kernel address disclosure, it seems we already rely on the
> > user setting kptr_restrict today, otherwise I can do cat
> > /proc/self/stack and the game is already lost, right?
> 
> The point is: kptr_restrict actually makes sense, and is widely useful. It
> doesn't really end up hurting normal things. It's a pretty targeted thing,
> and generally doesn't actually hurt. You can still do basic health
> monitoring without having to get elevated privileges, for example.
> 
> Even system maintainers don't want to be root all the time. In fact, I
> suspect that the better a system maintainer you are, the less you want to
> be root - but you'll still want to see logs etc.
> 
> So note the difference between kptr_restrict and dmesg_restrict.
> 
> One is useful in a pretty wide environment, the other simply is not.
> 
> > So what's the difference between expecting the user to set kptr_restrict
> > vs dmesg_restrict?
> 
> Do you see the difference now?
> 
> kptr_restrict simply doesn't hurt as much as dmesg_restrict, so you can
> enable it fairly widely by default.
> 
> That makes it the *much* better security option. Because security options
> that you can't enable aren't actually useful.

Yeah, at least for human-administered systems, that does make sense.
Grumpy sysadmins don't want to type "sudo dmesg" or "sudo journalctl"
because a) they don't like change; and b) using sudo adds risk.

And a security option which is never used is indeed useless.  So *maybe*
that's a good enough argument for expecting the user to only enable
kptr_restrict instead of both.

But with cloud, devops, mobile, embedded, IoT, [insert buzzword], it
seems most systems are actually managed by software nowadays.  Then the
above arguments don't seem to apply, and dmesg_restrict could still be
quite widely useful, no?

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1473702 — Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more generally useful

On Wed, Aug 24, 2016 at 02:37:21PM -0400, Linus Torvalds wrote:
> On Wed, Aug 24, 2016 at 2:22 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > I actively disable KASLR on my dev box and feed these hex numbers into
> > addr2line -ie vmlinux to find where in the function we are.
> >
> > Having the option to make %pB generate them works for me.
> 
> Yeah, considering that this is the only place this is used, changing
> %pB sounds quite reasonable.
> 
> We could perhaps make %pB show the hex numbers and address (so pB
> would expand to "[<hex>] symbolname".if
> 
>  (a) not randomizing (so the hex numbers _may_ be useful)
> 
>  (b) kptr_restrict is 0 (so the hex numbers are "safe" in the dmesg)
> 
> and fall back to just the symbolic name if either of those aren't true?
> 
> And obviously, if KALLSYMS isn't enabled, you always show hex
> numbers.. That's already the case (but we might want to add the "[<>}'
> markers around the hex numbers just to make the user space automation
> we do have work).

Here's a simple patch to skip the kernel text addresses in the stack
dump, based on top of the up-thread patch set.

It doesn't do the '%pB' thing because now in tip there's a non-printk
user of it in /proc/<pid>/stack, for which there are different rules
(print zeros on kptr_restrict vs skipping the number altogether, for
example).  And anyway that could maybe be added later.

It also doesn't check for randomization or kptr_restrict, which keeps it
simple and maximizes its "usefulness" ;-)

Thoughts?

---

From: Josh Poimboeuf <jpoimboe@redhat.com>
Subject: [PATCH] x86/dumpstack: don't print kernel text addresses in stack dump

Printing kernel text addresses in stack dumps is of questionable value,
especially now that address randomization is becoming common.

It can be a security issue because it leaks kernel addresses.

It also affects the usefulness of the stack dump.  Linus says:

  "I actually spend time cleaning up commit messages in logs, because
  useless data that isn't actually information (random hex numbers) is
  actively detrimental.

  It makes commit logs less legible.

  It also makes it harder to parse dumps.

  It's not useful. That makes it actively bad.

  I probably look at more oops reports than most people. I have not
  found the hex numbers useful for the last five years, because they are
  just randomized crap.

  The stack content thing just makes code scroll off the screen etc, for
  example."

The only real downside to removing these addresses is that they can be
used to disambiguate duplicate symbol names.  However such cases are
rare, and the context of the stack dump should be enough to be able to
figure it out.

Some kernel developers like to use addr2line to convert the addresses to
a file name and line (which only works without randomization).  They can
instead use gdb for that purpose:

  $ echo "list *driver_probe_device+0x223" |gdb vmlinux |grep "is in"
  (gdb) 0xffffffff815b5d83 is in driver_probe_device (/home/jpoimboe/git/linux/drivers/base/dd.c:378).

(But note that when there are duplicate symbol names, that will only
show the first symbol it finds.)

Here's an example of what a stack dump looks like after this change:

  kernel BUG at /home/jpoimboe/git/linux/include/linux/scatterlist.h:140!
  invalid opcode: 0000 [#1] PREEMPT SMP
  Modules linked in: ...
  CPU: 0 PID: 338 Comm: systemd-udevd Not tainted 4.8.0-rc3+ #7
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.9.1-1.fc24 04/01/2014
  task: ffff880078978000 task.stack: ffffc900008f0000
  RIP: sg_init_one+0x87/0xa0
  RSP: 0018:ffffc900008f3a68  EFLAGS: 00010246
  RAX: 0000000000000000 RBX: ffffc900008f3a98 RCX: 0000000000000028
  RDX: 0000000000000041 RSI: 0000000000000000 RDI: 00004100008f3a98
  RBP: ffffc900008f3a80 R08: 00000002a014a073 R09: ffffc900008f3aa0
  R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000008
  R13: ffffc900008f3aa0 R14: 0000000000000000 R15: 0000000000000001
  FS:  00007fa18fc20880(0000) GS:ffff88007d200000(0000) knlGS:0000000000000000
  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  CR2: 00007fa18fc29000 CR3: 00000000799f8000 CR4: 00000000001406f0
  Stack:
   ffff880079af7350 ffff880079905400 0000000000000000 ffffc900008f3ae0
   ffffffffa0196610 0000000000000001 00010000ffffffff 0000000087654321
   0000000000000002 0000000000000000 0000000000000000 0000000000000000
  Call Trace:
   __send_control_msg+0x80/0xf0 [virtio_console]
   virtcons_probe+0x256/0x400 [virtio_console]
   virtio_dev_probe+0x144/0x1e0 [virtio]
   driver_probe_device+0x223/0x430
   __driver_attach+0xe3/0xf0
   ? driver_probe_device+0x430/0x430
   bus_for_each_dev+0x73/0xc0
   driver_attach+0x1e/0x20
   bus_add_driver+0x173/0x270
   ? virtio_cons_early_init+0x1d/0x1d [virtio_console]
   driver_register+0x60/0xe0
   ? virtio_cons_early_init+0x1d/0x1d [virtio_console]
   register_virtio_driver+0x20/0x30 [virtio]
   init+0x9f/0xfe3 [virtio_console]
   do_one_initcall+0x50/0x180
   ? do_init_module+0x27/0x1f9
   ? rcu_read_lock_sched_held+0x45/0x80
   ? kmem_cache_alloc_trace+0x28a/0x2f0
   do_init_module+0x5f/0x1f9
   load_module+0x270b/0x2c20
   ? __symbol_put+0x90/0x90
   ? show_coresize+0x30/0x30
   ? vfs_read+0x121/0x140
   SYSC_finit_module+0xdf/0x110
   SyS_finit_module+0xe/0x10
   entry_SYSCALL_64_fastpath+0x1f/0xbd
  Code: ff 83 e2 03 48 c1 e8 0c 81 e3 ff 0f 00 00 45 89 65 14 48 c1 e0 06 41 89 5d 10 48 01 c8 48 09 d0 5b 49 89 45 08 41 5c 41 5d 5d c3 <0f> 0b 0f 0b 0f 0b 48 8b 0d fc c6 9a 00 eb a6 66 2e 0f 1f 84 00
  RIP: sg_init_one+0x87/0xa0 RSP: ffffc900008f3a68

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/include/asm/kdebug.h |  1 -
 arch/x86/kernel/dumpstack.c   | 18 ++++--------------
 arch/x86/kernel/process_32.c  |  7 +++----
 arch/x86/kernel/process_64.c  |  3 +--
 arch/x86/mm/fault.c           |  3 +--
 arch/x86/platform/uv/uv_nmi.c |  4 ++--
 6 files changed, 11 insertions(+), 25 deletions(-)

diff --git a/arch/x86/include/asm/kdebug.h b/arch/x86/include/asm/kdebug.h
index d318811..29a594a 100644
--- a/arch/x86/include/asm/kdebug.h
+++ b/arch/x86/include/asm/kdebug.h
@@ -21,7 +21,6 @@ enum die_val {
 	DIE_NMIUNKNOWN,
 };
 
-extern void printk_address(unsigned long address);
 extern void die(const char *, struct pt_regs *,long);
 extern int __must_check __die(const char *, struct pt_regs *, long);
 extern void show_stack_regs(struct pt_regs *regs);
diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index c6c6c39..a56c04c 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -29,14 +29,7 @@ static void printk_stack_address(unsigned long address, int reliable,
 				 char *log_lvl)
 {
 	touch_nmi_watchdog();
-	printk("%s [<%p>] %s%pB\n",
-		log_lvl, (void *)address, reliable ? "" : "? ",
-		(void *)address);
-}
-
-void printk_address(unsigned long address)
-{
-	pr_cont(" [<%p>] %pS\n", (void *)address, (void *)address);
+	printk("%s %s%pB\n", log_lvl, reliable ? "" : "? ", (void *)address);
 }
 
 /*
@@ -277,14 +270,11 @@ int __die(const char *str, struct pt_regs *regs, long err)
 		sp = kernel_stack_pointer(regs);
 		savesegment(ss, ss);
 	}
-	printk(KERN_EMERG "EIP: [<%08lx>] ", regs->ip);
-	print_symbol("%s", regs->ip);
-	printk(" SS:ESP %04x:%08lx\n", ss, sp);
+	printk(KERN_EMERG "EIP: %pS SS:ESP: %04x:%08lx\n",
+	       (void *)regs->ip, ss, sp);
 #else
 	/* Executive summary in case the oops scrolled away */
-	printk(KERN_ALERT "RIP ");
-	printk_address(regs->ip);
-	printk(" RSP <%016lx>\n", regs->sp);
+	printk(KERN_ALERT "RIP: %pS RSP: %016lx\n", (void *)regs->ip, regs->sp);
 #endif
 	return 0;
 }
diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index 404efdf..9f828a9 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -72,10 +72,9 @@ void __show_regs(struct pt_regs *regs, int all)
 		savesegment(gs, gs);
 	}
 
-	printk(KERN_DEFAULT "EIP: %04x:[<%08lx>] EFLAGS: %08lx CPU: %d\n",
-			(u16)regs->cs, regs->ip, regs->flags,
-			smp_processor_id());
-	print_symbol("EIP is at %s\n", regs->ip);
+	printk(KERN_DEFAULT "EIP: %pS\n", (void *)regs->ip);
+	printk(KERN_DEFAULT "EFLAGS: %08lx CPU: %d\n", regs->flags,
+		smp_processor_id());
 
 	printk(KERN_DEFAULT "EAX: %08lx EBX: %08lx ECX: %08lx EDX: %08lx\n",
 		regs->ax, regs->bx, regs->cx, regs->dx);
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index b812cd0..d250088 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -60,8 +60,7 @@ void __show_regs(struct pt_regs *regs, int all)
 	unsigned int fsindex, gsindex;
 	unsigned int ds, cs, es;
 
-	printk(KERN_DEFAULT "RIP: %04lx:[<%016lx>] ", regs->cs & 0xffff, regs->ip);
-	printk_address(regs->ip);
+	printk(KERN_DEFAULT "RIP: %pS\n", (void *)regs->ip);
 	printk(KERN_DEFAULT "RSP: %04lx:%016lx  EFLAGS: %08lx\n", regs->ss,
 			regs->sp, regs->flags);
 	printk(KERN_DEFAULT "RAX: %016lx RBX: %016lx RCX: %016lx\n",
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index dc80230..0e7b66c 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -679,8 +679,7 @@ show_fault_oops(struct pt_regs *regs, unsigned long error_code,
 		printk(KERN_CONT "paging request");
 
 	printk(KERN_CONT " at %p\n", (void *) address);
-	printk(KERN_ALERT "IP:");
-	printk_address(regs->ip);
+	printk(KERN_ALERT "IP: %pS\n", (void *)regs->ip);
 
 	dump_pagetable(address);
 }
diff --git a/arch/x86/platform/uv/uv_nmi.c b/arch/x86/platform/uv/uv_nmi.c
index cd5173a..8410e7d 100644
--- a/arch/x86/platform/uv/uv_nmi.c
+++ b/arch/x86/platform/uv/uv_nmi.c
@@ -387,8 +387,8 @@ static void uv_nmi_dump_cpu_ip_hdr(void)
 /* Dump Instruction Pointer info */
 static void uv_nmi_dump_cpu_ip(int cpu, struct pt_regs *regs)
 {
-	pr_info("UV: %4d %6d %-32.32s ", cpu, current->pid, current->comm);
-	printk_address(regs->ip);
+	pr_info("UV: %4d %6d %-32.32s %pS",
+		cpu, current->pid, current->comm, (void *)regs->ip);
 }
 
 /*
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1473731 — Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more generally useful

On Wed, Aug 31, 2016 at 9:53 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> Here's an example of what a stack dump looks like after this change:

Looks good, but it also shows:

>   RSP: 0018:ffffc900008f3a68  EFLAGS: 00010246

Ok, we know the stack pointer now...

>   RBP: ffffc900008f3a80 R08: 00000002a014a073 R09: ffffc900008f3aa0

.. and it looks like we have a frame pointer too, which is pretty
close to the stack pointer. So let's look at the stack dump:

>   Stack:
>    ffff880079af7350 ffff880079905400 0000000000000000 ffffc900008f3ae0
>    ffffffffa0196610 0000000000000001 00010000ffffffff 0000000087654321

Yeah, counting down, the frame pointer points to the next frame, and
right after that you see the return address: 0xffffffffa0196610.

So let's look at the call trace:

>   Call Trace:
>    __send_control_msg+0x80/0xf0 [virtio_console]

Ok, so now we can match up that value to a symbol.

And that means that now we can match up *all* symbols in that module.
Even if the module isn't readable on that machine, you can just look
it up by checking the distro modules on another machine.

And if it had been core kernel code, you'd haev had the whole core
kernel ofdfsets.

So I think the patch is good, and I think the oops looks great, but I
think we should also just remove the stack dump. Sure, the register
state *can* contain these things too, but almost never do (and didn't,
in this example).

The stack dump actually goes back to forever, and it used to be useful
back in 1992 or so. But it used to be useful mainly because stacks
were simpler and we didn't have very good call traces anyway. I
definitely remember having used them - I just do not remember having
used them in the last ten+ years.

Of course, it's still true that if you can trigger an oops, you've
likely already lost the security game, but since the stack dump is so
useless, let's aim to just remove it and make games like the above
harder.

I'm also sure that we probably have a lot of other addresses in dmesg
that we should make sure aren't leaked. I did a quick grep and found

 Base memory trampoline at [ffff8f5e00097000] 97000 size 24576
 percpu: Embedded 35 pages/cpu @ffff8f6236c00000 s103640 r8192 d31528 u262144
 Freeing SMP alternatives memory: 32K (ffffffffaaec1000 - ffffffffaaec9000)

and a few more, and didn't check if those might give load addresses
away, but it would be good to check.

                 Linus

[toc] | [prev] | [next] | [standalone]

#1474395 — Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more generally useful

On Wed, Aug 31, 2016 at 10:15:19AM -0700, Linus Torvalds wrote:
> So I think the patch is good, and I think the oops looks great, but I
> think we should also just remove the stack dump. Sure, the register
> state *can* contain these things too, but almost never do (and didn't,
> in this example).
> 
> The stack dump actually goes back to forever, and it used to be useful
> back in 1992 or so. But it used to be useful mainly because stacks
> were simpler and we didn't have very good call traces anyway. I
> definitely remember having used them - I just do not remember having
> used them in the last ten+ years.
> 
> Of course, it's still true that if you can trigger an oops, you've
> likely already lost the security game, but since the stack dump is so
> useless, let's aim to just remove it and make games like the above
> harder.

Yeah.  I'll do another patch to get rid of the raw stack dump (though
maybe I'll wait until the other patches get merged first so I don't have
patches flying around everywhere).

> I'm also sure that we probably have a lot of other addresses in dmesg
> that we should make sure aren't leaked. I did a quick grep and found
> 
>  Base memory trampoline at [ffff8f5e00097000] 97000 size 24576
>  percpu: Embedded 35 pages/cpu @ffff8f6236c00000 s103640 r8192 d31528 u262144
>  Freeing SMP alternatives memory: 32K (ffffffffaaec1000 - ffffffffaaec9000)
> 
> and a few more, and didn't check if those might give load addresses
> away, but it would be good to check.

On my system, a grep found these:

  $ dmesg |grep "ffff[8-e]\|ffffffff[8-e]"
  [    0.000000] found SMP MP-table at [mem 0x000f6b40-0x000f6b4f] mapped at [ffffa0b7000f6b40]
  [    0.000000] Base memory trampoline at [ffffa0b700099000] 99000 size 24576
  [    0.000000] percpu: Embedded 485 pages/cpu @ffffa0b77d200000 s1946904 r8192 d31464 u2097152
  [    0.475975] Freeing SMP alternatives memory: 32K (ffffffff9e309000 - ffffffff9e311000)
  [    2.656380] Freeing initrd memory: 10588K (ffffa0b736b42000 - ffffa0b737599000)
  [    4.444099] Freeing unused kernel memory: 3592K (ffffffff9df87000 - ffffffff9e309000)
  [    4.447080] Freeing unused kernel memory: 1352K (ffffa0b7288ae000 - ffffa0b728a00000)
  [    4.449517] Freeing unused kernel memory: 632K (ffffa0b728d62000 - ffffa0b728e00000)

The text starts at 0xffffa0b728000000 and 0xffffffff9d000000.  I think
only the "Freeing" messages would give away the vmlinux mappings.

I'm wonder if it might be useful to encode the addresses somehow; they
could conceivably be used to debug use-after-free issues.  Or we could
just remove them.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1474537 — Re: [PATCH 3/6] x86/dumpstack: make printk_stack_address() more generally useful

On Thu, Sep 1, 2016 at 6:09 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> I'm wonder if it might be useful to encode the addresses somehow; they
> could conceivably be used to debug use-after-free issues.  Or we could
> just remove them.

I suspect we should just remove them. I'm sure they are useful in
theory, but I suspect they were more useful back when the whole "free
init memory" was originally done.

These days, if we have a use-after-free, I suspect the init-mem
situation is the easiest situation by far. Compared to all the dynamic
allocations which are much more likely to show it anyway. So having
debug output for that case is likely not all that productive.

              Linus

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web