From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753825AbbIKUwf (ORCPT ); Fri, 11 Sep 2015 16:52:35 -0400 Received: from mail-wi0-f178.google.com ([209.85.212.178]:37269 "EHLO mail-wi0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751696AbbIKUwd (ORCPT ); Fri, 11 Sep 2015 16:52:33 -0400 Date: Fri, 11 Sep 2015 23:52:29 +0300 From: Alexey Dobriyan To: Linus Torvalds Cc: Andrew Morton , Rasmus Villemoes , Linux Kernel Mailing List Subject: Re: [patch 27/95] scanf: fix type range overflow Message-ID: <20150911205229.GA6679@p183.telecom.by> References: <55f0b472.ReLomj/XdFgiHSkY%akpm@linux-foundation.org> <20150910112831.GA1748@p183.telecom.by> 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 On Thu, Sep 10, 2015 at 11:37:53AM -0700, Linus Torvalds wrote: 1. Please use more caps, thread is rapidly dropping off Hacker News front page. 2. Security was never a concern here (or a very small one), this is all your imagination thinking strlcpy 2.0 is coming. Security was a concern when kstrto*_from_user() was added because people were copying and parsing numbers by hand so the opportunities for mistakes were real. I've checked grsec patches, they don't touch kstrto*_from_user(), which gives some hope that the code is all right. But then some idiot decided that kernel should also parse whitespace so the code like below couldn't be simplified: memset(buffer, 0, sizeof(buffer)); if (count > sizeof(buffer) - 1) count = sizeof(buffer) - 1; if (copy_from_user(buffer, buf, count)) return -EFAULT; rv = kstrtoint(strstrip(buffer), 0, &make_it_fail); if (rv < 0) return rv; if (make_it_fail < 0 || make_it_fail > 1) return -EINVAL; Returning to parse_integer(), whole thing is a matter of general robustness (no "be liberal in what you accept" rubbish) and hopefully nicer interface which is pleasant to use (1 function instead of 4, etc). 3. Your point that unchecked errors will result in bogus pointer is a valid one and I admit this is the case. Compiler warning on unchecked error in this case require gcc plugin or external checker or something non-trivial which kernel doesn't use. Quite sad. 4. Overall notion that overflow integer can't be security issue is a laughable one. I can't give you kernel example (kernel doesn't munge integers much obviously) but I can give you userspace example. with libpcre. Version 8.37 has notation (?[0-9]+) for forward/backward reference group. Internally group number is parsed into variable "int recno;" at pcre_compile.c:7324 recno = 0; while(IS_DIGIT(*ptr)) recno = recno * 10 + *ptr++ - CHAR_0; Typical code without overflow check. Later forward reference groups (starting with sign "+" or signless) are distinguished from backward reference groups (those starting with "-"). And there is very final check for overflowing past the number of match groups: if (recno != 0) called = PRIV(find_bracket)(cd->start_code, utf, recno); /* Forward reference */ if (called == NULL) { ===> if (recno > cd->final_bracount) { *errorcodeptr = ERR15; goto FAILED; } /* Fudge the value of "called" so that when it is inserted as an offset below, what it actually inserted is the reference number of the group. Then remember the forward reference. */ called = cd->start_code + recno; So without type overflow check it is possible to supply seemingly positive number which will be parsed as negative and sneak in negative "recno" past all checks (signed variables for the win!). Real example: the following regex will crach pcre-8.37 with NULL pointer dereference (which is fallout, real bug happens earlier) if you try to pcre_compile()+pcre_study(PCRE_STUDY_JIT_COMPILE) ((?='))(?=(/(='D))?(?