Re: [RFC v4 PATCH 00/13] HARDENED_ATOMIC

Re: [RFC v4 PATCH 00/13] HARDENED_ATOMIC

[Peter Zijlstra]

On Thu, Nov 10, 2016 at 10:24:35PM +0200, Elena Reshetova wrote:
> This series brings the PaX/Grsecurity PAX_REFCOUNT
> feature support to the upstream kernel. All credit for the
> feature goes to the feature authors.
> 
> The name of the upstream feature is HARDENED_ATOMIC
> and it is configured using CONFIG_HARDENED_ATOMIC and
> HAVE_ARCH_HARDENED_ATOMIC.
> 
> This series only adds x86 support; other architectures are expected
> to add similar support gradually.
> 
> More information about the feature can be found in the following
> commit messages.

No, this should be here. As it stands this is completely without
content.

In any case, NAK on this approach. Its the wrong way around.

_IF_ you want to do a non-wrapping variant, it must not be the default.

Since you need to audit every single atomic_t user in the kernel anyway,
it doesn't matter. But changing atomic_t to non-wrap by default is not
robust, if you forgot one, you can then trivially dos the kernel.

That said, I still don't much like this.

I would much rather you make kref useful and use that. It still means
you get to audit all refcounts in the kernel, but hey, you had to do
that anyway.

Re: [RFC v4 PATCH 00/13] HARDENED_ATOMIC

[Will Deacon]

On Thu, Nov 10, 2016 at 09:37:49PM +0100, Peter Zijlstra wrote:
> On Thu, Nov 10, 2016 at 10:24:35PM +0200, Elena Reshetova wrote:
> > This series brings the PaX/Grsecurity PAX_REFCOUNT
> > feature support to the upstream kernel. All credit for the
> > feature goes to the feature authors.
> > 
> > The name of the upstream feature is HARDENED_ATOMIC
> > and it is configured using CONFIG_HARDENED_ATOMIC and
> > HAVE_ARCH_HARDENED_ATOMIC.
> > 
> > This series only adds x86 support; other architectures are expected
> > to add similar support gradually.
> > 
> > More information about the feature can be found in the following
> > commit messages.
> 
> No, this should be here. As it stands this is completely without
> content.
> 
> In any case, NAK on this approach. Its the wrong way around.
> 
> _IF_ you want to do a non-wrapping variant, it must not be the default.
> 
> Since you need to audit every single atomic_t user in the kernel anyway,
> it doesn't matter. But changing atomic_t to non-wrap by default is not
> robust, if you forgot one, you can then trivially dos the kernel.

Completely agreed.

Whilst I understand that you're addressing an important and commonly
exploited vulnerability, this really needs to be opt-in rather than
opt-out given the prevalence of atomic_t users in the kernel. Having a
"hardened" kernel that does the wrong thing is useless.

> That said, I still don't much like this.
> 
> I would much rather you make kref useful and use that. It still means
> you get to audit all refcounts in the kernel, but hey, you had to do
> that anyway.

What needs to happen to kref to make it useful? Like many others, I've
been guilty of using atomic_t for refcounts in the past.

Will

Re: [RFC v4 PATCH 00/13] HARDENED_ATOMIC

[Kees Cook]

On Thu, Nov 10, 2016 at 12:48 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Thu, Nov 10, 2016 at 09:37:49PM +0100, Peter Zijlstra wrote:
>> On Thu, Nov 10, 2016 at 10:24:35PM +0200, Elena Reshetova wrote:
>> > This series brings the PaX/Grsecurity PAX_REFCOUNT
>> > feature support to the upstream kernel. All credit for the
>> > feature goes to the feature authors.
>> >
>> > The name of the upstream feature is HARDENED_ATOMIC
>> > and it is configured using CONFIG_HARDENED_ATOMIC and
>> > HAVE_ARCH_HARDENED_ATOMIC.
>> >
>> > This series only adds x86 support; other architectures are expected
>> > to add similar support gradually.
>> >
>> > More information about the feature can be found in the following
>> > commit messages.
>>
>> No, this should be here. As it stands this is completely without
>> content.
>>
>> In any case, NAK on this approach. Its the wrong way around.
>>
>> _IF_ you want to do a non-wrapping variant, it must not be the default.
>>
>> Since you need to audit every single atomic_t user in the kernel anyway,
>> it doesn't matter. But changing atomic_t to non-wrap by default is not
>> robust, if you forgot one, you can then trivially dos the kernel.
>
> Completely agreed.
>
> Whilst I understand that you're addressing an important and commonly
> exploited vulnerability, this really needs to be opt-in rather than
> opt-out given the prevalence of atomic_t users in the kernel. Having a
> "hardened" kernel that does the wrong thing is useless.

I (obviously) disagree. It's not useless. Such a kernel is totally
safe against refcount errors and would be exposed to DoS issues only
where mistakes were made. This is the fundamental shift here:

- we already have exploitable privilege escalation refcount flaws on a
regular basis
- this changes things to have zero exploitable refcount flaws now and
into the future
- the risk is bugs leading to DoS instead of the risk of exploitable flaws

That's the real trade.

>> That said, I still don't much like this.
>>
>> I would much rather you make kref useful and use that. It still means
>> you get to audit all refcounts in the kernel, but hey, you had to do
>> that anyway.
>
> What needs to happen to kref to make it useful? Like many others, I've
> been guilty of using atomic_t for refcounts in the past.

That's the point: expecting everyone to get this right and not miss
mistake from now into the future is not a solution. This solves the
privilege escalation issue for refcounts now and forever.

-Kees

-- 
Kees Cook
Nexus Security

Re: [kernel-hardening] Re: [RFC v4 PATCH 00/13] HARDENED_ATOMIC

[David Windsor]

On Thu, Nov 10, 2016 at 4:01 PM, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Nov 10, 2016 at 12:48 PM, Will Deacon <will.deacon@arm.com> wrote:
>> On Thu, Nov 10, 2016 at 09:37:49PM +0100, Peter Zijlstra wrote:
>>> On Thu, Nov 10, 2016 at 10:24:35PM +0200, Elena Reshetova wrote:
>>> > This series brings the PaX/Grsecurity PAX_REFCOUNT
>>> > feature support to the upstream kernel. All credit for the
>>> > feature goes to the feature authors.
>>> >
>>> > The name of the upstream feature is HARDENED_ATOMIC
>>> > and it is configured using CONFIG_HARDENED_ATOMIC and
>>> > HAVE_ARCH_HARDENED_ATOMIC.
>>> >
>>> > This series only adds x86 support; other architectures are expected
>>> > to add similar support gradually.
>>> >
>>> > More information about the feature can be found in the following
>>> > commit messages.
>>>
>>> No, this should be here. As it stands this is completely without
>>> content.
>>>
>>> In any case, NAK on this approach. Its the wrong way around.
>>>
>>> _IF_ you want to do a non-wrapping variant, it must not be the default.
>>>
>>> Since you need to audit every single atomic_t user in the kernel anyway,
>>> it doesn't matter. But changing atomic_t to non-wrap by default is not
>>> robust, if you forgot one, you can then trivially dos the kernel.
>>
>> Completely agreed.
>>
>> Whilst I understand that you're addressing an important and commonly
>> exploited vulnerability, this really needs to be opt-in rather than
>> opt-out given the prevalence of atomic_t users in the kernel. Having a
>> "hardened" kernel that does the wrong thing is useless.
>
> I (obviously) disagree. It's not useless. Such a kernel is totally
> safe against refcount errors and would be exposed to DoS issues only
> where mistakes were made. This is the fundamental shift here:
>
> - we already have exploitable privilege escalation refcount flaws on a
> regular basis
> - this changes things to have zero exploitable refcount flaws now and
> into the future
> - the risk is bugs leading to DoS instead of the risk of exploitable flaws
>
> That's the real trade.
>
>>> That said, I still don't much like this.
>>>
>>> I would much rather you make kref useful and use that. It still means
>>> you get to audit all refcounts in the kernel, but hey, you had to do
>>> that anyway.
>>
>> What needs to happen to kref to make it useful? Like many others, I've
>> been guilty of using atomic_t for refcounts in the past.
>

