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

Groups > linux.kernel > #1692236 > unrolled thread

Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t

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 1/3] ipc: convert ipc_namespace.count from atomic_t to  refcount_t Andrew Morton <akpm@linux-foundation.org> - 2017-07-20 00:40 +0200
    Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to  refcount_t Davidlohr Bueso <dave@stgolabs.net> - 2017-07-20 01:00 +0200
      Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to  refcount_t Andrew Morton <akpm@linux-foundation.org> - 2017-07-20 01:00 +0200
        Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to  refcount_t Davidlohr Bueso <dave@stgolabs.net> - 2017-07-20 01:20 +0200
          Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t Kees Cook <keescook@chromium.org> - 2017-07-20 01:30 +0200
            Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t Kees Cook <keescook@chromium.org> - 2017-07-20 02:40 +0200
        Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to  refcount_t Ingo Molnar <mingo@kernel.org> - 2017-07-20 11:40 +0200
          Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t ebiederm@xmission.com (Eric W. Biederman) - 2017-07-20 14:50 +0200
            Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t Kees Cook <keescook@chromium.org> - 2017-07-20 17:20 +0200

#1692236 — Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t

On Sun, 09 Jul 2017 16:59:55 -0500 ebiederm@xmission.com (Eric W. Biederman) wrote:

> Elena Reshetova <elena.reshetova@intel.com> writes:
> 
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> 
> In this patch you can see all of the uses of the count.
> What accidental refcount overflows are possible?

I do rather dislike these conversions from the point of view of
performance overhead and general code bloat.  But I seem to have lost
that struggle and I don't think any of these are fastpath(?).

[toc] | [next] | [standalone]

#1692242

On Wed, 19 Jul 2017, Andrew Morton wrote:

>I do rather dislike these conversions from the point of view of
>performance overhead and general code bloat.  But I seem to have lost
>that struggle and I don't think any of these are fastpath(?).

Well, since we now have fd25d19 (locking/refcount: Create unchecked atomic_t
implementation), performance is supposed to be ok. It would be lovely to have
some actual numbers nonetheless.

Thanks,
Davidlohr

>

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

#1692247

On Wed, 19 Jul 2017 15:54:27 -0700 Davidlohr Bueso <dave@stgolabs.net> wrote:

> On Wed, 19 Jul 2017, Andrew Morton wrote:
> 
> >I do rather dislike these conversions from the point of view of
> >performance overhead and general code bloat.  But I seem to have lost
> >that struggle and I don't think any of these are fastpath(?).
> 
> Well, since we now have fd25d19 (locking/refcount: Create unchecked atomic_t
> implementation), performance is supposed to be ok.

Sure, things are OK for people who disable the feature.

But for people who want to enable the feature we really should minimize
the cost by avoiding blindly converting sites which simply don't need
it: simple, safe, old, well-tested code.  Why go and slow down such
code?  Need to apply some common sense here...

> It would be lovely to have
> some actual numbers nonetheless.

Very much so.

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

#1692260

On Wed, 19 Jul 2017, Andrew Morton wrote:

>On Wed, 19 Jul 2017 15:54:27 -0700 Davidlohr Bueso <dave@stgolabs.net> wrote:
>
>> On Wed, 19 Jul 2017, Andrew Morton wrote:
>>
>> >I do rather dislike these conversions from the point of view of
>> >performance overhead and general code bloat.  But I seem to have lost
>> >that struggle and I don't think any of these are fastpath(?).
>>
>> Well, since we now have fd25d19 (locking/refcount: Create unchecked atomic_t
>> implementation), performance is supposed to be ok.
>
>Sure, things are OK for people who disable the feature.
>
>But for people who want to enable the feature we really should minimize
>the cost by avoiding blindly converting sites which simply don't need
>it: simple, safe, old, well-tested code.  Why go and slow down such
>code?  Need to apply some common sense here...

Fair points.

>
>> It would be lovely to have
>> some actual numbers nonetheless.
>
>Very much so.

May I suggest using mmtests with the following config file:

