linux-kernel - Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern versions of gcc

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:   Tue, 30 Aug 2016 14:33:16 -0400
From:   Kees Cook <keescook@...omium.org>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...nel.org>,
        "H . Peter Anvin" <hpa@...or.com>,
        "x86@...nel.org" <x86@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Andy Lutomirski <luto@...capital.net>,
        Steven Rostedt <rostedt@...dmis.org>,
        Brian Gerst <brgerst@...il.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Frederic Weisbecker <fweisbec@...il.com>,
        Byungchul Park <byungchul.park@....com>,
        Nilay Vaish <nilayvaish@...il.com>
Subject: Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern
 versions of gcc

On Mon, Aug 29, 2016 at 10:48 AM, Josh Poimboeuf <jpoimboe@...hat.com> wrote:
> On Fri, Aug 26, 2016 at 05:37:20PM -0700, Linus Torvalds wrote:
>> On Fri, Aug 26, 2016 at 1:56 PM, Josh Poimboeuf <jpoimboe@...hat.com> wrote:
>> >
>> > There's one problem with that though.  It's going to annoy a lot of
>> > people who do allyesconfig/allmodconfig builds because
>> > DEBUG_STRICT_USER_COPY_CHECKS adds several fake warnings.
>>
>> How bad is it?
>>
>> In particular, we've definitely had issues with the "warning"
>> attribute before. Because as you pointed out somewhere elsewhere, the
>> warrning can happen before the call is actually optimized away by a
>> later compiler phase.
>
> So I *think* your patch fixes the wrong problem.  That's probably at
> least somewhat my fault because I misunderstood the issue before and may
> have described it wrong at some point.
>
> AFAICT, gcc isn't doing anything wrong, and the false positives are
> "intentional".
>
> There are in fact two static warnings (which are being silenced for new
> versions of gcc):
>
> 1) "copy_from_user() buffer size is too small"
>
>    This happens when object size and copy size are both const, and copy
>    size > object size.  I didn't see any false positives for this one.
>    So the function warning attribute seems to be working fine here.
>    Your patch "fixed" this warning, but it didn't need fixing.
>
>    Note this scenario is always a bug and so I think it should be
>    changed to *always* be an error, regardless of
>    DEBUG_STRICT_USER_COPY_CHECKS.
>
> 2) "copy_from_user() buffer size is not provably correct"
>
>    This is the (cryptic) false positive warning which happens when I
>    enable __compiletime_object_size() for new compilers (and
>    DEBUG_STRICT_USER_COPY_CHECKS).  It happens when object size is
>    const, but copy size is *not*.  In this case there's no way to
>    compare the two at build time, so it gives the warning.  (Note the
>    warning is a byproduct of the fact that gcc has no way of knowing
>    whether the overflow function will be called, so the call isn't dead
>    code and the warning attribute is activated.)
>
>    So this warning seems to only indicate "this is an unusual pattern,
>    maybe you should check it out" rather than "this is a bug".  It seems
>    to be working "as designed": it has nothing to do with gcc compiler
>    phases AFAICT.
>
>    (Which begs the question: why didn't these warnings appear with older
>    versions of gcc?  I have no idea...)
>
>    I get 102(!) of these warnings with allyesconfig and the
>    __compiletime_object_size() gcc check removed.  I don't know if there
>    are any real bugs hiding in there, but from looking at a small
>    sample, I didn't see any.
>
> So warning 2 seems to be intentional for some reason.  I suggested
> removing it, while keeping the corresponding runtime check.  But
> according to Kees it sometimes finds real bugs.
>
> (Kees, can you confirm that at least some of the recent bugs you found
> were from warning 2?)

Yup, that's correct. In a perfect world, gcc would perform dead-code
analysis and constrain the values of the copy size, removing the "not
provably correct" option when it wasn't possible to hit it. But that
ability regressed, and we started getting more warnings.

> Anyway I don't currently see any doable option other than just removing
> warning 2 (yet still keeping the corresponding copy_user_overflow()
> runtime check).

Yeah. I think consolidating all the usercopy logic into asm-generic is
probably the first task, then we can reexamine adding the warning back
once the gcc bug is fixed.

-Kees

-- 
Kees Cook
Nexus Security