[kernel-hardening] Re: Conversion from atomic_t to refcount_t: summary of issues

[Boqun Feng <boqun.feng@gmail.com>]

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

On Thu, Dec 01, 2016 at 03:20:29PM -0800, Kees Cook wrote:
> On Thu, Dec 1, 2016 at 3:03 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Thu, Dec 01, 2016 at 04:31:16PM -0500, David Windsor wrote:
> >> Also, I'd like to point out that while identifying stats_t instances, I
> >> have found a similar distribution of non-standard functions (as agreed upon
> >> for the stats_t API).
> >
> >> First, usage of atomic_long_wrap_t (there currently isn't a stats_long_t
> >> planned for implementation):
> >
> > There isn't even a stats_t planned. I'm still very much not convinced
> > stats_t is needed or even makes sense.
> >
> > It wouldn't have any different semantics from atomic_t, and the only
> > argument Kees made was that reduced atomic_t usage would make it easier
> > to spot refcounts, but you're already building tools to find those.
> >
> > Once the tools work, who cares.
> 
> The tool is only part of the whole thing. By distinctly splitting the
> other major atomic_t usage pattern away from atomic_t, it solidifies a
> stats_t as NOT a reference counter. It's the slow feature-creep or bad
> example situations that I'd like to avoid. Also, tools won't catch
> everything, and doing manual inspection is much easier if we know a
> stats_t cannot be misused.
> 
> There doesn't seem to be a good reason NOT to have stats_t, beyond the
> work needed to create it and audit the places it should be used.
> 

So we currently don't have a clear semantics for stats_t, do we? What
kind of atomic_t should be replaced with stats_t? In the link David
pointed out, there are a few cases where a stats_t is put on a
correctness-related variable. I don't think that's a good place to use
stats_t.

Besides, there are many statistics-purpose variables in kernel which are
not atomic, given those, only calling atomic statistic variables stats_t
seems inappropriate and misleading.

How about provide a modifier, say __stats, like __rcu, __percpu. We can
add it to the types of the variables that are only for
statistics-purpose. I think it won't be difficult to find all related
callsites of a __stats with the help of some compiler frontend tools, we
can then detect a possible problem if we do a cmpxchg on a __stats,
which is unlikely a proper usage for a statistic variable. And we don't
need to change or use special APIs, we just need to mark variables and
fields.

Thoughts?

Regards,
Boqun

> -Kees
> 
> -- 
> Kees Cook
> Nexus Security

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]