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]

Message-ID: <CAGXu5jJz_dHt-QNqPrYFnQNhj1oV_ux62SSi=PN=VEBm6oq0Mg@mail.gmail.com>
Date:   Thu, 10 Nov 2016 15:15:44 -0800
From:   Kees Cook <keescook@...omium.org>
To:     Greg KH <gregkh@...uxfoundation.org>,
        Peter Zijlstra <peterz@...radead.org>
Cc:     "kernel-hardening@...ts.openwall.com" 
        <kernel-hardening@...ts.openwall.com>,
        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

(PeterZ went missing from your reply? I've added him back to the thread...)

On Thu, Nov 10, 2016 at 2:27 PM, Greg KH <gregkh@...uxfoundation.org> wrote:
> On Thu, Nov 10, 2016 at 10:13:10PM +0100, Peter Zijlstra wrote:
>> On Thu, Nov 10, 2016 at 08:48:38PM +0000, Will Deacon wrote:
>> > > That said, I still don't much like this.
>> > >
>> > > I would much rather you make kref useful and use that. It still means
>> > > you get to audit all refcounts in the kernel, but hey, you had to do
>> > > that anyway.
>> >
>> > What needs to happen to kref to make it useful? Like many others, I've
>> > been guilty of using atomic_t for refcounts in the past.
>>
>> As it stands kref is a pointless wrapper. If it were to provide
>> something actually useful, like wrap protection, then it might actually
>> make sense to use it.
>
> It provides the correct cleanup ability for a reference count and the
> object it is in, so it's not all that pointless :)
>
> But I'm always willing to change it to make it work better for people,
> if kref did the wrapping protection (i.e. used a non-wrapping atomic
> type), then you would have that.  I thought that was what this patchset
> provided...
>
> And yes, this is a horridly large patchset.  I've looked at these
> changes, and in almost all of them, people are using atomic_t as merely
> a "counter" for something (sequences, rx/tx stats, etc), to get away
> without having to lock it with an external lock.
>
> So, does it make more sense to just provide a "pointless" api for this
> type of "counter" pattern:
>         counter_inc()
>         counter_dec()
>         counter_read()
>         counter_set()
>         counter_add()
>         counter_subtract()
> Those would use the wrapping atomic type, as they can wrap all they want
> and no one really is in trouble.  Once those changes are done, just make
> atomic_t not wrap and all should be fine, no other code should need to
> be changed.
>
> We can bikeshed on the function names for a while, to let everyone feel
> they contributed (counter, kcount, ksequence, sequence_t, cnt_t, etc.)...

Bikeshed: "counter" doesn't tell me anything about its behavior at max value.

> And yes, out-of-tree code will work differently, but really, the worse
> that could happen is their "sequence number" stops wrapping :)
>
> Would that be a better way to implement this?

A thought I had if the opt-out approach is totally unacceptable would
be to make it a CONFIG option that can toggle the risk as desired. It
would require splitting into three cases:

reference counters (say, "refcount" implemented with new atomic_nowrap_t)

statistic counters (say, "statcount" implemented with new atomic_wrap_t)

everything else (named "atomic_t", implemented as either
atomic_nowrap_t or atomic_wrap_t, depending on CONFIG)

-Kees

-- 
Kees Cook
Nexus Security