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

[Kees Cook <keescook@chromium.org>]

(And now Greg went missing from the reply? Re-added...)

On Thu, Nov 10, 2016 at 11:50 PM, David Windsor <dave@progbits.org> wrote:
> On Thu, Nov 10, 2016 at 6:38 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> On Thu, Nov 10, 2016 at 03:15:44PM -0800, Kees Cook wrote:
> While I really, really don't want to go down the opt-in route, here
> are the three relevant types and their associated use cases:
>
> 1) kref: Used for honest-to-goodness reference counters that want
> overflow protection.  Uses a new type: atomic_nowrap_t that has
> HARDENED_ATOMIC protection.

Based on other feedback, it sounds like we're better off with
refcount_t (which kref could be implemented on top of). And refcount_t
would have a limited API: inc, dec_and_test (or whatever is determined
as sanely minimal).

> 2) statistical counters: Atomic in all cases, but wraps.

Yup. sequence_t seems to make the most sense on naming, I think. If we
want to get crazy, the type could be sequence_wrap_t.

> 3) atomic_t: All other users of atomics (locks, etc.).  Wrapping
> behavior depends on a CONFIG setting.

Correct: if CONFIG_PARANOID_ATOMIC (or something) is set, atomic_t is
implemented as refcount_t, otherwise as sequence_t.

As far as refcount_t is concerned, I worry using cmpxchg will be too
costly, but it's worth benchmarking.

I suspect the first pass at this style will be:

1) create refcount_t, API, and wrap detection
2) convert atomic_t-as-refcount users to refcount_t (coccinelle to
find "dec_and_test" followed by "free"?)
3) create sequence_t API
4) convert atomic_t-as-stat users to sequence_t
5) implement atomic_t as one-or-the-other based on CONFIG

1,2 and 3,4 can happen at the same time.

> We'd need to audit the kernel and identify all reference counter users
> of atomic_t and convert them to kref.  This isn't nearly as large a
> task as enumerating all statistical counter users of atomic_t.
>
> The third category, users of atomic_t that fit neither into the
> category of reference counters or stat counters, is the interesting
> case.  I'm drawing a blank as to which of these users would require
> overflow protection?  If none do, we could drastically reduce the size
> of this patchset: leave atomic_t alone and implement kref with
> atomic_nowrap_t.  Again, it's very late and I'm drawing a blank as to
> "misc" users of atomic_t (non-refcount and non-stat-counters).
>
>>> 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)
>>
>
> I don't like rerfcount/refcount_t: kref should be used.  In the same
> spirit, kstatcount might be more consistent.
>
> Pure, unadulterated bikeshedding.  =)

I suspect the mechanical changes to using krefcount_t will be easier
that switching to kref. Large comments around krefcount_t can be added
to encourage people to just use kref, though.

Peter, Will, Greg, how does this 5-step approach (above) sound to you?
It gets us explicit marking in the kernel for what is expected to be a
reference counter or a sequence counter, and give us the ability to go
paranoid with the remaining atomic_t users.

-Kees

-- 
Kees Cook
Nexus Security