Re: [PATCH 1/5] drm: Kernel Crash in drm_unlock

From: Dave Gordon <david.s.gordon@intel.com>
To: "Antoine, Peter" <peter.antoine@intel.com>
Cc: "daniel.vetter@ffwll.ch" <daniel.vetter@ffwll.ch>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"airlied@redhat.com" <airlied@redhat.com>
Subject: Re: [PATCH 1/5] drm: Kernel Crash in drm_unlock
Date: Tue, 28 Apr 2015 15:56:44 +0100	[thread overview]
Message-ID: <553F9FAC.1020806@intel.com> (raw)
In-Reply-To: <553F512D.7070002@intel.com>

On 28/04/15 10:21, 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.

[snip]

Nope, I've changed my mind about this. I thought the problematic case
was just a process releasing the lock without having acquired it, but on
further examination it's really that the DRM master process has gone
away or otherwise deleted (or never created?) the lock. And THEN the
(non-master?) process tries to release the (now) non-existent lock.

But more importantly, there's existing code in the acquire-lock path
that sends SIGTERM for this case, so obviously the release-lock code
should be as similar as possible.

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx