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

Groups > linux.kernel > #1470303 > unrolled thread

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

  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

#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] | [next] | [standalone]

#1470415

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

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

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

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

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

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

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

Back to top | Article view | linux.kernel

csiph-web