Discussions have been occurring since KSPP has begun: do we need a
specialized type for reference counters?  Oh, wait, we do: kref.
Wait!  kref is implemented with atomic_t.

So, what?  We obviously need an atomicity for a reference counter
type.  So, do we simply implement the HARDENED_ATOMIC protected
version of atomic_t "inside" of kref and leave atomic_t alone?

That would certainly reduce the number of users using atomic_t when
they don't need a refcounter: kernel users using kref probably meant
to use it as a reference counter, so wrap protection wouldn't cause a
DoS.

> That's the point: expecting everyone to get this right and not miss
> mistake from now into the future is not a solution. This solves the
> privilege escalation issue for refcounts now and forever.
>
> -Kees
>
> --
> Kees Cook
> Nexus Security

Re: [kernel-hardening] Re: [RFC v4 PATCH 00/13] HARDENED_ATOMIC

[Kees Cook]

On Thu, Nov 10, 2016 at 1:23 PM, David Windsor <dave@progbits.org> wrote:
> On Thu, Nov 10, 2016 at 4:01 PM, Kees Cook <keescook@chromium.org> wrote:
>> On Thu, Nov 10, 2016 at 12:48 PM, Will Deacon <will.deacon@arm.com> wrote:
>>> On Thu, Nov 10, 2016 at 09:37:49PM +0100, Peter Zijlstra wrote:
>>>> On Thu, Nov 10, 2016 at 10:24:35PM +0200, Elena Reshetova wrote:
>>>> > This series brings the PaX/Grsecurity PAX_REFCOUNT
>>>> > feature support to the upstream kernel. All credit for the
>>>> > feature goes to the feature authors.
>>>> >
>>>> > The name of the upstream feature is HARDENED_ATOMIC
>>>> > and it is configured using CONFIG_HARDENED_ATOMIC and
>>>> > HAVE_ARCH_HARDENED_ATOMIC.
>>>> >
>>>> > This series only adds x86 support; other architectures are expected
>>>> > to add similar support gradually.
>>>> >
>>>> > More information about the feature can be found in the following
>>>> > commit messages.
>>>>
>>>> No, this should be here. As it stands this is completely without
>>>> content.
>>>>
>>>> In any case, NAK on this approach. Its the wrong way around.
>>>>
>>>> _IF_ you want to do a non-wrapping variant, it must not be the default.
>>>>
>>>> Since you need to audit every single atomic_t user in the kernel anyway,
>>>> it doesn't matter. But changing atomic_t to non-wrap by default is not
>>>> robust, if you forgot one, you can then trivially dos the kernel.
>>>
>>> Completely agreed.
>>>
>>> Whilst I understand that you're addressing an important and commonly
>>> exploited vulnerability, this really needs to be opt-in rather than
>>> opt-out given the prevalence of atomic_t users in the kernel. Having a
>>> "hardened" kernel that does the wrong thing is useless.
>>
>> I (obviously) disagree. It's not useless. Such a kernel is totally
>> safe against refcount errors and would be exposed to DoS issues only
>> where mistakes were made. This is the fundamental shift here:
>>
>> - we already have exploitable privilege escalation refcount flaws on a
>> regular basis
>> - this changes things to have zero exploitable refcount flaws now and
>> into the future
>> - the risk is bugs leading to DoS instead of the risk of exploitable flaws
>>
>> That's the real trade.
>>
>>>> That said, I still don't much like this.
>>>>
>>>> I would much rather you make kref useful and use that. It still means
>>>> you get to audit all refcounts in the kernel, but hey, you had to do
>>>> that anyway.
>>>
>>> What needs to happen to kref to make it useful? Like many others, I've
>>> been guilty of using atomic_t for refcounts in the past.
>>
>
> Discussions have been occurring since KSPP has begun: do we need a
> specialized type for reference counters?  Oh, wait, we do: kref.
> Wait!  kref is implemented with atomic_t.
>
> So, what?  We obviously need an atomicity for a reference counter
> type.  So, do we simply implement the HARDENED_ATOMIC protected
> version of atomic_t "inside" of kref and leave atomic_t alone?
>
> That would certainly reduce the number of users using atomic_t when
> they don't need a refcounter: kernel users using kref probably meant
> to use it as a reference counter, so wrap protection wouldn't cause a
> DoS.

But it leaves all the newly added drivers that get it wrong (by not
using wrap-protected kref) exposed to privilege escalation. We have to
kill the entire class of vulnerability. It needs to be impossible to
get refcounting wrong from a pragmatic approach: we can't educate
everyone, so the infrastructure must be safe.

>> That's the point: expecting everyone to get this right and not miss
>> mistake from now into the future is not a solution. This solves the
>> privilege escalation issue for refcounts now and forever.

-Kees

-- 
Kees Cook
Nexus Security

Re: [kernel-hardening] Re: [RFC v4 PATCH 00/13] HARDENED_ATOMIC

[David Windsor]

On Thu, Nov 10, 2016 at 4:27 PM, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Nov 10, 2016 at 1:23 PM, David Windsor <dave@progbits.org> wrote:
>> On Thu, Nov 10, 2016 at 4:01 PM, Kees Cook <keescook@chromium.org> wrote:
>>> On Thu, Nov 10, 2016 at 12:48 PM, Will Deacon <will.deacon@arm.com> wrote:
>>>> On Thu, Nov 10, 2016 at 09:37:49PM +0100, Peter Zijlstra wrote:
>>>>> On Thu, Nov 10, 2016 at 10:24:35PM +0200, Elena Reshetova wrote:
>>>>> > This series brings the PaX/Grsecurity PAX_REFCOUNT
>>>>> > feature support to the upstream kernel. All credit for the
>>>>> > feature goes to the feature authors.
>>>>> >
>>>>> > The name of the upstream feature is HARDENED_ATOMIC
>>>>> > and it is configured using CONFIG_HARDENED_ATOMIC and
>>>>> > HAVE_ARCH_HARDENED_ATOMIC.
>>>>> >
>>>>> > This series only adds x86 support; other architectures are expected
>>>>> > to add similar support gradually.
>>>>> >
>>>>> > More information about the feature can be found in the following
>>>>> > commit messages.
>>>>>
>>>>> No, this should be here. As it stands this is completely without
>>>>> content.
>>>>>
>>>>> In any case, NAK on this approach. Its the wrong way around.
>>>>>
>>>>> _IF_ you want to do a non-wrapping variant, it must not be the default.
>>>>>
>>>>> Since you need to audit every single atomic_t user in the kernel anyway,
>>>>> it doesn't matter. But changing atomic_t to non-wrap by default is not
>>>>> robust, if you forgot one, you can then trivially dos the kernel.
>>>>
>>>> Completely agreed.
>>>>
>>>> Whilst I understand that you're addressing an important and commonly
>>>> exploited vulnerability, this really needs to be opt-in rather than
>>>> opt-out given the prevalence of atomic_t users in the kernel. Having a
>>>> "hardened" kernel that does the wrong thing is useless.
>>>
>>> I (obviously) disagree. It's not useless. Such a kernel is totally
>>> safe against refcount errors and would be exposed to DoS issues only
>>> where mistakes were made. This is the fundamental shift here:
>>>
>>> - we already have exploitable privilege escalation refcount flaws on a
>>> regular basis
>>> - this changes things to have zero exploitable refcount flaws now and
>>> into the future
>>> - the risk is bugs leading to DoS instead of the risk of exploitable flaws
>>>
>>> That's the real trade.
>>>
>>>>> That said, I still don't much like this.
>>>>>
>>>>> I would much rather you make kref useful and use that. It still means
>>>>> you get to audit all refcounts in the kernel, but hey, you had to do
>>>>> that anyway.
>>>>
>>>> What needs to happen to kref to make it useful? Like many others, I've
>>>> been guilty of using atomic_t for refcounts in the past.
>>>
>>
>> Discussions have been occurring since KSPP has begun: do we need a
>> specialized type for reference counters?  Oh, wait, we do: kref.
>> Wait!  kref is implemented with atomic_t.
>>
>> So, what?  We obviously need an atomicity for a reference counter
>> type.  So, do we simply implement the HARDENED_ATOMIC protected
>> version of atomic_t "inside" of kref and leave atomic_t alone?
>>
>> That would certainly reduce the number of users using atomic_t when
>> they don't need a refcounter: kernel users using kref probably meant
>> to use it as a reference counter, so wrap protection wouldn't cause a
>> DoS.
>
> But it leaves all the newly added drivers that get it wrong (by not
> using wrap-protected kref) exposed to privilege escalation. We have to
> kill the entire class of vulnerability. It needs to be impossible to
> get refcounting wrong from a pragmatic approach: we can't educate
> everyone, so the infrastructure must be safe.
>

I completely agree with you: I'm just bringing to light now the
arguments that were voiced about this (particularly by someone from
the PaX/grsecurity team; I forget which thread).

To address Peter's concerns about trivially DoS-ing a kernel, would it
be feasible to still use atomic_t as a protected type, but change
hardened_atomic_overflow() to not kill the process?  Just for a time:
we let this bake in distros for a while, fix all of the
yet-to-be-identified wrappable users of atomic_t, then re-enable
process killing?

>>> That's the point: expecting everyone to get this right and not miss
>>> mistake from now into the future is not a solution. This solves the
>>> privilege escalation issue for refcounts now and forever.
>
> -Kees
>
> --
> Kees Cook
> Nexus Security

Re: [kernel-hardening] Re: [RFC v4 PATCH 00/13] HARDENED_ATOMIC

[Peter Zijlstra]

On Thu, Nov 10, 2016 at 04:23:08PM -0500, David Windsor wrote:
> Discussions have been occurring since KSPP has begun: do we need a

Note that I was not included in any of that. If you hide in a corner on
the intartubes don't be surprised people have no clue what you're on
about.

> specialized type for reference counters?  Oh, wait, we do: kref.
> Wait!  kref is implemented with atomic_t.
> 
> So, what?  We obviously need an atomicity for a reference counter
> type.  So, do we simply implement the HARDENED_ATOMIC protected
> version of atomic_t "inside" of kref and leave atomic_t alone?

But you could provide a small subset of the atomic_t API for that, under
a different type.

That way you don't get utter shite like atomic_cmpxchg_wrap() for
instance.

>From what I can see only all the add/sub variants have overflow checks,
but all the operations get _wrap() prefixes, even where it doesn't make
any bloody sense. _wrap() on bitops?, _wrap() on cmpxchg(). You must be
bloody joking right?

Re: [RFC v4 PATCH 00/13] HARDENED_ATOMIC

[Peter Zijlstra]

On Thu, Nov 10, 2016 at 08:48:38PM +0000, Will Deacon wrote:
> > That said, I still don't much like this.
> > 
> > I would much rather you make kref useful and use that. It still means
> > you get to audit all refcounts in the kernel, but hey, you had to do
> > that anyway.
> 
> What needs to happen to kref to make it useful? Like many others, I've
> been guilty of using atomic_t for refcounts in the past.

As it stands kref is a pointless wrapper. If it were to provide
something actually useful, like wrap protection, then it might actually
make sense to use it.

Re: [RFC v4 PATCH 00/13] HARDENED_ATOMIC

[Kees Cook]

On Thu, Nov 10, 2016 at 1:13 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, Nov 10, 2016 at 08:48:38PM +0000, Will Deacon wrote:
>> > That said, I still don't much like this.
>> >
>> > I would much rather you make kref useful and use that. It still means
>> > you get to audit all refcounts in the kernel, but hey, you had to do
>> > that anyway.
>>
>> What needs to happen to kref to make it useful? Like many others, I've
>> been guilty of using atomic_t for refcounts in the past.
>
> As it stands kref is a pointless wrapper. If it were to provide
> something actually useful, like wrap protection, then it might actually
> make sense to use it.

An example of opt-in vs opt-out was the kernel's handling of format
strings. For many years, %n was part of the recognized format strings,
which meant that every time there was a format string error, the
kernel had a write-anything-anywhere primitive exposed to an attacker.
I, along with others, watched the kernel for format string errors, an
there were _always_ newly added misuses. It took too much time to
review, and burned lots of people's time. Instead, %n was removed
(again) from the kernel's format processor, and the entire class of
bugs switched from arbitrary writes to simple info leaks.

If we don't use opt-out for atomics, we're going to be in the same
situation where we have to constantly review every commit with an
atomic for exploitable refcount flaws. Kicking this down from
"privilege escalation" to "DoS" is a significant change in the
kernel's weaknesses.

-Kees

-- 
Kees Cook
Nexus Security

Re: [kernel-hardening] Re: [RFC v4 PATCH 00/13] HARDENED_ATOMIC

[Rik van Riel]

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

On Thu, 2016-11-10 at 13:23 -0800, Kees Cook wrote:

> If we don't use opt-out for atomics, we're going to be in the same
> situation where we have to constantly review every commit with an
> atomic for exploitable refcount flaws. Kicking this down from
> "privilege escalation" to "DoS" is a significant change in the
> kernel's weaknesses.

The only way I see around that would be to totally get
rid of the name atomic_t, forcing people with out of
tree code to use kref_t, or whatever name we pick for
the variable type that can wrap.

Something like checkpatch or a patch checking bot
could warn whenever new code is submitted that uses
the counter type that can wrap.

Not sure whether I like my idea :)

-- 
All Rights Reversed.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [kernel-hardening] Re: [RFC v4 PATCH 00/13] HARDENED_ATOMIC

[Greg KH]

On Thu, Nov 10, 2016 at 10:13:10PM +0100, Peter Zijlstra wrote:
> On Thu, Nov 10, 2016 at 08:48:38PM +0000, Will Deacon wrote:
> > > That said, I still don't much like this.
> > > 
> > > I would much rather you make kref useful and use that. It still means
> > > you get to audit all refcounts in the kernel, but hey, you had to do
> > > that anyway.
> > 
> > What needs to happen to kref to make it useful? Like many others, I've
> > been guilty of using atomic_t for refcounts in the past.
> 
> As it stands kref is a pointless wrapper. If it were to provide
> something actually useful, like wrap protection, then it might actually
> make sense to use it.

It provides the correct cleanup ability for a reference count and the
object it is in, so it's not all that pointless :)

But I'm always willing to change it to make it work better for people,
if kref did the wrapping protection (i.e. used a non-wrapping atomic
type), then you would have that.  I thought that was what this patchset
provided...

And yes, this is a horridly large patchset.  I've looked at these
changes, and in almost all of them, people are using atomic_t as merely
a "counter" for something (sequences, rx/tx stats, etc), to get away
without having to lock it with an external lock.

So, does it make more sense to just provide a "pointless" api for this
type of "counter" pattern:
	counter_inc()
	counter_dec()
	counter_read()
	counter_set()
	counter_add()
	counter_subtract()
Those would use the wrapping atomic type, as they can wrap all they want
and no one really is in trouble.  Once those changes are done, just make
atomic_t not wrap and all should be fine, no other code should need to
be changed.

We can bikeshed on the function names for a while, to let everyone feel
they contributed (counter, kcount, ksequence, sequence_t, cnt_t, etc.)...

And yes, out-of-tree code will work differently, but really, the worse
that could happen is their "sequence number" stops wrapping :)

Would that be a better way to implement this?

thanks,

greg k-h

Re: [kernel-hardening] Re: [RFC v4 PATCH 00/13] HARDENED_ATOMIC

[Kees Cook]

(PeterZ went missing from your reply? I've added him back to the thread...)

On Thu, Nov 10, 2016 at 2:27 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Nov 10, 2016 at 10:13:10PM +0100, Peter Zijlstra wrote:
>> On Thu, Nov 10, 2016 at 08:48:38PM +0000, Will Deacon wrote:
>> > > That said, I still don't much like this.
>> > >
>> > > I would much rather you make kref useful and use that. It still means
>> > > you get to audit all refcounts in the kernel, but hey, you had to do
>> > > that anyway.
>> >
>> > What needs to happen to kref to make it useful? Like many others, I've
>> > been guilty of using atomic_t for refcounts in the past.
>>
>> As it stands kref is a pointless wrapper. If it were to provide
>> something actually useful, like wrap protection, then it might actually
>> make sense to use it.
>
> It provides the correct cleanup ability for a reference count and the
> object it is in, so it's not all that pointless :)
>
> But I'm always willing to change it to make it work better for people,
> if kref did the wrapping protection (i.e. used a non-wrapping atomic
> type), then you would have that.  I thought that was what this patchset
> provided...
>
> And yes, this is a horridly large patchset.  I've looked at these
> changes, and in almost all of them, people are using atomic_t as merely
> a "counter" for something (sequences, rx/tx stats, etc), to get away
> without having to lock it with an external lock.
>
> So, does it make more sense to just provide a "pointless" api for this
> type of "counter" pattern:
>         counter_inc()
>         counter_dec()
>         counter_read()
>         counter_set()
>         counter_add()
>         counter_subtract()
> Those would use the wrapping atomic type, as they can wrap all they want
> and no one really is in trouble.  Once those changes are done, just make
> atomic_t not wrap and all should be fine, no other code should need to
> be changed.
>
> We can bikeshed on the function names for a while, to let everyone feel
> they contributed (counter, kcount, ksequence, sequence_t, cnt_t, etc.)...

Bikeshed: "counter" doesn't tell me anything about its behavior at max value.

> And yes, out-of-tree code will work differently, but really, the worse
> that could happen is their "sequence number" stops wrapping :)
>
> Would that be a better way to implement this?

A thought I had if the opt-out approach is totally unacceptable would
be to make it a CONFIG option that can toggle the risk as desired. It
would require splitting into three cases:

reference counters (say, "refcount" implemented with new atomic_nowrap_t)

statistic counters (say, "statcount" implemented with new atomic_wrap_t)

everything else (named "atomic_t", implemented as either
atomic_nowrap_t or atomic_wrap_t, depending on CONFIG)

-Kees

-- 
Kees Cook
Nexus Security

Re: [kernel-hardening] Re: [RFC v4 PATCH 00/13] HARDENED_ATOMIC

[Greg KH]

On Thu, Nov 10, 2016 at 03:15:44PM -0800, Kees Cook wrote:
> (PeterZ went missing from your reply? I've added him back to the thread...)

argh, not intentional at all, thanks for that...

> On Thu, Nov 10, 2016 at 2:27 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Thu, Nov 10, 2016 at 10:13:10PM +0100, Peter Zijlstra wrote:
> >> On Thu, Nov 10, 2016 at 08:48:38PM +0000, Will Deacon wrote:
> >> > > That said, I still don't much like this.
> >> > >
> >> > > I would much rather you make kref useful and use that. It still means
> >> > > you get to audit all refcounts in the kernel, but hey, you had to do
> >> > > that anyway.
> >> >
> >> > What needs to happen to kref to make it useful? Like many others, I've
> >> > been guilty of using atomic_t for refcounts in the past.
> >>
> >> As it stands kref is a pointless wrapper. If it were to provide
> >> something actually useful, like wrap protection, then it might actually
> >> make sense to use it.
> >
> > It provides the correct cleanup ability for a reference count and the
> > object it is in, so it's not all that pointless :)
> >
> > But I'm always willing to change it to make it work better for people,
> > if kref did the wrapping protection (i.e. used a non-wrapping atomic
> > type), then you would have that.  I thought that was what this patchset
> > provided...
> >
> > And yes, this is a horridly large patchset.  I've looked at these
> > changes, and in almost all of them, people are using atomic_t as merely
> > a "counter" for something (sequences, rx/tx stats, etc), to get away
> > without having to lock it with an external lock.
> >
> > So, does it make more sense to just provide a "pointless" api for this
> > type of "counter" pattern:
> >         counter_inc()
> >         counter_dec()
> >         counter_read()
> >         counter_set()
> >         counter_add()
> >         counter_subtract()
> > Those would use the wrapping atomic type, as they can wrap all they want
> > and no one really is in trouble.  Once those changes are done, just make
> > atomic_t not wrap and all should be fine, no other code should need to
> > be changed.
> >
> > We can bikeshed on the function names for a while, to let everyone feel
> > they contributed (counter, kcount, ksequence, sequence_t, cnt_t, etc.)...
> 
> Bikeshed: "counter" doesn't tell me anything about its behavior at max value.

True :)