https://github.com/gormanm/mmtests/blob/7e070a810bc0af92e592e5121d0ea75fada51aeb/configs/config-global-dhp__workload-ipc-scale-short

It will run two of Manfred's ipcscale sem benchmarks.

Thanks,
Davidlohr

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

#1692263 — Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t

On Wed, Jul 19, 2017 at 4:11 PM, Davidlohr Bueso <dave@stgolabs.net> wrote:
> On Wed, 19 Jul 2017, Andrew Morton wrote:
>
>> On Wed, 19 Jul 2017 15:54:27 -0700 Davidlohr Bueso <dave@stgolabs.net>
>> wrote:
>>
>>> On Wed, 19 Jul 2017, Andrew Morton wrote:
>>>
>>> >I do rather dislike these conversions from the point of view of
>>> >performance overhead and general code bloat.  But I seem to have lost
>>> >that struggle and I don't think any of these are fastpath(?).
>>>
>>> Well, since we now have fd25d19 (locking/refcount: Create unchecked
>>> atomic_t
>>> implementation), performance is supposed to be ok.
>>
>>
>> Sure, things are OK for people who disable the feature.

FWIW, it's off by default.

>>
>> But for people who want to enable the feature we really should minimize
>> the cost by avoiding blindly converting sites which simply don't need
>> it: simple, safe, old, well-tested code.  Why go and slow down such
>> code?  Need to apply some common sense here...

These are the very code paths we'd want to make sure are well
protected since people may never expect them to misbehave when some
"small change" goes in.

> Fair points.
>
>>> It would be lovely to have
>>> some actual numbers nonetheless.
>>
>> Very much so.
>
> May I suggest using mmtests with the following config file:
>
> https://github.com/gormanm/mmtests/blob/7e070a810bc0af92e592e5121d0ea75fada51aeb/configs/config-global-dhp__workload-ipc-scale-short
>
> It will run two of Manfred's ipcscale sem benchmarks.

I'll see if I can figure out how to use this for testing the fast
refcount protection:
https://lkml.org/lkml/2017/7/18/1223

Then we could see:

before conversion
after conversion
with CONFIG_REFCOUNT_FULL
with fast refcount protection

-Kees

-- 
Kees Cook
Pixel Security

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

#1692316 — Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t

On Wed, Jul 19, 2017 at 4:20 PM, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Jul 19, 2017 at 4:11 PM, Davidlohr Bueso <dave@stgolabs.net> wrote:
>> May I suggest using mmtests with the following config file:
>>
>> https://github.com/gormanm/mmtests/blob/7e070a810bc0af92e592e5121d0ea75fada51aeb/configs/config-global-dhp__workload-ipc-scale-short
>>
>> It will run two of Manfred's ipcscale sem benchmarks.
>
> I'll see if I can figure out how to use this for testing the fast
> refcount protection:
> https://lkml.org/lkml/2017/7/18/1223
>
> Then we could see:
>
> before conversion
> after conversion
> with CONFIG_REFCOUNT_FULL
> with fast refcount protection

I have no idea how to read this report. It seems to be mostly noise
(multiple baseline runs seem to show greater variability than compared
against the other possible results).

Test runs were atomic_t, atomic_t-2, refcount_t, refcount-full, and
refcount-fast. (Two baselines, refcount_t conversion, with FULL, and
with the fast implementation.) Output here:
http://pastebin.ubuntu.com/25129382/

-Kees

-- 
Kees Cook
Pixel Security

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

#1692692

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Wed, 19 Jul 2017 15:54:27 -0700 Davidlohr Bueso <dave@stgolabs.net> wrote:
> 
> > On Wed, 19 Jul 2017, Andrew Morton wrote:
> > 
> > >I do rather dislike these conversions from the point of view of
> > >performance overhead and general code bloat.  But I seem to have lost
> > >that struggle and I don't think any of these are fastpath(?).
> > 
> > Well, since we now have fd25d19 (locking/refcount: Create unchecked atomic_t
> > implementation), performance is supposed to be ok.
> 
> Sure, things are OK for people who disable the feature.

