Re: [kernel-hardening] [RFCv2 PATCH 00/18] refcount_t API + usage

[Peter Zijlstra <peterz@infradead.org>]

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).

Also, I agree GCC does a very poor job generating code from it. But
again, I've not had a report where refcounting is performance critical.
I've also been very busy with other work and haven't spend much if any
time on this since your last email.

If you want something done, contribute.