> > And yes, out-of-tree code will work differently, but really, the worse
> > that could happen is their "sequence number" stops wrapping :)
> >
> > Would that be a better way to implement this?
> 
> A thought I had if the opt-out approach is totally unacceptable would
> be to make it a CONFIG option that can toggle the risk as desired. It
> would require splitting into three cases:
> 
> reference counters (say, "refcount" implemented with new atomic_nowrap_t)

These should almost always be just using a kref.  Yes, there are some
vfs reference counters that can't use a kref, but those are rare.  And
make kref use atomic_nowrap_t.

This should be a very rare type, hopefully.

> statistic counters (say, "statcount" implemented with new atomic_wrap_t)

Lots of these are also "sequences", that drivers rely on.  Hopefully
they can wrap as that's what happens today.  So that might not be the
best name, but naming is hard...

> everything else (named "atomic_t", implemented as either
> atomic_nowrap_t or atomic_wrap_t, depending on CONFIG)

Sounds reasonable, will that still give you the protection that you want
here?

thanks,

greg k-h

Re: [kernel-hardening] Re: [RFC v4 PATCH 00/13] HARDENED_ATOMIC

[Peter Zijlstra]

On Thu, Nov 10, 2016 at 03:15:44PM -0800, Kees Cook wrote:
> On Thu, Nov 10, 2016 at 2:27 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Thu, Nov 10, 2016 at 10:13:10PM +0100, Peter Zijlstra wrote:

> >> As it stands kref is a pointless wrapper. If it were to provide
> >> something actually useful, like wrap protection, then it might actually
> >> make sense to use it.
> >
> > It provides the correct cleanup ability for a reference count and the
> > object it is in, so it's not all that pointless :)

I really fail to see the point of:

	kref_put(&obj->ref, obj_free);

over:

	if (atomic_dec_and_test(&obj->ref))
		obj_free(obj);

Utter pointless wrappery afaict.

> > But I'm always willing to change it to make it work better for people,
> > if kref did the wrapping protection (i.e. used a non-wrapping atomic
> > type), then you would have that.  I thought that was what this patchset
> > provided...

So kref could simply do something like the below patch. But this patch
set completely rapes the atomic interface.

> > And yes, this is a horridly large patchset.  I've looked at these
> > changes, and in almost all of them, people are using atomic_t as merely
> > a "counter" for something (sequences, rx/tx stats, etc), to get away
> > without having to lock it with an external lock.
> >
> > So, does it make more sense to just provide a "pointless" api for this
> > type of "counter" pattern:
> >         counter_inc()
> >         counter_dec()
> >         counter_read()
> >         counter_set()
> >         counter_add()
> >         counter_subtract()
> > Those would use the wrapping atomic type, as they can wrap all they want
> > and no one really is in trouble.  Once those changes are done, just make
> > atomic_t not wrap and all should be fine, no other code should need to
> > be changed.

Still hate; yes there's a lot of stats which are just fine to wrap. But
there's a lot more atomic out there than refcounts and stats.

The locking primitives for example use atomic_t, and they really don't
want the extra overhead associated with this overflow crap, or the
namespace pollution.

> reference counters (say, "refcount" implemented with new atomic_nowrap_t)
> 
> statistic counters (say, "statcount" implemented with new atomic_wrap_t)
> 
> everything else (named "atomic_t", implemented as either
> atomic_nowrap_t or atomic_wrap_t, depending on CONFIG)

So the problem is that atomic_t has _much_ _much_ more than just add/sub
operations, which are the only ones modified for this patch set.

The whole wrap/nowrap thing doesn't make any bloody sense what so ever
for !arith operators like bitops or just plain cmpxchg.


---
 include/linux/kref.h | 55 ++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 51 insertions(+), 4 deletions(-)

diff --git a/include/linux/kref.h b/include/linux/kref.h
index e15828fd71f1..6af1f9344793 100644
--- a/include/linux/kref.h
+++ b/include/linux/kref.h
@@ -39,11 +39,26 @@ static inline void kref_init(struct kref *kref)
  */
 static inline void kref_get(struct kref *kref)
 {
-	/* If refcount was 0 before incrementing then we have a race
+	/*
+	 * If refcount was 0 before incrementing then we have a race
 	 * condition when this kref is freeing by some other thread right now.
 	 * In this case one should use kref_get_unless_zero()
 	 */
-	WARN_ON_ONCE(atomic_inc_return(&kref->refcount) < 2);
+	unsigned int old, new, val = atomic_read(&kref->refcount);
+
+	for (;;) {
+		WARN_ON_ONCE(val < 1);
+
+		new = val + 1;
+		if (new < val)
+			BUG(); /* overflow */
+
+		old = atomic_cmpxchg_relaxed(&kref->refcount, val, new);
+		if (old == val)
+			break;
+
+		val = old;
+	}
 }
 
 /**
@@ -67,9 +82,23 @@ static inline void kref_get(struct kref *kref)
 static inline int kref_sub(struct kref *kref, unsigned int count,
 	     void (*release)(struct kref *kref))
 {
+	unsigned int old, new, val = atomic_read(&kref->refcount);
+
 	WARN_ON(release == NULL);
 
-	if (atomic_sub_and_test((int) count, &kref->refcount)) {
+	for (;;) {
+		new = val - count;
+		if (new > val)
+			BUG(); /* underflow */
+
+		old = atomic_cmpxchg_release(&kref->refcount, val, new);
+		if (old == val)
+			break;
+
+		val = old;
+	}
+
+	if (!new) {
 		release(kref);
 		return 1;
 	}
@@ -102,6 +131,7 @@ static inline int kref_put_mutex(struct kref *kref,
 				 void (*release)(struct kref *kref),
 				 struct mutex *lock)
 {
+	/* XXX also fix */
 	WARN_ON(release == NULL);
 	if (unlikely(!atomic_add_unless(&kref->refcount, -1, 1))) {
 		mutex_lock(lock);
@@ -133,6 +163,23 @@ static inline int kref_put_mutex(struct kref *kref,
  */
 static inline int __must_check kref_get_unless_zero(struct kref *kref)
 {
-	return atomic_add_unless(&kref->refcount, 1, 0);
+	unsigned int old, new, val = atomic_read(&kref->refcount);
+
+	for (;;) {
+		if (!val)
+			return 0;
+
+		new = val + 1;
+		if (new < val)
+			BUG(); /* overflow */
+
+		old = atomic_cmpxchg_relaxed(&kref->refcount, val, new);
+		if (old == val)
+			break;
+
+		val = old;
+	}
+
+	return 1;
 }
 #endif /* _KREF_H_ */

Re: [kernel-hardening] Re: [RFC v4 PATCH 00/13] HARDENED_ATOMIC

[Colin Vidal]

On Fri, 2016-11-11 at 00:57 +0100, Peter Zijlstra wrote:
> On Thu, Nov 10, 2016 at 03:15:44PM -0800, Kees Cook wrote:
> > 
> > On Thu, Nov 10, 2016 at 2:27 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > > 
> > > On Thu, Nov 10, 2016 at 10:13:10PM +0100, Peter Zijlstra wrote:
> 
> > 
> > > 
> > > > 
> > > > As it stands kref is a pointless wrapper. If it were to provide
> > > > something actually useful, like wrap protection, then it might actually
> > > > make sense to use it.
> > > 
> > > It provides the correct cleanup ability for a reference count and the
> > > object it is in, so it's not all that pointless :)
> 
> I really fail to see the point of:
> 
> 	kref_put(&obj->ref, obj_free);
> 
> over:
> 
> 	if (atomic_dec_and_test(&obj->ref))
> 		obj_free(obj);
> 
> Utter pointless wrappery afaict.
> 
> > 
> > > 
> > > But I'm always willing to change it to make it work better for people,
> > > if kref did the wrapping protection (i.e. used a non-wrapping atomic
> > > type), then you would have that.  I thought that was what this patchset
> > > provided...
> 
> So kref could simply do something like the below patch. But this patch
> set completely rapes the atomic interface.
> 
> > 
> > > 
> > > And yes, this is a horridly large patchset.  I've looked at these
> > > changes, and in almost all of them, people are using atomic_t as merely
> > > a "counter" for something (sequences, rx/tx stats, etc), to get away
> > > without having to lock it with an external lock.
> > > 
> > > So, does it make more sense to just provide a "pointless" api for this
> > > type of "counter" pattern:
> > >         counter_inc()
> > >         counter_dec()
> > >         counter_read()
> > >         counter_set()
> > >         counter_add()
> > >         counter_subtract()
> > > Those would use the wrapping atomic type, as they can wrap all they want
> > > and no one really is in trouble.  Once those changes are done, just make
> > > atomic_t not wrap and all should be fine, no other code should need to
> > > be changed.
> 
> Still hate; yes there's a lot of stats which are just fine to wrap. But
> there's a lot more atomic out there than refcounts and stats.
> 
> The locking primitives for example use atomic_t, and they really don't
> want the extra overhead associated with this overflow crap, or the
> namespace pollution.
> 
> > 
> > reference counters (say, "refcount" implemented with new atomic_nowrap_t)
> > 
> > statistic counters (say, "statcount" implemented with new atomic_wrap_t)
> > 
> > everything else (named "atomic_t", implemented as either
> > atomic_nowrap_t or atomic_wrap_t, depending on CONFIG)
> 
> So the problem is that atomic_t has _much_ _much_ more than just add/sub
> operations, which are the only ones modified for this patch set.
> 
> The whole wrap/nowrap thing doesn't make any bloody sense what so ever
> for !arith operators like bitops or just plain cmpxchg.

I wonder if we didn't make a confusion between naming and
specifications. I have thought about Kees idea and what you're saying:

- The name "atomic_t" name didn't tell anything about if the variable
  can wrap or not. It just tells there is no race condition on
  concurrent access, nothing else, and users are well with that. OK
  then, we don't modify atomic_t, it makes sense.

- Hence, let's say a new type "refcount_t". It names exactly what we
  try to protect in this patch set. A much more simpler interface than
  atomic_t would be needed, and it protects on race condition and
  overflows (precisely what is expected of a counter reference). Not
  an opt-in solution, but it is much less invasive since we "just"
  have to modify the kref implementation and some vfs reference
  counters.

That didn't tell us how actually implements refcount_t: reuse some
atomic_t code or not (it would be simpler anyways, since we don't have
to implement the whole atomic_t interface). Still, this is another
problem.

Sounds better?

Thanks,

Colin

Re: [kernel-hardening] Re: [RFC v4 PATCH 00/13] HARDENED_ATOMIC

[Mark Rutland]

On Fri, Nov 11, 2016 at 01:29:21AM +0100, Colin Vidal wrote:
> On Fri, 2016-11-11 at 00:57 +0100, Peter Zijlstra wrote:
> > On Thu, Nov 10, 2016 at 03:15:44PM -0800, Kees Cook wrote:
> > > On Thu, Nov 10, 2016 at 2:27 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > On Thu, Nov 10, 2016 at 10:13:10PM +0100, Peter Zijlstra wrote:

> I wonder if we didn't make a confusion between naming and
> specifications. I have thought about Kees idea and what you're saying:
> 
> - The name "atomic_t" name didn't tell anything about if the variable
>   can wrap or not. It just tells there is no race condition on
>   concurrent access, nothing else, and users are well with that. OK
>   then, we don't modify atomic_t, it makes sense.
> 
> - Hence, let's say a new type "refcount_t". It names exactly what we
>   try to protect in this patch set. A much more simpler interface than
>   atomic_t would be needed, and it protects on race condition and
>   overflows (precisely what is expected of a counter reference). Not
>   an opt-in solution, but it is much less invasive since we "just"
>   have to modify the kref implementation and some vfs reference
>   counters.
> 
> That didn't tell us how actually implements refcount_t: reuse some
> atomic_t code or not (it would be simpler anyways, since we don't have
> to implement the whole atomic_t interface). Still, this is another
> problem.
> 
> Sounds better?

Regardless of atomic_t semantics, a refcount_t would be far more obvious
to developers than atomic_t and/or kref, and better documents the intent
of code using it.

We'd still see abuse of atomic_t (and so this won't solve the problems
Kees mentioned), but even as something orthogonal I think that would
make sense to have.

Thanks,
Mark.

Re: [kernel-hardening] Re: [RFC v4 PATCH 00/13] HARDENED_ATOMIC

[Peter Zijlstra]

On Fri, Nov 11, 2016 at 12:41:27PM +0000, Mark Rutland wrote:
> On Fri, Nov 11, 2016 at 01:29:21AM +0100, Colin Vidal wrote:

> > I wonder if we didn't make a confusion between naming and
> > specifications. I have thought about Kees idea and what you're saying:
> > 
> > - The name "atomic_t" name didn't tell anything about if the variable
> >   can wrap or not. It just tells there is no race condition on
> >   concurrent access, nothing else, and users are well with that. OK
> >   then, we don't modify atomic_t, it makes sense.
> > 
> > - Hence, let's say a new type "refcount_t". It names exactly what we
> >   try to protect in this patch set. A much more simpler interface than
> >   atomic_t would be needed, and it protects on race condition and
> >   overflows (precisely what is expected of a counter reference). Not
> >   an opt-in solution, but it is much less invasive since we "just"
> >   have to modify the kref implementation and some vfs reference
> >   counters.
> > 
> > That didn't tell us how actually implements refcount_t: reuse some
> > atomic_t code or not (it would be simpler anyways, since we don't have
> > to implement the whole atomic_t interface). Still, this is another
> > problem.
> > 
> > Sounds better?
> 
> Regardless of atomic_t semantics, a refcount_t would be far more obvious
> to developers than atomic_t and/or kref, and better documents the intent
> of code using it.
> 
> We'd still see abuse of atomic_t (and so this won't solve the problems
> Kees mentioned), but even as something orthogonal I think that would
> make sense to have.

Furthermore, you could implement that refcount_t stuff using
atomic_cmpxchg() in generic code. While that is sub-optimal for ll/sc
architectures you at least get generic code that works to get started.

Also, I suspect that if your refcounts are heavily contended, you'll
have other problems than the performance of these primitives.

Code for refcount_inc(), refcount_inc_not_zero() and
refcount_sub_and_test() can be copy-pasted from the kref patch I send
yesterday.

Re: [kernel-hardening] Re: [RFC v4 PATCH 00/13] HARDENED_ATOMIC

[Peter Zijlstra]

On Fri, Nov 11, 2016 at 01:47:55PM +0100, Peter Zijlstra wrote:
> On Fri, Nov 11, 2016 at 12:41:27PM +0000, Mark Rutland wrote:

> > Regardless of atomic_t semantics, a refcount_t would be far more obvious
> > to developers than atomic_t and/or kref, and better documents the intent
> > of code using it.
> > 
> > We'd still see abuse of atomic_t (and so this won't solve the problems
> > Kees mentioned), but even as something orthogonal I think that would
> > make sense to have.
> 
> Furthermore, you could implement that refcount_t stuff using
> atomic_cmpxchg() in generic code. While that is sub-optimal for ll/sc
> architectures you at least get generic code that works to get started.
> 
> Also, I suspect that if your refcounts are heavily contended, you'll
> have other problems than the performance of these primitives.
> 
> Code for refcount_inc(), refcount_inc_not_zero() and
> refcount_sub_and_test() can be copy-pasted from the kref patch I send
> yesterday.

