Message-ID: <20160829144801.nufhyoq5paqffgdc@treble>
Date:   Mon, 29 Aug 2016 09:48:01 -0500
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
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 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?)

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).

-- 
Josh

Powered by blists - more mailing lists