linux-kernel - Re: [kernel-hardening] [PATCH v5 3/3] x86/refcount: Implement fast refcount overflow protection

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: <CAGXu5jLzL3URMvF81EbH1Bt74MnwL9-25AU41yQnbEBcWA7J8Q@mail.gmail.com>
Date:   Thu, 29 Jun 2017 15:05:44 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Li Kun <hw.likun@...wei.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Christoph Hellwig <hch@...radead.org>,
        Peter Zijlstra <peterz@...radead.org>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        PaX Team <pageexec@...email.hu>, Jann Horn <jannh@...gle.com>,
        Eric Biggers <ebiggers3@...il.com>,
        Elena Reshetova <elena.reshetova@...el.com>,
        Hans Liljestrand <ishkamiel@...il.com>,
        David Windsor <dwindsor@...il.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        Ingo Molnar <mingo@...hat.com>,
        Alexey Dobriyan <adobriyan@...il.com>,
        "Serge E. Hallyn" <serge@...lyn.com>, arozansk@...hat.com,
        Davidlohr Bueso <dave@...olabs.net>,
        Manfred Spraul <manfred@...orfullife.com>,
        "axboe@...nel.dk" <axboe@...nel.dk>,
        James Bottomley <James.Bottomley@...senpartnership.com>,
        "x86@...nel.org" <x86@...nel.org>, Ingo Molnar <mingo@...nel.org>,
        Arnd Bergmann <arnd@...db.de>,
        "David S. Miller" <davem@...emloft.net>,
        Rik van Riel <riel@...hat.com>,
        linux-arch <linux-arch@...r.kernel.org>,
        "kernel-hardening@...ts.openwall.com" 
        <kernel-hardening@...ts.openwall.com>,
        "Wangkai (Morgan, Euler)" <morgan.wang@...wei.com>
Subject: Re: [kernel-hardening] [PATCH v5 3/3] x86/refcount: Implement fast
 refcount overflow protection

On Wed, Jun 28, 2017 at 9:13 PM, Li Kun <hw.likun@...wei.com> wrote:
> 在 2017/5/31 5:39, Kees Cook 写道:
>> +bool ex_handler_refcount(const struct exception_table_entry *fixup,
>> +                        struct pt_regs *regs, int trapnr)
>> +{
>> +       int reset;
>> +
>> +       /*
>> +        * If we crossed from INT_MAX to INT_MIN, the OF flag (result
>> +        * wrapped around) and the SF flag (result is negative) will be
>> +        * set. In this case, reset to INT_MAX in an attempt to leave the
>> +        * refcount usable. Otherwise, we've landed here due to producing
>> +        * a negative result from either decrementing zero or operating on
>> +        * a negative value. In this case things are badly broken, so we
>> +        * we saturate to INT_MIN / 2.
>> +        */
>> +       if (regs->flags & (X86_EFLAGS_OF | X86_EFLAGS_SF))
>> +               reset = INT_MAX;
>
>     Should it be like this to indicate that the refcount is wapped from
> INT_MAX to INT_MIN ?
>         if (regs->flags & (X86_EFLAGS_OF | X86_EFLAGS_SF)
>                 == (X86_EFLAGS_OF | X86_EFLAGS_SF))
>
>                 reset = INT_MAX;

Ah yes, thanks for the catch. Yeah, that test is expecting both
condition flags to be set.

I'm still on the fence about the best way to deal with the bad states.
I've been pondering just strictly using a saturation value (INT_MIN /
2), which should offer the same system state protection (except for
the inherent resource leak), but that means there isn't really a good
way to kill an offending process (since after saturation ALL processes
will look like violators). It can be argued that killing the process
doesn't actually provide any benefit since the system is still safe,
though.

>> +       else
>> +               reset = INT_MIN / 2;
>> +       *(int *)regs->cx = reset;

Thanks for looking at this!

-Kees

-- 
Kees Cook
Pixel Security