Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965295AbbGHPxs (ORCPT ); Wed, 8 Jul 2015 11:53:48 -0400 Received: from mail-ie0-f171.google.com ([209.85.223.171]:33484 "EHLO mail-ie0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965243AbbGHPxf (ORCPT ); Wed, 8 Jul 2015 11:53:35 -0400 MIME-Version: 1.0 In-Reply-To: <20150708112607.03df9c2a@gandalf.local.home> References: <20150708112607.03df9c2a@gandalf.local.home> Date: Wed, 8 Jul 2015 08:53:34 -0700 X-Google-Sender-Auth: JaMRi_SNICBTKgYzKDF3mYhci6s Message-ID: Subject: Re: [PATCH] Revert: audit: Fix check of return value of strnlen_user() From: Linus Torvalds To: Steven Rostedt Cc: LKML , Jan Kara , Paul Moore , Andrew Morton Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2371 Lines: 63 On Wed, Jul 8, 2015 at 8:26 AM, Steven Rostedt wrote: > > Yes, strnlen_user() returns 0 on fault, but if you look at what len is > set to, than you would notice that on fault len would be -1. Ugh. I hate that. It looks bad, but it's also pointless. It turns out that "len" is unsigned (it's a "size_t"), so "len > MAX_ARG_STRLEN - 1" already takes care of the error case. And people arguing for clarity are clearly wrong, since comparing an unsigned value against "-1" is sure as hell not "clarity". I personally tend to like range comparisons, so "len < 0 || len > MAX_ARG_STRLEN - 1" is both readable and correct (and trivial for the compiler to generate the optimal code for). Sadly I think gcc has occasionally generated warnings for code like that (warning for the "len < 0" test when "len" is unsigned). Compilers that warn for the good kind of safe range tests should be taken out and shot. But it looks like we've either disabled that warning, or gcc has learnt its lesson, because at least the version I have on F22 doesn't warn. Also, the code should use if (WARN_ON_ONCE(..)) { instead of if (unlikely(..)) { WARN_ON(); so I just detest that buggy piece of crap for *so* many reasons. It's also sad that a one-liner commit that claims to "fix" something was this broken to begin with. Grr. Honza, not good. Anyway, to make a long rant more on-point, does this alternative version work for you? diff --git a/kernel/auditsc.c b/kernel/auditsc.c index 09c65640cad6..e85bdfd15fed 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -1021,8 +1021,7 @@ static int audit_log_single_execve_arg( * for strings that are too long, we should not have created * any. */ - if (unlikely((len == 0) || len > MAX_ARG_STRLEN - 1)) { - WARN_ON(1); + if (WARN_ON_ONCE(len < 0 || len > MAX_ARG_STRLEN - 1)) { send_sig(SIGKILL, current, 0); return -1; } because that really looks better to me. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/