Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757033AbdCULYF (ORCPT ); Tue, 21 Mar 2017 07:24:05 -0400 Received: from mga14.intel.com ([192.55.52.115]:23681 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756364AbdCULYD (ORCPT ); Tue, 21 Mar 2017 07:24:03 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,198,1486454400"; d="scan'208";a="836982869" From: Jani Nikula To: Daniel Vetter , Arnd Bergmann Cc: Ander Conselvan de Oliveira , David Airlie , Linux Kernel Mailing List , dri-devel , Daniel Vetter , intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH] drm/i915: use static const array for PICK macro 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 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2926 Lines: 65 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: 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)) --- 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. -- Jani Nikula, Intel Open Source Technology Center