From patchwork Tue Mar 21 11:23:59 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jani Nikula X-Patchwork-Id: 9636421 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 3165D60216 for ; Tue, 21 Mar 2017 11:24:06 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 3469028305 for ; Tue, 21 Mar 2017 11:24:06 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 295B028307; Tue, 21 Mar 2017 11:24:06 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED, T_DKIM_INVALID autolearn=unavailable version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id DE88028305 for ; Tue, 21 Mar 2017 11:24:05 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id DBE466E396; Tue, 21 Mar 2017 11:24:04 +0000 (UTC) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTPS id 2E8B46E38F; Tue, 21 Mar 2017 11:24:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=intel.com; i=@intel.com; q=dns/txt; s=intel; t=1490095443; x=1521631443; h=from:to:cc:subject:in-reply-to:references:date: message-id:mime-version; bh=ST1jJxKSydZnhg3n3VU3Fbu+LEdm3uFBrexs0Lxp7+o=; b=aGYr/3nE285xanYMTwyUpDUP7X6iXf1qKI+3DOVbsArR5Mbv8XxvUNN0 Pn//U+V5J8+hyXRFUBaegZXpJWNrXw==; Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 21 Mar 2017 04:24:02 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,198,1486454400"; d="scan'208";a="836982869" Received: from jnikula-mobl.fi.intel.com (HELO localhost) ([10.237.72.162]) by FMSMGA003.fm.intel.com with ESMTP; 21 Mar 2017 04:24:00 -0700 From: Jani Nikula To: Daniel Vetter , Arnd Bergmann In-Reply-To: <20170321103302.fnrt4tnze46grmdi@phenom.ffwll.local> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <20170320215713.3086140-1-arnd@arndb.de> <877f3javde.fsf@intel.com> <20170321103302.fnrt4tnze46grmdi@phenom.ffwll.local> Date: Tue, 21 Mar 2017 13:23:59 +0200 Message-ID: <871stqc1ps.fsf@intel.com> MIME-Version: 1.0 Cc: Ander Conselvan de Oliveira , David Airlie , intel-gfx@lists.freedesktop.org, Linux Kernel Mailing List , dri-devel , Daniel Vetter Subject: Re: [Intel-gfx] [PATCH] drm/i915: use static const array for PICK macro X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Virus-Scanned: ClamAV using ClamSMTP On Tue, 21 Mar 2017, Daniel Vetter wrote: > On Tue, Mar 21, 2017 at 09:44:07AM +0100, Arnd Bergmann wrote: >> On Tue, Mar 21, 2017 at 9:26 AM, Jani Nikula >> wrote: >> > On Mon, 20 Mar 2017, Arnd Bergmann wrote: >> >> The varargs macro trick in _PIPE3/_PHY3/_PORT3 was meant as an optimization >> >> to shrink the i915 kernel module by around 1000 bytes. >> > >> > Really, I didn't care one bit about the size shrink, I only cared about >> > making it easier and less error prone to increase the number of args in >> > a number of places. Maintainability and correctness were the goals. Just >> > for the record. ;) >> >> Ok. My only interest here is the warning about possible stack overflow, >> though the fact that KASAN considers the array code to be fragile is >> an indication that it is perhaps actually dangerous: if we ever run into >> a bug that causes the array index to overflow, we might in theory >> have a security bug that lets users access arbitrary kernel pointers. >> >> While the risk for that actually happening is very low, the original code >> was safer in that regard. My patch on top of yours merely turns a >> hypothetical arbitrary stack access into an arbitrary .data access, >> and I don't even know which one would be worse. > > Even without these arrays, if userspace could control the index we feed > into these you get arbitrary mmio access. Or semi-arbitrary at least. > > None of these are bugs we should ever let through, and I think with the > current code design (where the driver constructs structs that contain the > right indizes, and userspace only ever gets to point at these structs > using an idr lookup) none of these are likely to happen. That's all true, but I'm curious if explicit checks would help kasan. Something like: --- Arnd, can you check that with kasan please? (I don't have gcc 7.) For me the size diff against current git is text data bss dec hex filename -1137236 31211 2948 1171395 11dfc3 drivers/gpu/drm/i915/i915.ko +1139702 31211 2948 1173861 11e965 drivers/gpu/drm/i915/i915.ko BR, Jani. diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 04c8f69fcc62..0ab32a05b5d8 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -48,7 +48,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg) return !i915_mmio_reg_equal(reg, INVALID_MMIO_REG); } -#define _PICK(__index, ...) (((const u32 []){ __VA_ARGS__ })[__index]) +#define _PICK_NARGS(...) ARRAY_SIZE(((const u32 []){ __VA_ARGS__ })) +#define _PICK(__index, ...) ((__index) >= 0 && (__index) < _PICK_NARGS(__VA_ARGS__) ? ((const u32 []){ __VA_ARGS__ })[__index] : 0) #define _PIPE(pipe, a, b) ((a) + (pipe)*((b)-(a))) #define _MMIO_PIPE(pipe, a, b) _MMIO(_PIPE(pipe, a, b))