Re: [Intel-gfx] [PATCH 3/5] drm: Possible lock priority escalation.

From: Daniel Vetter <daniel@ffwll.ch>
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: [Intel-gfx] [PATCH 3/5] drm: Possible lock priority escalation.
Date: Tue, 5 May 2015 09:23:07 +0200	[thread overview]
Message-ID: <20150505072307.GX30184@phenom.ffwll.local> (raw)
In-Reply-To: <1430808329.15051.14.camel@peterant-linux>

On Tue, May 05, 2015 at 06:45:30AM +0000, Antoine, Peter wrote:
> On Mon, 2015-05-04 at 15:56 +0200, Daniel Vetter wrote:
> > On Mon, Apr 27, 2015 at 07:52:46PM +0300, Ville Syrjälä wrote:
> > > On Thu, Apr 23, 2015 at 03:07:56PM +0100, Peter Antoine wrote:
> > > > If an application that has a driver lock created, wants the lock the
> > > > kernel context, it is not allowed to. If the call to drm_lock has a
> > > > context of 0, it is rejected. If you set the context to _DRM_LOCK_CONT
> > > > then call drm lock, it will pass the context == DRM_KERNEL_CONTEXT checks.
> > > > But as the DRM_LOCK_CONT bits are not part of the context id this allows
> > > > operations on the DRM_KERNEL_CONTEXT.
> > > > 
> > > > Issue: VIZ-5485
> > > > Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> > 
> > If you're touching code with drm_legacy_ prefix of in such a file you've
> > ended up in the horrible corners of the dri1 dungeons and should head back
> > out pronto ;-)
> > 
> > If we can actually run into this code on production i915 then we need to
> > improve the locks at the door of these dungeons for kms drivers, not try
> > to fix up the mess behind them. That's just plain impossible.
> > 
> > If you want to make really sure we get this right some simple drm igt
> > tests to make sure these codepaths are really dead for kms driver might be
> > good. But otherwise we really can only annotate this as wontfix in
> > code security issue scanners.
> > -Daniel
> > 
> There is a test that covers this fix. This is a simple three line fix
> that stops a userspace driver locking the kernel context. Yes they are
> other problems with this code, but why are they stopping this patch that
> does a simple fix from going in?
> 
> I'll happily drop this patch if it causes more problems that it fixes.

Because we don't want to fix the legacy context crap but instead outright
disable it for everyone (well except nouveau) running in kms code. drm
legacy code really is broken by design, there's no way to fix it.

And worst case we'll end up breaking some old machines in some and then
have to deal with the regression. Which means I won't apply these patches
if we can somehow just disable all that code for i915.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel