From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755702AbcHZDvl (ORCPT ); Thu, 25 Aug 2016 23:51:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34524 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751316AbcHZDvk (ORCPT ); Thu, 25 Aug 2016 23:51:40 -0400 Date: Thu, 25 Aug 2016 22:27:29 -0500 From: Josh Poimboeuf To: Kees Cook Cc: Linus Torvalds , 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: <20160826032729.pbubh5imtpocwjc7@treble> References: <20160823160629.pgnwzl65zji5l76w@treble> <20160825204759.fxgbdodqpl55fnji@treble> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit 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.31]); Fri, 26 Aug 2016 03:27:32 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Aug 25, 2016 at 10:14:36PM -0400, Kees Cook wrote: > On Thu, Aug 25, 2016 at 4:47 PM, Josh Poimboeuf wrote: > > On Tue, Aug 23, 2016 at 10:37:43PM -0400, Kees Cook wrote: > >> On Tue, Aug 23, 2016 at 3:28 PM, Josh Poimboeuf wrote: > >> > This is a revert of: > >> > > >> > 2fb0815c9ee6 ("gcc4: disable __compiletime_object_size for GCC 4.6+") > >> > > >> > The goal of that commit was to silence the "provably correct" gcc > >> > warnings. But it went too far: it also disabled the runtime warnings. > >> > > >> > Now that the pretty much useless gcc warnings have been properly > >> > disposed of with the previous patch, re-enable this checking on modern > >> > versions of gcc so we can get the runtime warnings again. > >> > >> As far as I know, this will still be broken since it's > >> __builtin_object_size() that is buggy. Maybe I'm misunderstanding > >> which piece is busted, though? > > > > What specifically is buggy with __builtin_object_size()? Looking at the > > generated code for a few of the "provably correct" warning sites, the > > values generated by __builtin_object_size() are correct. > > > > I think the problem is really related to the compile-time warning > > function attribute used by __copy_to_user_overflow(). The warning is > > printed when gcc *can* determine the object size but it *can't* > > determine the copy size. The warning just means that, even though the > > object has a const size, gcc isn't able to prove that the overflow won't > > happen. > > > > As an example, here's one of the warnings: > > > > In file included from /home/jpoimboe/git/linux/include/linux/uaccess.h:5:0, > > from /home/jpoimboe/git/linux/arch/x86/include/asm/stacktrace.h:9, > > from /home/jpoimboe/git/linux/arch/x86/include/asm/perf_event.h:246, > > from /home/jpoimboe/git/linux/include/linux/perf_event.h:24, > > from /home/jpoimboe/git/linux/kernel/sys.c:16: > > In function ‘copy_to_user.part.10’, > > inlined from ‘copy_to_user’, > > inlined from ‘override_release.part.11’ at /home/jpoimboe/git/linux/kernel/sys.c:1136:9: > > /home/jpoimboe/git/linux/arch/x86/include/asm/uaccess.h:723:46: warning: call to ‘__copy_to_user_overflow’ declared with attribute warning: copy_to_user() buffer size is not provably correct > > #define __copy_to_user_overflow(size, count) __copy_to_user_overflow() > > ^~~~~~~~~~~~~~~~~~~~~~~~~ > > /home/jpoimboe/git/linux/arch/x86/include/asm/uaccess.h:791:3: note: in expansion of macro ‘__copy_to_user_overflow’ > > __copy_to_user_overflow(sz, n); > > ^~~~~~~~~~~~~~~~~~~~~~~ > > > > This is from override_release()'s use of copy_to_user(). The object > > code shows that __builtin_object_size() correctly reports 65 bytes for > > the 'buf' object size. But the copy size ('copy + 1') isn't known at > > compile-time. Thus the (bogus) warning. > > > > Maybe I'm missing something but I don't even see a gcc bug. To me it > > looks like a mismatch in expectations between the code and the compiler. > > Ah, yes, I had a total brain failure. This is what I get trying to do > email between sessions at a conference. :) > > Okay, right. __builtin_object_size() is totally fine, I absolutely > misspoke: it's the resolution of const value ranges. I wouldn't expect > gcc to warn here, though, since "copy + 1" isn't a const value... Look at the code again :-) __copy_to_user_overflow(), which does the "provably correct" warning, is "called" when the copy size is non-const (and the object size is const). So "copy + 1" being non-const is consistent with the warning. -- Josh