A wee bit like so...

---
diff --git a/include/linux/refcount.h b/include/linux/refcount.h
new file mode 100644
index 000000000000..d1eae0d2345e
--- /dev/null
+++ b/include/linux/refcount.h
@@ -0,0 +1,75 @@
+#ifndef _LINUX_REFCOUNT_H
+#define _LINUX_REFCOUNT_H
+
+#include <linux/atomic.h>
+
+typedef struct refcount_struct {
+	atomic_t refs;
+} refcount_t;
+
+static inline void refcount_inc(refcount_t *r)
+{
+	unsigned int old, new, val = atomic_read(&r->refs);
+
+	for (;;) {
+		WARN_ON_ONCE(!val);
+
+		new = val + 1;
+		if (new < val)
+			BUG(); /* overflow */
+
+		old = atomic_cmpxchg_relaxed(&r->refs, val, new);
+		if (old == val)
+			break;
+
+		val = old;
+	}
+}
+
+static inline bool refcount_inc_not_zero(refcount_t *r)
+{
+	unsigned int old, new, val = atomic_read(&r->refs);
+
+	for (;;) {
+		if (!val)
+			return false;
+
+		new = val + 1;
+		if (new < val)
+			BUG(); /* overflow */
+
+		old = atomic_cmpxchg_relaxed(&r->refs, val, new);
+		if (old == val)
+			break;
+
+		val = old;
+	}
+
+	return true;
+}
+
+static inline bool refcount_sub_and_test(int i, refcount_t *r)
+{
+	unsigned int old, new, val = atomic_read(&r->refs);
+
+	for (;;) {
+		new = val - i;
+		if (new > val)
+			BUG(); /* underflow */
+
+		old = atomic_cmpxchg_release(&r->refs, val, new);
+		if (old == val)
+			break;
+
+		val = old;
+	}
+
+	return !new;
+}
+
+static inline bool refcount_dec_and_test(refcount_t *r)
+{
+	return refcount_sub_and_test(1, r);
+}
+
+#endif /* _LINUX_REFCOUNT_H */

Re: [kernel-hardening] Re: [RFC v4 PATCH 00/13] HARDENED_ATOMIC

[Thomas Gleixner]

On Fri, 11 Nov 2016, Peter Zijlstra wrote:
> A wee bit like so...
> +
> +static inline bool refcount_sub_and_test(int i, refcount_t *r)

Why would we want to expose that at all? refcount_inc() and
refcount_dec_and_test() is what is required for refcounting.

I know there are a few users of kref_sub() in tree, but that's all
undocumented voodoo, which should not be proliferated.

Thanks,

	tglx

Re: [kernel-hardening] Re: [RFC v4 PATCH 00/13] HARDENED_ATOMIC

[Peter Zijlstra]

On Fri, Nov 11, 2016 at 03:39:05PM +0100, Thomas Gleixner wrote:
> On Fri, 11 Nov 2016, Peter Zijlstra wrote:
> > A wee bit like so...
> > +
> > +static inline bool refcount_sub_and_test(int i, refcount_t *r)
> 
> Why would we want to expose that at all? refcount_inc() and
> refcount_dec_and_test() is what is required for refcounting.
> 
> I know there are a few users of kref_sub() in tree, but that's all
> undocumented voodoo, which should not be proliferated.

I tend to agree. There are a few other sites that do multiple get/put as
well using atomic_t.

So supporting them using refcount_t is trivial -- simply match the
atomic*() functions in semantics with added wrappery tests, but you're
right in that having these encourages 'creative' use which we would be
better off without.

Ideally the audit would include sanitizing this. Moreover, there really
is only a handfull of these creative user, so maybe we could just leave
them be.

Re: [kernel-hardening] Re: [RFC v4 PATCH 00/13] HARDENED_ATOMIC

[Peter Zijlstra]

On Fri, Nov 11, 2016 at 02:00:34PM +0100, Peter Zijlstra wrote:
> +static inline bool refcount_sub_and_test(int i, refcount_t *r)
> +{
> +	unsigned int old, new, val = atomic_read(&r->refs);
> +
> +	for (;;) {

regardless of the sub_and_test vs inc_and_test issue, this should
probably also have:

		if (val == UINT_MAX)
			return false;

such that we stay saturated. If for some reason someone can trigger more
dec's than inc's, we'd be hosed.

> +		new = val - i;
> +		if (new > val)
> +			BUG(); /* underflow */
> +
> +		old = atomic_cmpxchg_release(&r->refs, val, new);
> +		if (old == val)
> +			break;
> +
> +		val = old;
> +	}
> +
> +	return !new;
> +}

Re: [kernel-hardening] Re: [RFC v4 PATCH 00/13] HARDENED_ATOMIC

[Greg KH]

