[Intel-gfx] [PATCH 1/5] drm: Kernel Crash in drm_unlock

[Daniel Vetter]

On Tue, May 05, 2015 at 06:37:51AM +0000, Antoine, Peter wrote:
> On Mon, 2015-05-04 at 15:52 +0200, Daniel Vetter wrote:
> > On Tue, Apr 28, 2015 at 10:52:32AM +0100, chris at chris-wilson.co.uk wrote:
> > > On Tue, Apr 28, 2015 at 10:21:49AM +0100, Dave Gordon wrote:
> > > > On 24/04/15 06:52, Antoine, Peter wrote:
> > > > > I picked up this work due to the following Jira ticket created by the
> > > > > security team (on Android) and was asked to give it a second look and
> > > > > found a few more issues with the hw lock code.
> > > > > 
> > > > > https://jira01.devtools.intel.com/browse/GMINL-5388
> > > > > I/O control on /dev/dri/card0 crashes the kernel (0x4008642b)
> > > > > 
> > > > > It also stops Linux as it kills the driver, I guess it might be possible
> > > > > to reload the gfx driver. On a unpatched system the test that is
> > > > > included in the issue or the igt test that has been posted for the issue
> > > > > will show the problem.
> > > > > 
> > > > > I ran the test on an unpatched system here and the gui stopped and the
> > > > > keyboard stopped responding, so I rebooted. With the patched system I
> > > > > did not need to reboot.
> > > > > 
> > > > > Should I change the SIGTERM to SIGSEGV, not quite the same thing but
> > > > > tooling is better at handling a segfault than a SIGTERM and the
> > > > > application that calls this IOCTL is using an uninitialised hw lock so
> > > > > it is kind of the same as differencing an uninitialised pointer (kind
> > > > > of). Or, I could just remove it, but the bug has been in the code for at
> > > > > least two years (and known about), and I would guess that any code that
> > > > > is calling this is fuzzing the IOCTLs (as this is how the security team
> > > > > found it) and we should reward them with a application exit.
> > > > > 
> > > > > Peter. 
> > > > 
> > > > SIGSEGV would be a better choice.
> > > > 
> > > > SIGTERM is normally sent by a user -- it's the default signal sent by
> > > > kill(1). It's also commonly used to tell a long-running daemon process
> > > > to tidy up and exit cleanly.
> > > > 
> > > > SIGSEGV commonly means "you accessed something that doesn't exist/isn't
> > > > mapped/you don't have permissions for". There are specific subcases that
> > > > can be indicated via the siginfo data; this is from the sigaction(1)
> > > > manpage:
> > > > 
> > > >     The following values can be placed in si_code for a SIGSEGV signal:
> > > > 
> > > >         SEGV_MAPERR    address not mapped to object
> > > > 
> > > >         SEGV_ACCERR    invalid permissions for mapped object
> > > > 
> > > > SIGBUS would also be a possibility but that's generally taken to mean
> > > > that an access got all the way to some physical bus and then faulted,
> > > > whereas SIGSEGV suggests the access was rejected during the
> > > > virtual-to-physical mapping process.
> > > 
> > > None of the above. Just return -EINVAL, -EPERM, -EACCESS as appropriate.
> > 
> > Seconded, we really don't want to be in the business of fixing up the drm
> > design mistakes of the past 15 years. As long as we can fully lock out
> > this particular dragon when running i915 we're imo good enough. The dri1
> > design of a kernel shim driver cooperating with the ums driver for hw
> > ownership is fundamentally unfixable.
> > 
> > Also we can't change any of it for drivers actually using it since it'll
> > break them, which is a big no-go.
> > -Daniel
> 
> I will remove it. But, If you are using this code path the driver/kernel
> will have crashed. It covers a NULL pointer deference, so we are not
> changing the API that anyone is actually using.

With ums/dri1 userspace can crash the kernel. That's pretty much by
design, and it's pretty much unfixable. Trying to fix up the obvious ones
like here is just cargo-culting. The only real option is to make
absolutely sure we can't ever reach these codepaths at all from kms
drivers like i915. Which tends to be a lot more work for auditing, but
will be much more useful.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch