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

[Kees Cook]

On Thu, Aug 25, 2016 at 1:49 PM, Josh Poimboeuf <jpoimboe@xxxxxxxxxx> 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@xxxxxxxxxxxxx> 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