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]

Message-ID: <CA+55aFxy6D=cgP4bUObQ2Yg5Z+Hc__rXDD6OeKqanLwuk07WxA@mail.gmail.com>
Date:   Fri, 26 Aug 2016 17:37:20 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     Kees Cook <keescook@...omium.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 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.

In particular, we _have_ occasionally fixed this by turning it into a
link-time error instead (that's in fact the really traditional model).
That makes the errior happen much later, and the error message isn't
nearly as nice (you get something like "undefined reference to unknown
symbol '__copy_to_user_failed' in function xyz" without line numbers
etc nice things). But it cuts down on the false positives that come
from warnings triggering before the final code has actually been
generated.

So one option *might* be to make the copy_to_user checks do an
explicitly constant and static check like

     if (__builtin_constant_p(n) && sz >= 0 && n > sz)
          __copy_to_user_failed();

with the "__copy_to_user_failed()" function declared but never
defined. That way, at link time, if something still references it, you
get a link error and you'll know it's bad.

So something like the attached patch *might* work. As mentioned, it
makes the error messages much less legible if they happen, and it
delays them to link time, so it's not perfect. But it certainly has
the potential of avoiding bogus warnings.

It *seemed* to work in my quick allmodconfig build test, but that may
be because I screwed something up. So take that with a large pinch of
salt.

What do people think? The static built-time errors - if they happen -
really should be pretty exceptional and unusual. So maybe it's ok that
they then would be somewhat cryptic, and you'd have to maybe hunt
(possibly through several layers of inline functions) where the actual
offending user copy then ends up being..

So I'm not happy with this patch, but I also think that the false
positives make the *current* code simply unworkable with current gcc
versions.

Of course, somebody might be able to come up with a better approach
that still gets the nice error messages and avoids the false
positives.

                          Linus

View attachment "patch.diff" of type "text/plain" (4891 bytes)