From mboxrd@z Thu Jan 1 00:00:00 1970 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 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jani Nikula Subject: Re: [Intel-gfx] [PATCH] drm/i915: use static const array for PICK macro Date: Tue, 21 Mar 2017 13:23:59 +0200 Message-ID: <871stqc1ps.fsf@intel.com> References: <20170320215713.3086140-1-arnd@arndb.de> <877f3javde.fsf@intel.com> <20170321103302.fnrt4tnze46grmdi@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20170321103302.fnrt4tnze46grmdi@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Vetter , Arnd Bergmann Cc: Ander Conselvan de Oliveira , intel-gfx@lists.freedesktop.org, Linux Kernel Mailing List , dri-devel , Daniel Vetter List-Id: dri-devel@lists.freedesktop.org T24gVHVlLCAyMSBNYXIgMjAxNywgRGFuaWVsIFZldHRlciA8ZGFuaWVsQGZmd2xsLmNoPiB3cm90 ZToKPiBPbiBUdWUsIE1hciAyMSwgMjAxNyBhdCAwOTo0NDowN0FNICswMTAwLCBBcm5kIEJlcmdt YW5uIHdyb3RlOgo+PiBPbiBUdWUsIE1hciAyMSwgMjAxNyBhdCA5OjI2IEFNLCBKYW5pIE5pa3Vs YQo+PiA8amFuaS5uaWt1bGFAbGludXguaW50ZWwuY29tPiB3cm90ZToKPj4gPiBPbiBNb24sIDIw IE1hciAyMDE3LCBBcm5kIEJlcmdtYW5uIDxhcm5kQGFybmRiLmRlPiB3cm90ZToKPj4gPj4gVGhl IHZhcmFyZ3MgbWFjcm8gdHJpY2sgaW4gX1BJUEUzL19QSFkzL19QT1JUMyB3YXMgbWVhbnQgYXMg YW4gb3B0aW1pemF0aW9uCj4+ID4+IHRvIHNocmluayB0aGUgaTkxNSBrZXJuZWwgbW9kdWxlIGJ5 IGFyb3VuZCAxMDAwIGJ5dGVzLgo+PiA+Cj4+ID4gUmVhbGx5LCBJIGRpZG4ndCBjYXJlIG9uZSBi aXQgYWJvdXQgdGhlIHNpemUgc2hyaW5rLCBJIG9ubHkgY2FyZWQgYWJvdXQKPj4gPiBtYWtpbmcg aXQgZWFzaWVyIGFuZCBsZXNzIGVycm9yIHByb25lIHRvIGluY3JlYXNlIHRoZSBudW1iZXIgb2Yg YXJncyBpbgo+PiA+IGEgbnVtYmVyIG9mIHBsYWNlcy4gTWFpbnRhaW5hYmlsaXR5IGFuZCBjb3Jy ZWN0bmVzcyB3ZXJlIHRoZSBnb2Fscy4gSnVzdAo+PiA+IGZvciB0aGUgcmVjb3JkLiA7KQo+PiAK Pj4gT2suIE15IG9ubHkgaW50ZXJlc3QgaGVyZSBpcyB0aGUgd2FybmluZyBhYm91dCBwb3NzaWJs ZSBzdGFjayBvdmVyZmxvdywKPj4gdGhvdWdoIHRoZSBmYWN0IHRoYXQgS0FTQU4gY29uc2lkZXJz IHRoZSBhcnJheSBjb2RlIHRvIGJlIGZyYWdpbGUgaXMKPj4gYW4gaW5kaWNhdGlvbiB0aGF0IGl0 IGlzIHBlcmhhcHMgYWN0dWFsbHkgZGFuZ2Vyb3VzOiBpZiB3ZSBldmVyIHJ1biBpbnRvCj4+IGEg YnVnIHRoYXQgY2F1c2VzIHRoZSBhcnJheSBpbmRleCB0byBvdmVyZmxvdywgd2UgbWlnaHQgaW4g dGhlb3J5Cj4+IGhhdmUgYSBzZWN1cml0eSBidWcgdGhhdCBsZXRzIHVzZXJzIGFjY2VzcyBhcmJp dHJhcnkga2VybmVsIHBvaW50ZXJzLgo+PiAKPj4gV2hpbGUgdGhlIHJpc2sgZm9yIHRoYXQgYWN0 dWFsbHkgaGFwcGVuaW5nIGlzIHZlcnkgbG93LCB0aGUgb3JpZ2luYWwgY29kZQo+PiB3YXMgc2Fm ZXIgaW4gdGhhdCByZWdhcmQuIE15IHBhdGNoIG9uIHRvcCBvZiB5b3VycyBtZXJlbHkgdHVybnMg YQo+PiBoeXBvdGhldGljYWwgYXJiaXRyYXJ5IHN0YWNrIGFjY2VzcyBpbnRvIGFuIGFyYml0cmFy eSAuZGF0YSBhY2Nlc3MsCj4+IGFuZCBJIGRvbid0IGV2ZW4ga25vdyB3aGljaCBvbmUgd291bGQg YmUgd29yc2UuCj4KPiBFdmVuIHdpdGhvdXQgdGhlc2UgYXJyYXlzLCBpZiB1c2Vyc3BhY2UgY291 bGQgY29udHJvbCB0aGUgaW5kZXggd2UgZmVlZAo+IGludG8gdGhlc2UgeW91IGdldCBhcmJpdHJh cnkgbW1pbyBhY2Nlc3MuIE9yIHNlbWktYXJiaXRyYXJ5IGF0IGxlYXN0Lgo+Cj4gTm9uZSBvZiB0 aGVzZSBhcmUgYnVncyB3ZSBzaG91bGQgZXZlciBsZXQgdGhyb3VnaCwgYW5kIEkgdGhpbmsgd2l0 aCB0aGUKPiBjdXJyZW50IGNvZGUgZGVzaWduICh3aGVyZSB0aGUgZHJpdmVyIGNvbnN0cnVjdHMg c3RydWN0cyB0aGF0IGNvbnRhaW4gdGhlCj4gcmlnaHQgaW5kaXplcywgYW5kIHVzZXJzcGFjZSBv bmx5IGV2ZXIgZ2V0cyB0byBwb2ludCBhdCB0aGVzZSBzdHJ1Y3RzCj4gdXNpbmcgYW4gaWRyIGxv b2t1cCkgbm9uZSBvZiB0aGVzZSBhcmUgbGlrZWx5IHRvIGhhcHBlbi4KClRoYXQncyBhbGwgdHJ1 ZSwgYnV0IEknbSBjdXJpb3VzIGlmIGV4cGxpY2l0IGNoZWNrcyB3b3VsZCBoZWxwCmthc2FuLiBT b21ldGhpbmcgbGlrZToKCmRpZmYgLS1naXQgYS9kcml2ZXJzL2dwdS9kcm0vaTkxNS9pOTE1X3Jl Zy5oIGIvZHJpdmVycy9ncHUvZHJtL2k5MTUvaTkxNV9yZWcuaAppbmRleCAwNGM4ZjY5ZmNjNjIu LjBhYjMyYTA1YjVkOCAxMDA2NDQKLS0tIGEvZHJpdmVycy9ncHUvZHJtL2k5MTUvaTkxNV9yZWcu aAorKysgYi9kcml2ZXJzL2dwdS9kcm0vaTkxNS9pOTE1X3JlZy5oCkBAIC00OCw3ICs0OCw4IEBA IHN0YXRpYyBpbmxpbmUgYm9vbCBpOTE1X21taW9fcmVnX3ZhbGlkKGk5MTVfcmVnX3QgcmVnKQog CXJldHVybiAhaTkxNV9tbWlvX3JlZ19lcXVhbChyZWcsIElOVkFMSURfTU1JT19SRUcpOwogfQog Ci0jZGVmaW5lIF9QSUNLKF9faW5kZXgsIC4uLikgKCgoY29uc3QgdTMyIFtdKXsgX19WQV9BUkdT X18gfSlbX19pbmRleF0pCisjZGVmaW5lIF9QSUNLX05BUkdTKC4uLikgQVJSQVlfU0laRSgoKGNv bnN0IHUzMiBbXSl7IF9fVkFfQVJHU19fIH0pKQorI2RlZmluZSBfUElDSyhfX2luZGV4LCAuLi4p ICgoX19pbmRleCkgPj0gMCAmJiAoX19pbmRleCkgPCBfUElDS19OQVJHUyhfX1ZBX0FSR1NfXykg PyAoKGNvbnN0IHUzMiBbXSl7IF9fVkFfQVJHU19fIH0pW19faW5kZXhdIDogMCkKIAogI2RlZmlu ZSBfUElQRShwaXBlLCBhLCBiKSAoKGEpICsgKHBpcGUpKigoYiktKGEpKSkKICNkZWZpbmUgX01N SU9fUElQRShwaXBlLCBhLCBiKSBfTU1JTyhfUElQRShwaXBlLCBhLCBiKSkKCi0tLQoKQXJuZCwg Y2FuIHlvdSBjaGVjayB0aGF0IHdpdGgga2FzYW4gcGxlYXNlPyAoSSBkb24ndCBoYXZlIGdjYyA3 LikgRm9yIG1lCnRoZSBzaXplIGRpZmYgYWdhaW5zdCBjdXJyZW50IGdpdCBpcwoKICAgIHRleHQJ ICAgZGF0YQkgICAgYnNzCSAgICBkZWMJICAgIGhleAlmaWxlbmFtZQotMTEzNzIzNgkgIDMxMjEx CSAgIDI5NDgJMTE3MTM5NQkgMTFkZmMzCWRyaXZlcnMvZ3B1L2RybS9pOTE1L2k5MTUua28KKzEx Mzk3MDIJICAzMTIxMQkgICAyOTQ4CTExNzM4NjEJIDExZTk2NQlkcml2ZXJzL2dwdS9kcm0vaTkx NS9pOTE1LmtvCgpCUiwKSmFuaS4KCgotLSAKSmFuaSBOaWt1bGEsIEludGVsIE9wZW4gU291cmNl IFRlY2hub2xvZ3kgQ2VudGVyCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNr dG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2Ry aS1kZXZlbAo=