From: Jani Nikula <jani.nikula@linux.intel.com> To: Daniel Vetter <daniel@ffwll.ch>, Arnd Bergmann <arnd@arndb.de> Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>, David Airlie <airlied@linux.ie>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, dri-devel <dri-devel@lists.freedesktop.org>, Daniel Vetter <daniel.vetter@intel.com>, intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH] drm/i915: use static const array for PICK macro Date: Tue, 21 Mar 2017 13:23:59 +0200 [thread overview] Message-ID: <871stqc1ps.fsf@intel.com> (raw) In-Reply-To: <20170321103302.fnrt4tnze46grmdi@phenom.ffwll.local> On Tue, 21 Mar 2017, Daniel Vetter <daniel@ffwll.ch> 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 >> <jani.nikula@linux.intel.com> wrote: >> > On Mon, 20 Mar 2017, Arnd Bergmann <arnd@arndb.de> 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
WARNING: multiple messages have this Message-ID (diff)
From: Jani Nikula <jani.nikula@linux.intel.com> To: Daniel Vetter <daniel@ffwll.ch>, Arnd Bergmann <arnd@arndb.de> Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>, intel-gfx@lists.freedesktop.org, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, dri-devel <dri-devel@lists.freedesktop.org>, Daniel Vetter <daniel.vetter@intel.com> Subject: Re: [Intel-gfx] [PATCH] drm/i915: use static const array for PICK macro Date: Tue, 21 Mar 2017 13:23:59 +0200 [thread overview] Message-ID: <871stqc1ps.fsf@intel.com> (raw) In-Reply-To: <20170321103302.fnrt4tnze46grmdi@phenom.ffwll.local> On Tue, 21 Mar 2017, Daniel Vetter <daniel@ffwll.ch> 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 >> <jani.nikula@linux.intel.com> wrote: >> > On Mon, 20 Mar 2017, Arnd Bergmann <arnd@arndb.de> 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2017-03-21 11:24 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-03-20 21:56 [PATCH] drm/i915: use static const array for PICK macro Arnd Bergmann 2017-03-20 21:56 ` Arnd Bergmann 2017-03-20 23:28 ` ✗ Fi.CI.BAT: warning for " Patchwork 2017-03-21 8:26 ` [PATCH] " Jani Nikula 2017-03-21 8:26 ` Jani Nikula 2017-03-21 8:44 ` Arnd Bergmann 2017-03-21 8:44 ` Arnd Bergmann 2017-03-21 10:33 ` [Intel-gfx] " Daniel Vetter 2017-03-21 10:33 ` Daniel Vetter 2017-03-21 11:23 ` Jani Nikula [this message] 2017-03-21 11:23 ` [Intel-gfx] " Jani Nikula 2017-03-31 13:37 ` Arnd Bergmann 2017-03-31 13:37 ` Arnd Bergmann 2017-12-11 12:44 ` Arnd Bergmann 2017-12-11 12:44 ` Arnd Bergmann 2017-03-21 13:16 ` ✗ Fi.CI.BAT: warning for " Patchwork
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=871stqc1ps.fsf@intel.com \ --to=jani.nikula@linux.intel.com \ --cc=airlied@linux.ie \ --cc=ander.conselvan.de.oliveira@intel.com \ --cc=arnd@arndb.de \ --cc=daniel.vetter@intel.com \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=intel-gfx@lists.freedesktop.org \ --cc=linux-kernel@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.