linux-kernel - Re: [Intel-gfx] [PATCH] drm/i915: use static const array for PICK macro

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 21 Mar 2017 11:33:02 +0100
From:   Daniel Vetter <daniel@...ll.ch>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Jani Nikula <jani.nikula@...ux.intel.com>,
        Ander Conselvan de Oliveira 
        <ander.conselvan.de.oliveira@...el.com>,
        David Airlie <airlied@...ux.ie>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Daniel Vetter <daniel.vetter@...el.com>,
        intel-gfx@...ts.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm/i915: use static const array for PICK
 macro

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@...ux.intel.com> wrote:
> > On Mon, 20 Mar 2017, Arnd Bergmann <arnd@...db.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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch