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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 11 Nov 2016 13:47:55 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Mark Rutland <mark.rutland@....com>
Cc:     kernel-hardening@...ts.openwall.com,
        Kees Cook <keescook@...omium.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        Will Deacon <will.deacon@....com>,
        Elena Reshetova <elena.reshetova@...el.com>,
        Arnd Bergmann <arnd@...db.de>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <h.peter.anvin@...el.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [kernel-hardening] Re: [RFC v4 PATCH 00/13] HARDENED_ATOMIC

On Fri, Nov 11, 2016 at 12:41:27PM +0000, Mark Rutland wrote:
> On Fri, Nov 11, 2016 at 01:29:21AM +0100, Colin Vidal wrote:

> > I wonder if we didn't make a confusion between naming and
> > specifications. I have thought about Kees idea and what you're saying:
> > 
> > - The name "atomic_t" name didn't tell anything about if the variable
> >   can wrap or not. It just tells there is no race condition on
> >   concurrent access, nothing else, and users are well with that. OK
> >   then, we don't modify atomic_t, it makes sense.
> > 
> > - Hence, let's say a new type "refcount_t". It names exactly what we
> >   try to protect in this patch set. A much more simpler interface than
> >   atomic_t would be needed, and it protects on race condition and
> >   overflows (precisely what is expected of a counter reference). Not
> >   an opt-in solution, but it is much less invasive since we "just"
> >   have to modify the kref implementation and some vfs reference
> >   counters.
> > 
> > That didn't tell us how actually implements refcount_t: reuse some
> > atomic_t code or not (it would be simpler anyways, since we don't have
> > to implement the whole atomic_t interface). Still, this is another
> > problem.
> > 
> > Sounds better?
> 
> Regardless of atomic_t semantics, a refcount_t would be far more obvious
> to developers than atomic_t and/or kref, and better documents the intent
> of code using it.
> 
> We'd still see abuse of atomic_t (and so this won't solve the problems
> Kees mentioned), but even as something orthogonal I think that would
> make sense to have.

Furthermore, you could implement that refcount_t stuff using
atomic_cmpxchg() in generic code. While that is sub-optimal for ll/sc
architectures you at least get generic code that works to get started.

Also, I suspect that if your refcounts are heavily contended, you'll
have other problems than the performance of these primitives.

Code for refcount_inc(), refcount_inc_not_zero() and
refcount_sub_and_test() can be copy-pasted from the kref patch I send
yesterday.