From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756937AbcH2OsH (ORCPT ); Mon, 29 Aug 2016 10:48:07 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50488 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751195AbcH2OsG (ORCPT ); Mon, 29 Aug 2016 10:48:06 -0400 Date: Mon, 29 Aug 2016 09:48:01 -0500 From: Josh Poimboeuf To: Linus Torvalds Cc: Kees Cook , Thomas Gleixner , Ingo Molnar , "H . Peter Anvin" , "x86@kernel.org" , LKML , Andy Lutomirski , Steven Rostedt , Brian Gerst , Peter Zijlstra , Frederic Weisbecker , Byungchul Park , Nilay Vaish Subject: Re: [PATCH 2/2] mm/usercopy: enable usercopy size checking for modern versions of gcc Message-ID: <20160829144801.nufhyoq5paqffgdc@treble> References: <20160823160629.pgnwzl65zji5l76w@treble> <20160825204759.fxgbdodqpl55fnji@treble> <20160826032729.pbubh5imtpocwjc7@treble> <20160826135533.odypm5rb7irjk3x5@treble> <20160826205627.bugn5xzjiaebwjar@treble> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.6.0.1 (2016-04-01) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Mon, 29 Aug 2016 14:48:05 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Aug 26, 2016 at 05:37:20PM -0700, Linus Torvalds wrote: > On Fri, Aug 26, 2016 at 1:56 PM, Josh Poimboeuf 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