On Fri, Nov 11, 2016 at 12:57:14AM +0100, Peter Zijlstra wrote:
> On Thu, Nov 10, 2016 at 03:15:44PM -0800, Kees Cook wrote:
> > On Thu, Nov 10, 2016 at 2:27 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > > On Thu, Nov 10, 2016 at 10:13:10PM +0100, Peter Zijlstra wrote:
> 
> > >> As it stands kref is a pointless wrapper. If it were to provide
> > >> something actually useful, like wrap protection, then it might actually
> > >> make sense to use it.
> > >
> > > It provides the correct cleanup ability for a reference count and the
> > > object it is in, so it's not all that pointless :)
> 
> I really fail to see the point of:
> 
> 	kref_put(&obj->ref, obj_free);
> 
> over:
> 
> 	if (atomic_dec_and_test(&obj->ref))
> 		obj_free(obj);
> 
> Utter pointless wrappery afaict.

No, it's an abstraction that shows what you are trying to have the
driver code do (drop a reference), and is simple to track and verify
that the driver is doing stuff properly (automated tools can do it too).

It's also easier to review and audit, if I see a developer use a kref, I
know how to easily read the code to verify it works correctly.  If they
use a "raw" atomic, I need to spend more time and effort to review that
they got it all correctly (always test the same way, call the correct
function the same way, handle the lock that needs to be somewhere
involved here, etc.)

So it saves both new developers time and effort (how do I write a
reference count properly...) and experienced developers (easy to audit
and read), which is why we have kref in the first place.

I'm all for having it use a wrap-free type, if possible, to catch these
types of errors, and yes, you are right in that it would only take 3
functions to implement it correctly.  I'd love to have it, but please,
don't say that krefs are pointless, they aren't.

thanks,

greg k-h

Re: [RFC v4 PATCH 00/13] HARDENED_ATOMIC

[Kees Cook]

On Thu, Nov 10, 2016 at 12:37 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, Nov 10, 2016 at 10:24:35PM +0200, Elena Reshetova wrote:
>> This series brings the PaX/Grsecurity PAX_REFCOUNT
>> feature support to the upstream kernel. All credit for the
>> feature goes to the feature authors.
>>
>> The name of the upstream feature is HARDENED_ATOMIC
>> and it is configured using CONFIG_HARDENED_ATOMIC and
>> HAVE_ARCH_HARDENED_ATOMIC.
>>
>> This series only adds x86 support; other architectures are expected
>> to add similar support gradually.
>>
>> More information about the feature can be found in the following
>> commit messages.
>
> No, this should be here. As it stands this is completely without
> content.
>
> In any case, NAK on this approach. Its the wrong way around.
>
> _IF_ you want to do a non-wrapping variant, it must not be the default.

Unfortunately, we have to do it this way because there are so many
misuses of atomic_t, and they just keep appearing. We can't do opt-in
protections for the kernel -- we need to protect atomic_t and opt OUT
of the protection where it's not needed.

We must change the kernel culture to making things secure-by-default.
Without this, we're wasting our time and continuing to leave people
vulnerable every time some new driver lands that refcounts with
atomic_t. Since education is proven to not work, we have to harden the
_infrastructure_ of the kernel, of which atomic_t is a part.

> Since you need to audit every single atomic_t user in the kernel anyway,
> it doesn't matter. But changing atomic_t to non-wrap by default is not
> robust, if you forgot one, you can then trivially dos the kernel.

Correct: everything must be audited in either case. However, making a
mistake using opt-out means a DoS. Making a mistake using opt-in means
an exploitable kernel escalation. We must have the courage to
recognize this distinction. Right now, every refcount mistake is an
exploitable kernel flaw. Reducing this to a DoS is a giant
improvement.

> That said, I still don't much like this.
>
> I would much rather you make kref useful and use that. It still means
> you get to audit all refcounts in the kernel, but hey, you had to do
> that anyway.

This has already been suggested in the past, and suffers from the same
opt-in problem. I'll let Greg comment on it, though, as he's agreed
with going opt-out in the past when reviewing this work.

-Kees

-- 
Kees Cook
Nexus Security

Re: [kernel-hardening] Re: [RFC v4 PATCH 00/13] HARDENED_ATOMIC

[David Windsor]

On Thu, Nov 10, 2016 at 3:56 PM, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Nov 10, 2016 at 12:37 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>> On Thu, Nov 10, 2016 at 10:24:35PM +0200, Elena Reshetova wrote:
>>> This series brings the PaX/Grsecurity PAX_REFCOUNT
>>> feature support to the upstream kernel. All credit for the
>>> feature goes to the feature authors.
>>>
>>> The name of the upstream feature is HARDENED_ATOMIC
>>> and it is configured using CONFIG_HARDENED_ATOMIC and
>>> HAVE_ARCH_HARDENED_ATOMIC.
>>>
>>> This series only adds x86 support; other architectures are expected
>>> to add similar support gradually.
>>>
>>> More information about the feature can be found in the following
>>> commit messages.
>>
>> No, this should be here. As it stands this is completely without
>> content.
>>
>> In any case, NAK on this approach. Its the wrong way around.
>>
>> _IF_ you want to do a non-wrapping variant, it must not be the default.
>
> Unfortunately, we have to do it this way because there are so many
> misuses of atomic_t, and they just keep appearing. We can't do opt-in
> protections for the kernel -- we need to protect atomic_t and opt OUT
> of the protection where it's not needed.
>
> We must change the kernel culture to making things secure-by-default.
> Without this, we're wasting our time and continuing to leave people
> vulnerable every time some new driver lands that refcounts with
> atomic_t. Since education is proven to not work, we have to harden the
> _infrastructure_ of the kernel, of which atomic_t is a part.
>
>> Since you need to audit every single atomic_t user in the kernel anyway,
>> it doesn't matter. But changing atomic_t to non-wrap by default is not
>> robust, if you forgot one, you can then trivially dos the kernel.
>
> Correct: everything must be audited in either case. However, making a
> mistake using opt-out means a DoS. Making a mistake using opt-in means
> an exploitable kernel escalation. We must have the courage to
> recognize this distinction. Right now, every refcount mistake is an
> exploitable kernel flaw. Reducing this to a DoS is a giant
> improvement.
>

Agreed.  And once this DoS happens only once and gets reported, it's
solved forever.

What about the approach of changing the behavior or the overflow
response mechanism (hardened_atomic_overflow()) to only log the
overflow, rather than killing the offending process?  This way, it
could bake in distros for a while and all necessary conversions to
atomic_wrap_t can occur.  Then, we can turn back on the behavior of
killing the offending process.  While behavior after an overflow
occurs is undefined, this was happening before anyway.

>> That said, I still don't much like this.
>>
>> I would much rather you make kref useful and use that. It still means
>> you get to audit all refcounts in the kernel, but hey, you had to do
>> that anyway.
>
> This has already been suggested in the past, and suffers from the same
> opt-in problem. I'll let Greg comment on it, though, as he's agreed
> with going opt-out in the past when reviewing this work.
>
> -Kees
>
> --
> Kees Cook
> Nexus Security