[kernel-hardening] Long live %pK (was Re: [PATCH tip/core/rcu 02/20] torture: Prepare scripting for shift from %p to %pK)

[kernel-hardening] Long live %pK (was Re: [PATCH tip/core/rcu 02/20] torture: Prepare scripting for shift from %p to %pK)

[Michael Ellerman]

Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Sun, Dec 10, 2017 at 4:52 AM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>>>
>>>> Perhaps it should have printed a fixed, non-zero value for non-zero
>>>> pointers.
>>>
>>> I must leave this to the people who have a dog in that contest.  ;-)
>>
>> Since there is an ongoing discussion with security people near to %pK
>> and alike, I added Kees and Linus to Cc list.
>>
>> The proposed change can be done easily, though I have no knowledge
>> about possible implications.
>
> I'd rather make %pK act more like %p than have gratuitous differences.

But %pK has one crucial feature that %p does not, which is that %pK can
actually show the real value.

> I also think %pK is kind of pointless in general. It has not been a
> big success, and the whole "root or not" is kind of nasty anyway. Root
> in a container? Things like that.

At least with docker, root in a container doesn't get CAP_SYSLOG by
default, and so %pK works perfectly.

That is, root in the container can't see %pK things, but root on the
host can.

So consider /proc/vmallocinfo:

> So I think that if people worry  about leaking pointers, they should
> primarily go for:
>
>  - just use %p and now get the hashed value

Hopefully we can agree that the hashed value is not very useful when
you're looking at vmallocinfo. You're almost always trying to determine
if a value from an oops or elsewhere is contained within one of the
mappings, and for that you need to know the address ranges.

>  - if the hashed value is pointless, ask yourself whether the pointer
> itself is important. Maybe it should be removed?

A real bug we hit yesterday, a bad page fault in a *guest* that appears
to be trying to dereference a vmalloc address from the *host* (obviously
that should not happen). First step in debugging is to check if the
address lies within a valid mapping in /proc/vmallocinfo on the host. To
do that you need the pointers.

>  - as a last option, if you really  think the true pointer value is
> important, why is root so special, and maybe you should use %px and
> make sure you have proper sensible permissions.

/proc/vmallocinfo is already 0400.

But we definitely do not want to use %px for vmallocinfo, because that
would expose the values to root in containers.

> ..and %pK just isn't really the answer in any of those cases.

So in this case it seems %pK is a better answer than %p or %px.

I understand that the CAP_SYSLOG checking that %pK does is kind of
gross, but it does work in at least some useful cases like this.

What am I missing?

cheers

[kernel-hardening] Re: Long live %pK (was Re: [PATCH tip/core/rcu 02/20] torture: Prepare scripting for shift from %p to %pK)

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1301 bytes --]

This is a perfect example of just %pK being complete shit.

%pK doesn't actually do any file permissions right. It looks like it does
it, but it's just a hot mess of garbage.

And %pK doesn't even work the way you claim it does. Not in the general
case, and only with a particular value.

On Dec 11, 2017 21:26, "Michael Ellerman" <mpe@ellerman.id.au> wrote: I


I understand that the CAP_SYSLOG checking that %pK does is kind of
gross, but it does work in at least some useful cases like this.

What am I missing?


Just do the damn thing right, like /proc/kallsyms does these days.

With the proper open time cred check, not the wrong one at io time.

Which has the added advantage that it actually does the right thing even
when you don't have kptr_restrict set, or when you have patches to make it
print zero even for people with capabilities.

Don't depend on some random flag that has nothing to do with your actual
example and that has random values for security.

Just say no to kptr_restrict "logic". Your example basically depends
entirely on one particular setting, when (a) real distributions have a
different value and expose those pointers that your claim shouldn't be
exposed and (b) other people are pushing for values that will hide the
values that you claim area needed.

     Linus

[-- Attachment #2: Type: text/html, Size: 2176 bytes --]

[kernel-hardening] Re: Long live %pK (was Re: [PATCH tip/core/rcu 02/20] torture: Prepare scripting for shift from %p to %pK)

[Michael Ellerman]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> This is a perfect example of just %pK being complete shit.
>
> %pK doesn't actually do any file permissions right. It looks like it does
> it, but it's just a hot mess of garbage.
>
> And %pK doesn't even work the way you claim it does. Not in the general
> case, and only with a particular value.

Right. My email was only about the kptr_restrict = 1 case, but I didn't
actually make that clear.

But that's also sort of my point, it has multiple modes of operation,
which is useful.

> On Dec 11, 2017 21:26, "Michael Ellerman" <mpe@ellerman.id.au> wrote: I
> >
> >
> > I understand that the CAP_SYSLOG checking that %pK does is kind of
> > gross, but it does work in at least some useful cases like this.
> >
> > What am I missing?
>
>
> Just do the damn thing right, like /proc/kallsyms does these days.
>
> With the proper open time cred check, not the wrong one at io time.

OK, that's the piece I was missing - ie. what to do in the case where
%px for all users is too permissive but %p is not useful.

> Which has the added advantage that it actually does the right thing even
> when you don't have kptr_restrict set, or when you have patches to make it
> print zero even for people with capabilities.
>
> Don't depend on some random flag that has nothing to do with your actual
> example and that has random values for security.

> Just say no to kptr_restrict "logic". Your example basically depends
> entirely on one particular setting, when (a) real distributions have a
> different value and expose those pointers that your claim shouldn't be
> exposed and (b) other people are pushing for values that will hide the
> values that you claim area needed.

I'm still a bit confused by the above. Because kallsyms which is your
example of how to do it right, still uses kptr_restrict. I get that it
also checks kallsyms_for_perf(), but that's only in the
kptr_restrict = 0 case.

Anyway, I'll do a patch for vmallocinfo to do the CAP_SYSLOG check at
open time, and use that to decide if it should print 0 or the address.

I can't think of any other flag or setting to sensibly determine if
vmallocinfo should be visible, so I've just done:

	if (kptr_restrict < 2 && has_capability_noaudit(current, CAP_SYSLOG))
		priv->show_addrs = true;
	else
		priv->show_addrs = false;

So basically visible to root, unless kptr_restrict == 2, otherwise not
visible. 

cheers

[kernel-hardening] Re: Long live %pK (was Re: [PATCH tip/core/rcu 02/20] torture: Prepare scripting for shift from %p to %pK)

[Linus Torvalds]

On Wed, Dec 13, 2017 at 4:59 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> Right. My email was only about the kptr_restrict = 1 case, but I didn't
> actually make that clear.
>
> But that's also sort of my point, it has multiple modes of operation,
> which is useful.

No it isn't.

It's completely useless. Let me count the ways:

 - it ties a lot of completely unrelated things together in illogical
ways. What if you want vmallocinfo for debuggability, but not
something else?

 - we've had it for most of a decade, and few people use it, and it's
not actually fixed any real security holes. It has only helped on
particular setups, not in general.

> OK, that's the piece I was missing - ie. what to do in the case where
> %px for all users is too permissive but %p is not useful.

Right. THAT IS THE WHOLE POINT OF THE NEW %p BEHAVIOR.

Make people actually _think_ about the things they see, and hopefully
just remove it entirely. Not the total idiocy that was %pK that never
resulted in any actual improvement anywhere.

%pK was the "sprinkle some crack on him, let's get out of here"
approach to security. It's bogus shit. Don't do it.

Seriously. It's been sprinkled around randomly just to make random
people feel like they did something. IT DID NOTHING.

The 'K' literally stands for "krack". Because spelling wasn't the
strong part of the thing either.

> I'm still a bit confused by the above. Because kallsyms which is your
> example of how to do it right, still uses kptr_restrict. I get that it
> also checks kallsyms_for_perf(), but that's only in the
> kptr_restrict = 0 case.

Yeah, it was probably a mistake, but I didn't want to change the old behavior.

> Anyway, I'll do a patch for vmallocinfo to do the CAP_SYSLOG check at
> open time, and use that to decide if it should print 0 or the address.

.. don't use CAP_SYSLOG, at least without thihnking about it.  That
was just another mistake of mine in thinking that "let's keep the old
behavior" is a good idea.

Seriously, what does CAP_SYSLOG have to do with kernel address
debugging? Nothing, really.

I suspect CAP_SYS_ADMIN is a much saner thing to use.

Ask yourself: who really should get access to vmalloc addresses?

                 Linus