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

[Daniel Vetter <daniel@ffwll.ch>]

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

> > ---
> >  drivers/gpu/drm/drm_context.c | 6 +++---
> >  drivers/gpu/drm/drm_lock.c    | 4 ++--
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_context.c b/drivers/gpu/drm/drm_context.c
> > index 96350d1..1febcd3 100644
> > --- a/drivers/gpu/drm/drm_context.c
> > +++ b/drivers/gpu/drm/drm_context.c
> > @@ -123,7 +123,7 @@ void drm_legacy_ctxbitmap_flush(struct drm_device *dev, struct drm_file *file)
> >  
> >  	list_for_each_entry_safe(pos, tmp, &dev->ctxlist, head) {
> >  		if (pos->tag == file &&
> > -		    pos->handle != DRM_KERNEL_CONTEXT) {
> > +		    _DRM_LOCKING_CONTEXT(pos->handle) != DRM_KERNEL_CONTEXT) {
> >  			if (dev->driver->context_dtor)
> >  				dev->driver->context_dtor(dev, pos->handle);
> >  
> > @@ -342,7 +342,7 @@ int drm_legacy_addctx(struct drm_device *dev, void *data,
> >  	struct drm_ctx *ctx = data;
> >  
> >  	ctx->handle = drm_legacy_ctxbitmap_next(dev);
> > -	if (ctx->handle == DRM_KERNEL_CONTEXT) {
> > +	if (_DRM_LOCKING_CONTEXT(ctx->handle) == DRM_KERNEL_CONTEXT) {
> >  		/* Skip kernel's context and get a new one. */
> >  		ctx->handle = drm_legacy_ctxbitmap_next(dev);
> >  	}
> > @@ -449,7 +449,7 @@ int drm_legacy_rmctx(struct drm_device *dev, void *data,
> >  	struct drm_ctx *ctx = data;
> >  
> >  	DRM_DEBUG("%d\n", ctx->handle);
> > -	if (ctx->handle != DRM_KERNEL_CONTEXT) {
> > +	if (_DRM_LOCKING_CONTEXT(ctx->handle) != DRM_KERNEL_CONTEXT) {
> >  		if (dev->driver->context_dtor)
> >  			dev->driver->context_dtor(dev, ctx->handle);
> >  		drm_legacy_ctxbitmap_free(dev, ctx->handle);
> 
> How about just fixing the end parameter passed to idr_alloc()? AFAICS
> that would take care of the context code.
> 
> Well, there are a few more issues with the code:
> - not properly checking for negative return value from idr_alloc()
> - leaking the ctx id on kmalloc() error
> - pointless check for idr_alloc() returning 0 even though the min is 1
> 
> > diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
> > index 070dd5d..94500930 100644
> > --- a/drivers/gpu/drm/drm_lock.c
> > +++ b/drivers/gpu/drm/drm_lock.c
> > @@ -63,7 +63,7 @@ int drm_legacy_lock(struct drm_device *dev, void *data,
> >  
> >  	++file_priv->lock_count;
> 
> While you're poking around this dungeopn, maybe you can kill lock_count?
> We never seem to decrement it, and it's only checked in drm_legacy_i_have_hw_lock().
> 
> >  
> > -	if (lock->context == DRM_KERNEL_CONTEXT) {
> > +	if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) {
> >  		DRM_ERROR("Process %d using kernel context %d\n",
> >  			  task_pid_nr(current), lock->context);
> >  		return -EINVAL;
> > @@ -153,7 +153,7 @@ int drm_legacy_unlock(struct drm_device *dev, void *data, struct drm_file *file_
> >  	struct drm_lock *lock = data;
> >  	struct drm_master *master = file_priv->master;
> >  
> > -	if (lock->context == DRM_KERNEL_CONTEXT) {
> > +	if (_DRM_LOCKING_CONTEXT(lock->context) == DRM_KERNEL_CONTEXT) {
> >  		DRM_ERROR("Process %d using kernel context %d\n",
> >  			  task_pid_nr(current), lock->context);
> >  		return -EINVAL;
> 
> These two changes look OK to me.
> 
> > -- 
> > 1.9.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
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