From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752793AbdLKMoc (ORCPT ); Mon, 11 Dec 2017 07:44:32 -0500 Received: from mail-ot0-f195.google.com ([74.125.82.195]:42341 "EHLO mail-ot0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751347AbdLKMo3 (ORCPT ); Mon, 11 Dec 2017 07:44:29 -0500 X-Google-Smtp-Source: ACJfBotSu7jUJjXaheJWWKxhyaV8WGTHtp6r5hu+8wngziUMFe2zU7yuULVh3BiGubOZVCu/A4f+mv/9hEvIAK2aoeQ= MIME-Version: 1.0 In-Reply-To: <871stqc1ps.fsf@intel.com> References: <20170320215713.3086140-1-arnd@arndb.de> <877f3javde.fsf@intel.com> <20170321103302.fnrt4tnze46grmdi@phenom.ffwll.local> <871stqc1ps.fsf@intel.com> From: Arnd Bergmann Date: Mon, 11 Dec 2017 13:44:28 +0100 X-Google-Sender-Auth: Wps_RfWfuoAu8FSlQsK0RXVdEgs Message-ID: Subject: Re: [Intel-gfx] [PATCH] drm/i915: use static const array for PICK macro To: Jani Nikula Cc: Daniel Vetter , Ander Conselvan de Oliveira , David Airlie , Linux Kernel Mailing List , dri-devel , Daniel Vetter , Intel Graphics Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Mar 21, 2017 at 12:23 PM, Jani Nikula wrote: > 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 > I just revisited my old patch when I ran into the stack size warning once more, and realized I had not really answered your question earlier. I compared your version to what is in 4.15-rc3 now, and to my version, and confirmed that yours produces the largest code size of the three, and doesn't address the warnings we get, but does cause additional warnings ("comparison of constant '3' with boolean expression is always true"), so that won't get us anywhere. Here are the numbers I get with gcc-8: text data bss dec hex filename 2500045 486453 6912 2993410 2dad02 i915-kasan-4_14.ko 2488028 497909 6912 2992849 2daad1 i915-kasan-arnd.ko 2508814 486453 6912 3002179 2dcf43 i915-kasan-jani.ko 1639798 63269 4448 1707515 1a0dfb i915-nokasan-4.15.ko 1635284 63269 4448 1703001 19fc59 i915-nokasan-arnd.ko 1648331 63269 4448 1716048 1a2f50 i915-nokasan-jani.ko I'll resend my old patch with the original description since I can't easily reproduce it now without your original change, and the code has changed again in the meantime, so I had to slightly adapt my patch to still apply. Arnd From mboxrd@z Thu Jan 1 00:00:00 1970 From: Arnd Bergmann Subject: Re: [PATCH] drm/i915: use static const array for PICK macro Date: Mon, 11 Dec 2017 13:44:28 +0100 Message-ID: References: <20170320215713.3086140-1-arnd@arndb.de> <877f3javde.fsf@intel.com> <20170321103302.fnrt4tnze46grmdi@phenom.ffwll.local> <871stqc1ps.fsf@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <871stqc1ps.fsf@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Jani Nikula Cc: Ander Conselvan de Oliveira , David Airlie , Intel Graphics , Linux Kernel Mailing List , dri-devel , Daniel Vetter List-Id: dri-devel@lists.freedesktop.org T24gVHVlLCBNYXIgMjEsIDIwMTcgYXQgMTI6MjMgUE0sIEphbmkgTmlrdWxhCjxqYW5pLm5pa3Vs YUBsaW51eC5pbnRlbC5jb20+IHdyb3RlOgo+IE9uIFR1ZSwgMjEgTWFyIDIwMTcsIERhbmllbCBW ZXR0ZXIgPGRhbmllbEBmZndsbC5jaD4gd3JvdGU6Cj4+IE9uIFR1ZSwgTWFyIDIxLCAyMDE3IGF0 IDA5OjQ0OjA3QU0gKzAxMDAsIEFybmQgQmVyZ21hbm4gd3JvdGU6Cj4+PiBPbiBUdWUsIE1hciAy MSwgMjAxNyBhdCA5OjI2IEFNLCBKYW5pIE5pa3VsYQo+Pj4gPGphbmkubmlrdWxhQGxpbnV4Lmlu dGVsLmNvbT4gd3JvdGU6Cj4+PiA+IE9uIE1vbiwgMjAgTWFyIDIwMTcsIEFybmQgQmVyZ21hbm4g PGFybmRAYXJuZGIuZGU+IHdyb3RlOgo+Pj4gPj4gVGhlIHZhcmFyZ3MgbWFjcm8gdHJpY2sgaW4g X1BJUEUzL19QSFkzL19QT1JUMyB3YXMgbWVhbnQgYXMgYW4gb3B0aW1pemF0aW9uCj4+PiA+PiB0 byBzaHJpbmsgdGhlIGk5MTUga2VybmVsIG1vZHVsZSBieSBhcm91bmQgMTAwMCBieXRlcy4KPj4+ ID4KPj4+ID4gUmVhbGx5LCBJIGRpZG4ndCBjYXJlIG9uZSBiaXQgYWJvdXQgdGhlIHNpemUgc2hy aW5rLCBJIG9ubHkgY2FyZWQgYWJvdXQKPj4+ID4gbWFraW5nIGl0IGVhc2llciBhbmQgbGVzcyBl cnJvciBwcm9uZSB0byBpbmNyZWFzZSB0aGUgbnVtYmVyIG9mIGFyZ3MgaW4KPj4+ID4gYSBudW1i ZXIgb2YgcGxhY2VzLiBNYWludGFpbmFiaWxpdHkgYW5kIGNvcnJlY3RuZXNzIHdlcmUgdGhlIGdv YWxzLiBKdXN0Cj4+PiA+IGZvciB0aGUgcmVjb3JkLiA7KQo+Pj4KPj4+IE9rLiBNeSBvbmx5IGlu dGVyZXN0IGhlcmUgaXMgdGhlIHdhcm5pbmcgYWJvdXQgcG9zc2libGUgc3RhY2sgb3ZlcmZsb3cs Cj4+PiB0aG91Z2ggdGhlIGZhY3QgdGhhdCBLQVNBTiBjb25zaWRlcnMgdGhlIGFycmF5IGNvZGUg dG8gYmUgZnJhZ2lsZSBpcwo+Pj4gYW4gaW5kaWNhdGlvbiB0aGF0IGl0IGlzIHBlcmhhcHMgYWN0 dWFsbHkgZGFuZ2Vyb3VzOiBpZiB3ZSBldmVyIHJ1biBpbnRvCj4+PiBhIGJ1ZyB0aGF0IGNhdXNl cyB0aGUgYXJyYXkgaW5kZXggdG8gb3ZlcmZsb3csIHdlIG1pZ2h0IGluIHRoZW9yeQo+Pj4gaGF2 ZSBhIHNlY3VyaXR5IGJ1ZyB0aGF0IGxldHMgdXNlcnMgYWNjZXNzIGFyYml0cmFyeSBrZXJuZWwg cG9pbnRlcnMuCj4+Pgo+Pj4gV2hpbGUgdGhlIHJpc2sgZm9yIHRoYXQgYWN0dWFsbHkgaGFwcGVu aW5nIGlzIHZlcnkgbG93LCB0aGUgb3JpZ2luYWwgY29kZQo+Pj4gd2FzIHNhZmVyIGluIHRoYXQg cmVnYXJkLiBNeSBwYXRjaCBvbiB0b3Agb2YgeW91cnMgbWVyZWx5IHR1cm5zIGEKPj4+IGh5cG90 aGV0aWNhbCBhcmJpdHJhcnkgc3RhY2sgYWNjZXNzIGludG8gYW4gYXJiaXRyYXJ5IC5kYXRhIGFj Y2VzcywKPj4+IGFuZCBJIGRvbid0IGV2ZW4ga25vdyB3aGljaCBvbmUgd291bGQgYmUgd29yc2Uu Cj4+Cj4+IEV2ZW4gd2l0aG91dCB0aGVzZSBhcnJheXMsIGlmIHVzZXJzcGFjZSBjb3VsZCBjb250 cm9sIHRoZSBpbmRleCB3ZSBmZWVkCj4+IGludG8gdGhlc2UgeW91IGdldCBhcmJpdHJhcnkgbW1p byBhY2Nlc3MuIE9yIHNlbWktYXJiaXRyYXJ5IGF0IGxlYXN0Lgo+Pgo+PiBOb25lIG9mIHRoZXNl IGFyZSBidWdzIHdlIHNob3VsZCBldmVyIGxldCB0aHJvdWdoLCBhbmQgSSB0aGluayB3aXRoIHRo ZQo+PiBjdXJyZW50IGNvZGUgZGVzaWduICh3aGVyZSB0aGUgZHJpdmVyIGNvbnN0cnVjdHMgc3Ry dWN0cyB0aGF0IGNvbnRhaW4gdGhlCj4+IHJpZ2h0IGluZGl6ZXMsIGFuZCB1c2Vyc3BhY2Ugb25s eSBldmVyIGdldHMgdG8gcG9pbnQgYXQgdGhlc2Ugc3RydWN0cwo+PiB1c2luZyBhbiBpZHIgbG9v a3VwKSBub25lIG9mIHRoZXNlIGFyZSBsaWtlbHkgdG8gaGFwcGVuLgo+Cj4gVGhhdCdzIGFsbCB0 cnVlLCBidXQgSSdtIGN1cmlvdXMgaWYgZXhwbGljaXQgY2hlY2tzIHdvdWxkIGhlbHAKPiBrYXNh bi4gU29tZXRoaW5nIGxpa2U6Cj4KPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL2k5MTUv aTkxNV9yZWcuaCBiL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2k5MTVfcmVnLmgKPiBpbmRleCAwNGM4 ZjY5ZmNjNjIuLjBhYjMyYTA1YjVkOCAxMDA2NDQKPiAtLS0gYS9kcml2ZXJzL2dwdS9kcm0vaTkx NS9pOTE1X3JlZy5oCj4gKysrIGIvZHJpdmVycy9ncHUvZHJtL2k5MTUvaTkxNV9yZWcuaAo+IEBA IC00OCw3ICs0OCw4IEBAIHN0YXRpYyBpbmxpbmUgYm9vbCBpOTE1X21taW9fcmVnX3ZhbGlkKGk5 MTVfcmVnX3QgcmVnKQo+ICAgICAgICAgcmV0dXJuICFpOTE1X21taW9fcmVnX2VxdWFsKHJlZywg SU5WQUxJRF9NTUlPX1JFRyk7Cj4gIH0KPgo+IC0jZGVmaW5lIF9QSUNLKF9faW5kZXgsIC4uLikg KCgoY29uc3QgdTMyIFtdKXsgX19WQV9BUkdTX18gfSlbX19pbmRleF0pCj4gKyNkZWZpbmUgX1BJ Q0tfTkFSR1MoLi4uKSBBUlJBWV9TSVpFKCgoY29uc3QgdTMyIFtdKXsgX19WQV9BUkdTX18gfSkp Cj4gKyNkZWZpbmUgX1BJQ0soX19pbmRleCwgLi4uKSAoKF9faW5kZXgpID49IDAgJiYgKF9faW5k ZXgpIDwgX1BJQ0tfTkFSR1MoX19WQV9BUkdTX18pID8gKChjb25zdCB1MzIgW10peyBfX1ZBX0FS R1NfXyB9KVtfX2luZGV4XSA6IDApCj4KPiAgI2RlZmluZSBfUElQRShwaXBlLCBhLCBiKSAoKGEp ICsgKHBpcGUpKigoYiktKGEpKSkKPiAgI2RlZmluZSBfTU1JT19QSVBFKHBpcGUsIGEsIGIpIF9N TUlPKF9QSVBFKHBpcGUsIGEsIGIpKQo+Cj4gLS0tCj4KPiBBcm5kLCBjYW4geW91IGNoZWNrIHRo YXQgd2l0aCBrYXNhbiBwbGVhc2U/IChJIGRvbid0IGhhdmUgZ2NjIDcuKSBGb3IgbWUKPiB0aGUg c2l6ZSBkaWZmIGFnYWluc3QgY3VycmVudCBnaXQgaXMKPgo+ICAgICB0ZXh0ICAgICAgICAgICBk YXRhICAgICBic3MgICAgIGRlYyAgICAgaGV4IGZpbGVuYW1lCj4gLTExMzcyMzYgICAgICAgICAg MzEyMTEgICAgMjk0OCAxMTcxMzk1ICAxMWRmYzMgZHJpdmVycy9ncHUvZHJtL2k5MTUvaTkxNS5r bwo+ICsxMTM5NzAyICAgICAgICAgIDMxMjExICAgIDI5NDggMTE3Mzg2MSAgMTFlOTY1IGRyaXZl cnMvZ3B1L2RybS9pOTE1L2k5MTUua28KPgoKSSBqdXN0IHJldmlzaXRlZCBteSBvbGQgcGF0Y2gg d2hlbiBJIHJhbiBpbnRvIHRoZSBzdGFjayBzaXplIHdhcm5pbmcgb25jZQptb3JlLCBhbmQgcmVh bGl6ZWQgSSBoYWQgbm90IHJlYWxseSBhbnN3ZXJlZCB5b3VyIHF1ZXN0aW9uIGVhcmxpZXIuCgpJ IGNvbXBhcmVkIHlvdXIgdmVyc2lvbiB0byB3aGF0IGlzIGluIDQuMTUtcmMzIG5vdywgYW5kIHRv IG15IHZlcnNpb24sCmFuZCBjb25maXJtZWQgdGhhdCB5b3VycyBwcm9kdWNlcyB0aGUgbGFyZ2Vz dCBjb2RlIHNpemUgb2YgdGhlIHRocmVlLAphbmQgZG9lc24ndCBhZGRyZXNzIHRoZSB3YXJuaW5n cyB3ZSBnZXQsIGJ1dCBkb2VzIGNhdXNlIGFkZGl0aW9uYWwKd2FybmluZ3MgKCJjb21wYXJpc29u IG9mIGNvbnN0YW50ICczJyB3aXRoIGJvb2xlYW4gZXhwcmVzc2lvbiBpcyBhbHdheXMKdHJ1ZSIp LCBzbyB0aGF0IHdvbid0IGdldCB1cyBhbnl3aGVyZS4gSGVyZSBhcmUgdGhlIG51bWJlcnMgSSBn ZXQKd2l0aCBnY2MtODoKCiAgIHRleHQgICAgZGF0YSAgICAgYnNzICAgICBkZWMgICAgIGhleCBm aWxlbmFtZQoyNTAwMDQ1IDQ4NjQ1MyAgICA2OTEyIDI5OTM0MTAgMmRhZDAyIGk5MTUta2FzYW4t NF8xNC5rbwoyNDg4MDI4IDQ5NzkwOSAgICA2OTEyIDI5OTI4NDkgMmRhYWQxIGk5MTUta2FzYW4t YXJuZC5rbwoyNTA4ODE0IDQ4NjQ1MyAgICA2OTEyIDMwMDIxNzkgMmRjZjQzIGk5MTUta2FzYW4t amFuaS5rbwoxNjM5Nzk4ICAgNjMyNjkgICAgNDQ0OCAxNzA3NTE1IDFhMGRmYiBpOTE1LW5va2Fz YW4tNC4xNS5rbwoxNjM1Mjg0ICAgNjMyNjkgICAgNDQ0OCAxNzAzMDAxIDE5ZmM1OSBpOTE1LW5v a2FzYW4tYXJuZC5rbwoxNjQ4MzMxICAgNjMyNjkgICAgNDQ0OCAxNzE2MDQ4IDFhMmY1MCBpOTE1 LW5va2FzYW4tamFuaS5rbwoKSSdsbCByZXNlbmQgbXkgb2xkIHBhdGNoIHdpdGggdGhlIG9yaWdp bmFsIGRlc2NyaXB0aW9uIHNpbmNlIEkgY2FuJ3QgZWFzaWx5CnJlcHJvZHVjZSBpdCBub3cgd2l0 aG91dCB5b3VyIG9yaWdpbmFsIGNoYW5nZSwgYW5kIHRoZSBjb2RlIGhhcwpjaGFuZ2VkIGFnYWlu IGluIHRoZSBtZWFudGltZSwgc28gSSBoYWQgdG8gc2xpZ2h0bHkgYWRhcHQgbXkKcGF0Y2ggdG8g c3RpbGwgYXBwbHkuCgogICAgICAgIEFybmQKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMu ZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlz dGluZm8vaW50ZWwtZ2Z4Cg==