From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751722AbbJEOeA (ORCPT ); Mon, 5 Oct 2015 10:34:00 -0400 Received: from mail-wi0-f180.google.com ([209.85.212.180]:37042 "EHLO mail-wi0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751032AbbJEOd7 (ORCPT ); Mon, 5 Oct 2015 10:33:59 -0400 Date: Mon, 5 Oct 2015 16:33:54 +0200 From: Ingo Molnar To: Linus Torvalds Cc: Chris Metcalf , Thomas Gleixner , Peter Zijlstra , Borislav Petkov , open list , "H. Peter Anvin" Subject: Re: [PATCH] string: Improve the generic strlcpy() implementation Message-ID: <20151005143354.GB7478@gmail.com> References: <55F1DD53.1070102@ezchip.com> <20151005112700.GA1096@gmail.com> <20151005115355.GA27073@gmail.com> <20151005131524.GA807@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Linus Torvalds wrote: > On Oct 5, 2015 14:15, "Ingo Molnar" wrote: > > > > Hm, so GCC (v4.9.2) will only warn about this bug if -Wtype-limits is > enabled > > explicitly: > > Some of the warnings are really nasty, and cause people to write worse code. > > For example, this is inherently good code: > > if (x < 0 || x > MAXLEN) > return -EINVAL; > > and a compiler that warns about that is pure and utter crap. Obviously. > Agreed? > > Now, imagine that "x" here is some random type. Maybe it's s "char" and you > don't even know the sign. Maybe it's "loff_t". Maybe it's "size_t", or > whatever. > > Note how that test is correct *regardless* of the sign of the type. A > compiler that warns about the "x < 0" part just because x happens to be > unsigned is a bad bad compiler, and makes people remove that check, even > though it's good for readability, and good for robustness wrt changing the > type. Hm, so there's a flip side here - if we consider 'example 6)' in my previous mail: kernel/auditsc.c:1027:23: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits] size_t len, len_left, to_send; ... if (WARN_ON_ONCE(len < 0 || len > MAX_ARG_STRLEN - 1)) { Now if this code was written as: if (WARN_ON_ONCE(len < 0)) { then it would be a clear bug, right? So we could solve that by adding a generic range check: static inline int range_ok(unsigned long low, unsigned long val, unsigned long high) { if (val < low) return 0; if (val >= high) return 0; return 1; } and we could write this: if (len < 0 || len > MAX_ARG_STRLEN - 1) { as: if (!range_ok(0, len, MAX_ARG_STRLEN)) { ? That kind of construct: - is robust against a changed type for 'len' - is robust against these common typos for open coded security checks: if (len <= 0 || len > MAX_ARG_STRLEN - 1) { if (len < 0 || len >= MAX_ARG_STRLEN - 1) { if (len < 0 || len > MAX_ARG_STRLEN) { the first and second ones over-check and are harmless in this context, the third one is harmful because it does not catch the MAX_ARG_STRLEN case. - it would also clearly document range checking performed in a function that gets untrusted data. Hypothetically, if this was acceptable then we could use this in the cases where GCC generates a bogus warning. But ... no strong feelings. Just found it weird that GCC let my bug slip through. Thanks, Ingo