So with the WIP fast-refcount series from Kees:

	[PATCH v6 0/2] x86: Implement fast refcount overflow protection

I believe the robustness difference between optimized-refcount_t and 
full-refcount_t will be marginal.

I.e. we'll be able to have both higher API safety _and_ performance.

> But for people who want to enable the feature we really should minimize the cost 
> by avoiding blindly converting sites which simply don't need it: simple, safe, 
> old, well-tested code.  Why go and slow down such code?  Need to apply some 
> common sense here...

It's old, well-tested code _for existing, sane parameters_, until someone finds a 
decade old bug in one of these with an insane parameters no-one stumbled upon so 
far, and builds an exploit on top of it.

Only by touching all these places do we have a chance to improve things measurably 
in terms of reducing the probability of bugs.

Thanks,

	Ingo

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

#1692892 — Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t

Ingo Molnar <mingo@kernel.org> writes:

> * Andrew Morton <akpm@linux-foundation.org> wrote:
>
>> On Wed, 19 Jul 2017 15:54:27 -0700 Davidlohr Bueso <dave@stgolabs.net> wrote:
>> 
>> > On Wed, 19 Jul 2017, Andrew Morton wrote:
>> > 
>> > >I do rather dislike these conversions from the point of view of
>> > >performance overhead and general code bloat.  But I seem to have lost
>> > >that struggle and I don't think any of these are fastpath(?).
>> > 
>> > Well, since we now have fd25d19 (locking/refcount: Create unchecked atomic_t
>> > implementation), performance is supposed to be ok.
>> 
>> Sure, things are OK for people who disable the feature.
>
> So with the WIP fast-refcount series from Kees:
>
> 	[PATCH v6 0/2] x86: Implement fast refcount overflow protection
>
> I believe the robustness difference between optimized-refcount_t and 
> full-refcount_t will be marginal.
>
> I.e. we'll be able to have both higher API safety _and_ performance.
>
>> But for people who want to enable the feature we really should minimize the cost 
>> by avoiding blindly converting sites which simply don't need it: simple, safe, 
>> old, well-tested code.  Why go and slow down such code?  Need to apply some 
>> common sense here...
>
> It's old, well-tested code _for existing, sane parameters_, until someone finds a 
> decade old bug in one of these with an insane parameters no-one stumbled upon so 
> far, and builds an exploit on top of it.
>
> Only by touching all these places do we have a chance to improve things measurably 
> in terms of reducing the probability of bugs.

The more I hear people pushing the upsides of refcount_t without
considering the downsides the more I dislike it.

- refcount_t is really the wrong thing because it uses saturation
  semantics.  So by definition it includes a bug.

- refcount_t will only really prevent something if there is an extra
  increment.  That is not the kind of bug people are likely to make.

- refcount_t won't help if you have an extra decrement.  The bad
  use-after-free will still happen.

- refcount_t won't help if there is a memory stomp.  As with an extra
  decrement the bad use-after-free will still happen.
  
So all I see is a huge amount of code churn to implement a buggy (by
definition) refcounting API, that risks adding new bugs and only truly
helps with bugs that are unlikely in the first place.

I really don't think this is an obvious slam dunk.

Eric





  
 

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

#1693026 — Re: [PATCH 1/3] ipc: convert ipc_namespace.count from atomic_t to refcount_t

On Thu, Jul 20, 2017 at 5:34 AM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Ingo Molnar <mingo@kernel.org> writes:
>
>> * Andrew Morton <akpm@linux-foundation.org> wrote:
>>
>>> On Wed, 19 Jul 2017 15:54:27 -0700 Davidlohr Bueso <dave@stgolabs.net> wrote:
>>>
>>> > On Wed, 19 Jul 2017, Andrew Morton wrote:
>>> >
>>> > >I do rather dislike these conversions from the point of view of
>>> > >performance overhead and general code bloat.  But I seem to have lost
>>> > >that struggle and I don't think any of these are fastpath(?).
>>> >
>>> > Well, since we now have fd25d19 (locking/refcount: Create unchecked atomic_t
>>> > implementation), performance is supposed to be ok.
>>>
>>> Sure, things are OK for people who disable the feature.
>>
>> So with the WIP fast-refcount series from Kees:
>>
>>       [PATCH v6 0/2] x86: Implement fast refcount overflow protection
>>
>> I believe the robustness difference between optimized-refcount_t and
>> full-refcount_t will be marginal.
>>
>> I.e. we'll be able to have both higher API safety _and_ performance.
>>
>>> But for people who want to enable the feature we really should minimize the cost
>>> by avoiding blindly converting sites which simply don't need it: simple, safe,
>>> old, well-tested code.  Why go and slow down such code?  Need to apply some
>>> common sense here...
>>
>> It's old, well-tested code _for existing, sane parameters_, until someone finds a
>> decade old bug in one of these with an insane parameters no-one stumbled upon so
>> far, and builds an exploit on top of it.
>>
>> Only by touching all these places do we have a chance to improve things measurably
>> in terms of reducing the probability of bugs.
>
> The more I hear people pushing the upsides of refcount_t without
> considering the downsides the more I dislike it.
>
> - refcount_t is really the wrong thing because it uses saturation
>   semantics.  So by definition it includes a bug.

This is a feature, not a bug. :) If the kernel has a refcount overflow
flaw (which, in the pantheon of exploitable kernel bugs, is
_common_[1], as I've referenced earlier), then we're downgrading an
exploitable use-after-free to a harmless memory allocation leak. Even
if you don't include malicious attackers in the consideration, this
changes a memory corruption of unknown results into a memory leak.
That's actually an _improvement_ to availability and integrity.

> - refcount_t will only really prevent something if there is an extra
>   increment.  That is not the kind of bug people are likely to make.

Like I've said, this is common. This is usually a mistake in error
handling which forgets (or misplaces) a "put".

> - refcount_t won't help if you have an extra decrement.  The bad
>   use-after-free will still happen.

Yes, and not having a protected refcount_t will also allow a
use-after-free. There is no change here, so it's not a "downside" of
refcount_t. In fact, having gained the implicit annotation of
refcount_t being a refcounter (rather than a simple atomic_t) means
that auditing users is easier and more focused. This could reduce the
chance people make mistakes in the first place, especially since the
API is more constrained than atomic_t.

> - refcount_t won't help if there is a memory stomp.  As with an extra
>   decrement the bad use-after-free will still happen.

A stomp of the refcount_t value itself? Sure, and this remains as
vulnerable as atomic_t. This isn't a downside to refcount_t. And
again, since there _is_ checking of the value in places, it's possible
an actionable warning will be produced (though, yes, after the
use-after-free has been exposed), which is a benefit over simple
atomic_t. I mention this in the commit log ("better to maybe produce
the warning than be universally silent").

> So all I see is a huge amount of code churn to implement a buggy (by
> definition) refcounting API, that risks adding new bugs and only truly
> helps with bugs that are unlikely in the first place.

Given that the conversions alone have been uncovering refcount bugs
and that the implementation isn't "buggy" (it provides a specific set
of protections), I strongly disagree with your assessment.

> I really don't think this is an obvious slam dunk.

It entirely blocks a commonly exploitable flaw in the kernel. This
isn't a probabilistic mitigation, either. While I'm not sure I'd ever
describe a security protection as a slam dunk, I think this is up
there. :)

-Kees

[1] When I say "common", I'm speaking from the perspective of security
flaw frequency. The kernel sees about 1-2 high severity security flaws
a year (with an average lifetime of 5 years), and the
refcount-overflow use-after-free class of flaw is normally reliable
for attackers (and I'd classify as high severity). With 2016 seeing
two known separate refcount-overflow use-after-free flaws, this could
be better described as an epidemic, but I'll try to be less
inflammatory and just say "common".

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web