[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2236FBA76BA1254E88B949DDB74E612B41C3F3D8@IRSMSX102.ger.corp.intel.com>
Date: Thu, 19 Jan 2017 16:00:13 +0000
From: "Reshetova, Elena" <elena.reshetova@...el.com>
To: David Windsor <dwindsor@...il.com>, Peter Zijlstra <peterz@...radead.org>
CC: Eric Biggers <ebiggers3@...il.com>, "kernel-hardening@...ts.openwall.com"
<kernel-hardening@...ts.openwall.com>, "keescook@...omium.org"
<keescook@...omium.org>, "arnd@...db.de" <arnd@...db.de>,
"tglx@...utronix.de" <tglx@...utronix.de>, "mingo@...hat.com"
<mingo@...hat.com>, "Anvin, H Peter" <h.peter.anvin@...el.com>,
"will.deacon@....com" <will.deacon@....com>, "gregkh@...uxfoundation.org"
<gregkh@...uxfoundation.org>
Subject: RE: [RFCv2 PATCH 00/18] refcount_t API + usage
On Thursday, January 19, 2017, Peter Zijlstra <peterz@...radead.org> wrote:
> On Wed, Jan 18, 2017 at 01:52:47PM -0800, Eric Biggers wrote:
>> There seems to be a lot of focus on converting things to use refcount_t but much
>> less focus on providing a refcount_t implementation that actually meets the
>> performance and security goals of the feature.
>
> And here you go again... :-(
>
> The refcount_t implementation does meet the security goals afaict, it
> has full saturation semantics, which means an overflow bug gets turned
> into a resource leak.
>
> That covers the entirely of the security goal. If there is more, you'll
> need to spell it out.
>
> As for performance, you didn't reply to my earlier email on the subject.
>
>> Notably, the proposed patchset
>> provides no information about why the proposed implementation was chosen over
>> the PaX implementation (note that I'm talking about the actual implementation of
>> safe reference counts, not the atomic_t/atomic_unchecked_t division) which as
>> I've already mentioned is much more efficient (less bloated and faster) while
>> still meeting the security goal.
>
> You again failed to reply to my last email on the subject. The initial
> PaX thing was broken as heck, only later did you mention it got fixed. I
> told you we could change to that for x86 if it could be proven to be
> equivalent.
>
> If you want to expedite matters, provide said proof.
>
> The scheme does not make sense for LL/SC based architectures though, so
> its not something that belongs in generic code.
>
>> I'm especially worried that people will be put
>> in a position where they need to take performance concerns into account when
>> deciding whether to use refcount_t or not.
>
> First show a place where refcounting is performance critical, then we
> can see how much effort is required.
>
>> And the patch even still includes
>> the "don't allow incrementing a zero refcount" check which AFAICS is bogus from
>> a security perspective.
>
> Because use-after-free isn't a security problem, right? Reference
> counting semantics are fairly clear that 0 means it is, or is going to
> be, free()'ed. How does allowing to increment at that point make any
> sense?
>
>> Even if you and Peter disagree with the comments that I and also PaX Team have
>> made, the patch must at least explain the design decisions made.
>
> It was constructed as a generic atomic with saturation semantics because
> what was said PaX had was broken as hell (note, I have myself never
> looked at PaX code and have only seen what was sent me as derived code).
> If that later got fixed, or the derived code was buggy or whatever, your
> earlier email was the first I heard of that, and that was well after I
> wrote refcount_t.
>
> So the design decision was broken vs not-broken.
>
> Also, refcount_t is written using generic primitives (not arch
> specific), to avoid arch dependencies and provide a common
> implementation to determine semantics. That does not mean architectures
> cannot implement their own later on (matching semantics).
>
>I think it's important to emphasize this: the generic refcount_t implementation you provided is the default, arch-independent implementation. The original PaX code had a similar system in which default, arch-independent implementations were provided >(defined in include/asm-generic/atomic*.h, if memory serves). Individual architectures overrode those definitions with their own, possibly optimized, versions.
>Note that figuring out how this system of overridden implementations works was pretty difficult, so we may want to investigate whether or not there's an easier way to do what we want, or at least document where/how these types' definitions are >resolved. I have notes on this, and I bet Elena and Hans do as well.
If we go this path, I think it would be easier than before since we need to provide only a small subset of functions.
Also, the biggest headaches were actually atomic64_t and local_t but since we haven't found any use cases for them for refcount_t,
it is really much easier without.
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
